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: The tick is active on idle adaptive-tick CPUs when
 /dev/cpu_dma_latency is used
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc:     Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Frederic Weisbecker <frederic@kernel.org>
References: <79b247b3-e056-610e-9a07-e685dfdaa6c9@gmail.com>
 <7332404.L1nL2KBT3s@kreacher>
 <6ef6b96e-1772-6e80-60cf-eb57af618e99@gmail.com>
 <312565511.gEFFlSTcEG@kreacher>
From:   Thomas Lindroth <thomas.lindroth@gmail.com>
Message-ID: <2828f202-ae04-7069-c75f-328f52a8d938@gmail.com>
Date:   Thu, 11 Jul 2019 18:24:20 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <312565511.gEFFlSTcEG@kreacher>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 7/10/19 12:22 PM, Rafael J. Wysocki wrote:
> On Saturday, July 6, 2019 3:02:11 PM CEST Thomas Lindroth wrote:
>> On 7/6/19 1:06 PM, Rafael J. Wysocki wrote:
>>> The patch is below, but note that it adds the tick stopping overhead to the idle loop
>>> for CPUs that are not adaptive-tick and when PM QoS latency constraints are used
>>> which is not desirable in general.
>>>
>>> Please test it, but as I said above, the real solution appears to be to treat adaptive-tick
>>> CPUs in a special way in the idle loop.
>>>
>>> ---
>>>    drivers/cpuidle/governors/menu.c |   16 +++++-----------
>>>    1 file changed, 5 insertions(+), 11 deletions(-)
>>>
>>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>>> @@ -302,9 +302,10 @@ static int menu_select(struct cpuidle_dr
>>>    	     !drv->states[0].disabled && !dev->states_usage[0].disable)) {
>>>    		/*
>>>    		 * In this case state[0] will be used no matter what, so return
>>> -		 * it right away and keep the tick running.
>>> +		 * it right away and keep the tick running if state[0] is a
>>> +		 * polling one.
>>>    		 */
>>> -		*stop_tick = false;
>>> +		*stop_tick = !!(drv->states[0].flags & CPUIDLE_FLAG_POLLING);
>>>    		return 0;
>>>    	}
>>>    
>>> @@ -395,16 +396,9 @@ static int menu_select(struct cpuidle_dr
>>>    
>>>    			return idx;
>>>    		}
>>> -		if (s->exit_latency > latency_req) {
>>> -			/*
>>> -			 * If we break out of the loop for latency reasons, use
>>> -			 * the target residency of the selected state as the
>>> -			 * expected idle duration so that the tick is retained
>>> -			 * as long as that target residency is low enough.
>>> -			 */
>>> -			predicted_us = drv->states[idx].target_residency;
>>> +		if (s->exit_latency > latency_req)
>>>    			break;
>>> -		}
>>> +
>>>    		idx = i;
>>>    	}
>>
>> I tested the patch and it appears to work. Idle CPUs now have ticks disabled even
>> when /dev/cpu_dma_latency is used.
> 
> OK, thanks, but as I said previously, you'd see the problem again with the PM QoS
> latency constraint set to 0, which is somewhat inconsistent.  Also, this fix is
> specific to the menu governor and the behavior should not depend on the
> governor here IMO, so I have another patch to try (appended).
> 
> Please test it (instead of the previous one) and report back.
> 
> ---
>   kernel/sched/idle.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -191,7 +191,8 @@ static void cpuidle_idle_call(void)
>   		 */
>   		next_state = cpuidle_select(drv, dev, &stop_tick);
>   
> -		if (stop_tick || tick_nohz_tick_stopped())
> +		if (stop_tick || tick_nohz_tick_stopped() ||
> +		    !housekeeping_cpu(dev->cpu, HK_FLAG_TICK))
>   			tick_nohz_idle_stop_tick();
>   		else
>   			tick_nohz_idle_retain_tick();
> 

I tested this patch and it seems to work fine using the menu governor
and PM QoS latency constraints matching each C-state including 0.
I didn't test the TEO governor.