Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC v6 1/5] sched: Introduce switch to enable TurboSched for
 task packing
To:     Tim Chen <tim.c.chen@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Cc:     peterz@infradead.org, mingo@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, patrick.bellasi@matbug.net,
        valentin.schneider@arm.com, pavel@ucw.cz, dsmythies@telus.net,
        qperret@google.com
References: <20200121063307.17221-1-parth@linux.ibm.com>
 <20200121063307.17221-2-parth@linux.ibm.com>
 <0fb8fa5c-0edd-913d-912f-df383a3d4007@linux.intel.com>
From:   Parth Shah <parth@linux.ibm.com>
Date:   Thu, 23 Jan 2020 12:05:51 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <0fb8fa5c-0edd-913d-912f-df383a3d4007@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <1f99c7cc-223c-302d-2c42-a3933e2d8877@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 1/23/20 3:07 AM, Tim Chen wrote:
> On 1/20/20 10:33 PM, Parth Shah wrote:
>> Create a static key which allows to enable or disable TurboSched feature at
>> runtime.
>>
>> This key is added in order to enable the TurboSched feature only when
>> required. This helps in optimizing the scheduler fast-path when the
>> TurboSched feature is disabled.
>>
>> Also provide get/put methods to keep track of the tasks using the
>> TurboSched feature and also refcount classified background tasks. This
>> allows to enable the feature on setting first task classified as background
>> noise, similarly disable the feature on unsetting of such last task.
>>
>> Signed-off-by: Parth Shah <parth@linux.ibm.com>
>> ---
>>  kernel/sched/core.c  | 25 +++++++++++++++++++++++++
>>  kernel/sched/sched.h | 12 ++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index a9e5d157b1a5..dfbb52d66b29 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -73,6 +73,31 @@ __read_mostly int scheduler_running;
>>   */
>>  int sysctl_sched_rt_runtime = 950000;
>>  
>> +#ifdef CONFIG_SCHED_SMT
>> +DEFINE_STATIC_KEY_FALSE(__turbo_sched_enabled);
>> +static DEFINE_MUTEX(turbo_sched_lock);
>> +static int turbo_sched_count;
>> +
>> +void turbo_sched_get(void)
>> +{
>> +	mutex_lock(&turbo_sched_lock);
>> +	if (!turbo_sched_count++)
>> +		static_branch_enable(&__turbo_sched_enabled);
> 
> If you use static_branch_inc(&__turbo_sched_enabled) and
> static_branch_dec(&__turbo_sched_enabled),  you don't have
> to define turbo_sched_count. And turbo_sched_lock is
> also unnecessary as static_branch_inc/dec are atomic.
> 

That's a good suggestion. I will make those changes in the next version.

>> +	mutex_unlock(&turbo_sched_lock);
>> +}
>> +
>> +void turbo_sched_put(void)
>> +{
>> +	mutex_lock(&turbo_sched_lock);
>> +	if (!--turbo_sched_count)
>> +		static_branch_disable(&__turbo_sched_enabled);
>> +	mutex_unlock(&turbo_sched_lock);
>> +}
>> +#else
>> +void turbo_sched_get(void) { return ; }
>> +void turbo_sched_get(void) { return ; }
> 
> Double definition of turbo_sched_get.
> You probably meant turbo_sched_put in the second definition.

yes, my bad. I meant turbo_sched_put() instead.


Thanks,
Parth

> 
> Tim
>