Received-SPF: pass (google.com: domain of linux-kernel+bounces-36639-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <f7db8b6f-d97d-630d-28b3-97062fa2c7cc@amd.com>
Date: Wed, 24 Jan 2024 14:05:23 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH] sched/fair: Skip newidle_balance() when an idle CPU is
 woken up to process an IPI
Content-Language: en-US
To: David Vernet <void@manifault.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>, linux-kernel@vger.kernel.org,
 mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
 vincent.guittot@linaro.org, dietmar.eggemann@arm.com, rostedt@goodmis.org,
 bsegall@google.com, mgorman@suse.de, bristot@redhat.com,
 vschneid@redhat.com, gautham.shenoy@amd.com
References: <20240119084548.2788-1-kprateek.nayak@amd.com>
 <b4f5ac150685456cf45a342e3bb1f28cdd557a53.camel@linux.intel.com>
 <21c8694c-26e4-3bc1-edd8-2267b0164a09@amd.com>
 <20240123211756.GA221793@maniforge>
From: K Prateek Nayak <kprateek.nayak@amd.com>
In-Reply-To: <20240123211756.GA221793@maniforge>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?YWVaZDBsanVXRklJK2h3UjE2R0pmRGxqRncrazFRdlVNN2hDUmlPQWltTUZv?=
 =?utf-8?B?SDJCSW5WUEpmMUZGNkpUdXRoZEhXRlN1NWVLTHdrR2lwR2JsUlpsaHpvNzdB?=
 =?utf-8?B?amU2Y0F6aU0xNGs5U2dRSHRiRlJ4cmIwSDJzaDJLR3FWNFpId21iSVRPS2Rj?=
 =?utf-8?B?eXcyL3l5aEo2dVBqYXZremxrckdVbm9hMG51SVJYZ1N0SGM0aWVhZUpUdndx?=
 =?utf-8?B?T1N3TDQ1WkZXQmtyZkl0RUtSekFwLzdaMjc1OVZRN1ZLTFNhb3JvV1A2ZkZE?=
 =?utf-8?B?bGROSlh3WVBjUnp5UlRZeUtqOGxFUm1qNW56MC92UHorbEhVSzN4bCtncmgr?=
 =?utf-8?B?UlptY1hWNlowSytMbFhxSWN3ZVBFMS85eVNaMUlIVWt5RXQvTGJDWlpuMThG?=
 =?utf-8?B?b09IaS8wM0RKcnhPa3doQUNwTURZNHlOdFdwdGFDTlN3VmIwdDdkdEFvcWkw?=
 =?utf-8?B?OXlaNGl5VSsrUnNMZ2c0RXAvRTlUNnlwWjM4dXYrRFlicVNYc0NzQ1o0UUNv?=
 =?utf-8?B?ek4wWFovbnlXMFFpSXUxUTZ1MTZwZFBna3FxaTUzRnhHditObzNtQzZPcW0x?=
 =?utf-8?B?UllnWFgvNThEcFlrd1lYUDJZOTg2bzcxa0QwMnNYN2JzRG16OWNGTStDWDcx?=
 =?utf-8?B?SHIrZXltU1RXZG44clJDTm44Yk1JSTV2SUMwYXZZSG9nbjMwMERNa1VtMnUv?=
 =?utf-8?B?S0NueS9rNFMvdUdrNVpCRXNjU0k5dE5yenU4YnJQMU5xQVdYcmxOb3hHTjZ4?=
 =?utf-8?B?NnVIdUtkZ0VSbzIrNmJKTVI3VmdLODZJa1hvNmhhSEZnemRFQ1BUMnVpUG9h?=
 =?utf-8?B?MXRpSE1uV1RJWXM5MmRjZDBHb1p1N1RFNnQ5WUZUeld1UDk1U0NVMmJBcWlW?=
 =?utf-8?B?bWlBVkZTM2lHd2NMT3VIOC9qQ0tPNkozOUxZRUZmL3dFclROY3AvbHRNWVBp?=
 =?utf-8?B?UTNweGdDSGljVlJsanUrZzIrZmc3bWNYNTl5RktOSnd1amk4bTVMS29zUHho?=
 =?utf-8?B?QjNSOWo3dVVZK29VeXoxMllXOFQ1UFlYVHg0RjNjd2JPQmZOQUFaay9MZkxr?=
 =?utf-8?B?aFpiN3kzNGpsUHB4ZEl1Und6dktrTU5lU1VQNG90aVFqeGZkWFVGQ2JNS2tW?=
 =?utf-8?B?bkN3V0xxQVNTeEgxYUEyalRrTlJWS1RTSy9rS2ttRWZkdmlOSzBGaCtKd0xQ?=
 =?utf-8?B?VWQrZDlyZjZESHlOaVhxWjF1V3JKYVV5cUJPYmZFV09MWUhObzZ0bUp1ZjdN?=
 =?utf-8?B?M3VGYUhYNVpndzVOK29EYXpkVXJOdDZwQW9KbWYzUEkxV1BsUVFsSkFFVTJq?=
 =?utf-8?B?eFVxdTQyM2RHc08yVFNab3BjTkxOeDNXMzRzQ1lhcCtsZ1ZRb3F5SWRjSzRy?=
 =?utf-8?B?WTkvWmpPN0FGQkpNOTZNSlNvTnJxRnFRQkVJMXV5UFhRbDlLaUMwSHUySVBh?=
 =?utf-8?B?RWUzdVNEbWRCejNGb1p0K1F1ZkM3cXUwNmtSaGRlR2RjeitVbkFsdVJvQmlP?=
 =?utf-8?B?WVVUN3BSVkhsTmRCWlB0QzEzamtSSzBoQVpiMEhTeWxNbjB2UVRCTEZTVy9v?=
 =?utf-8?B?K1l2d1UzKzdJZDVBOGorQUNFR1lTZFdreVZMeEZNUTFzbmxFdGUwMFF5QUwz?=
 =?utf-8?B?bzJVczlwb3VhdEpKeDJIRkZUUXN6YzR1OTA5Tk1rMlVYL1ZRYzZlV1Exb3U5?=
 =?utf-8?B?TTRxNGVQL2tLaktvczFGb1RzUzVHeUNTZS9HSlJWay9rVjZiemxRMEFQVnFU?=
 =?utf-8?B?b2t2Wit4YnFHcGRYVDVwYlZQekc4VjdmVFRXK2NZbTB6WFVmY2pjeUUwQ0Fj?=
 =?utf-8?B?aE5pNDZkbHBSUTY4dmovZ01rOFpNVVNpVzEycFBxSFJnL1BGc0tqdFZ2S3VR?=
 =?utf-8?B?TXZIc3hvanFpWjhxMjFPWmNiVDl5MGRwTUR0THJqZEwrbUlxemsrUHRJdlJH?=
 =?utf-8?B?cGJ5RDNLYS9ZRU9rNnIzWk5UNmZtUmM2bjM5QzY1TldDNS9mUG5naHJlcExX?=
 =?utf-8?B?VTlHdmxla29YTGFuZkRmUndvbWtPelMyUUtjK3lwNzRJZS93R1U5R0NGME9C?=
 =?utf-8?B?NzlhbEJGcWhWUUp3cGlmUXF2bWE0anNLZ3dIZ21MbUVTOGJxb0lnT1k5czhJ?=
 =?utf-8?Q?dWhuG2v+Xih3vJnCKgR++cNvM?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 80c3dcc8-5aae-488a-4cf1-08dc1cb76e28
X-MS-Exchange-CrossTenant-AuthSource: MW2PR12MB2379.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jan 2024 08:35:33.5623
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WowTbyBXj07wNFHm2l3iEob+69NujFewkJHSMqtSi9B3mnYM1/fZCekzOxSV6H0uihHpa2EhOQLcw4jUpVC++w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR12MB9263

Hello David,

Thank you for taking a look at the patch.

On 1/24/2024 2:47 AM, David Vernet wrote:
> On Tue, Jan 23, 2024 at 10:28:31AM +0530, K Prateek Nayak wrote:
>> Hello Tim,
>>
>> On 1/23/2024 3:29 AM, Tim Chen wrote:
>>> On Fri, 2024-01-19 at 14:15 +0530, K Prateek Nayak wrote:
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index b803030c3a03..1fedc7e29c98 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -8499,6 +8499,16 @@ done: __maybe_unused;
>>>>  	if (!rf)
>>>>  		return NULL;
>>>>  
>>>> +	/*
>>>> +	 * An idle CPU in TIF_POLLING mode might end up here after processing
>>>> +	 * an IPI when the sender sets the TIF_NEED_RESCHED bit and avoids
>>>> +	 * sending an actual IPI. In such cases, where an idle CPU was woken
>>>> +	 * up only to process an interrupt, without necessarily queuing a task
>>>> +	 * on it, skip newidle_balance() to facilitate faster idle re-entry.
>>>> +	 */
>>>> +	if (prev == rq->idle)
>>>> +		return NULL;
>>>> +
>>>
>>> Should we check the call function queue directly to detect that there is
>>> an IPI waiting to be processed? something like
>>>
>>> 	if (!llist_empty(&per_cpu(call_single_queue, rq->cpu)))
>>> 		return NULL;
>>
>> That could be a valid check too. However, if an IPI is queued right
>> after this check, the processing is still delayed since
>> newidle_balance() only bails out for scenarios when a wakeup is trying
>> to queue a new task on the CPU running the newidle_balance().
>>
>>>
>>> Could there be cases where we want to do idle balance in this code path?
>>> Say a cpu is idle and a scheduling tick came in, we may try
>>> to look for something to run on the idle cpu.  Seems like after
>>> your change above, that would be skipped.
>>
>> Wouldn't scheduler_tick() do load balancing when the time comes? In my
>> testing, I did not see a case where the workloads I tested were
>> sensitive to the aspect of newidle_balance() being invoked at scheduler
>> tick. Have you come across a workload which might be sensitive to this
>> aspect that I can quickly test and verify? Meanwhile, I'll run the
>> workloads mentioned in the commit log on an Intel system to see if I
>> can spot any sensitivity to this change.
>>
>> Adding David to the thread too since HHVM seems to be one of those
>> workloads that is very sensitive to a successful newidle_balance().
> 
> Thanks for the cc. FWIW, I think a lot of things are very sensitive to
> timing in newidle_balance(), but it goes both ways. For example, we had
> to revert commit e60b56e46b38 ("sched/fair: Wait before decaying
> max_newidle_lb_cost") [0] on our internal kernel because it regressed
> some workloads by causing us to load_balance() too frequently. I think
> the fix is correct in that there's no reason we shouldn't apply the ~1%
> decay / second to newidle lb cost in newidle_balance(), but by causing
> us to (correctly) decay newidle lb cost in newidle_balance(), it also
> increased CPU util rather significantly and had us spending too much
> time in load_balance().
> 
> [0]: https://lore.kernel.org/all/20211019123537.17146-4-vincent.guittot@linaro.org/
> 
> On the other hand, on other hosts, we use SHARED_RUNQ to load balance as
> aggressively as possible, and those hosts would have benefited from that
> change if SHARED_RUNQ wasn't an option.
> 
> My 2 cents is that I think it's impossible to make everyone happy, and I
> think the change here makes sense. If there's imbalance, it's something
> we would uncover when load_balance() is kicked off on the tick path
> anyways. I also agree with Vincent [1] that your idea / prototype of
> adding a TIF_NEED_IPI flag is an overall better solution, but this does
> seem fine to me as well in the interim.

Thank you for the detailing your explorations and giving more insights
around newidle_balance(). I'll clean up and test the TIF_NEED_IPI
prototype some more before sending out an RFC.

> 
> [1]: https://lore.kernel.org/all/CAKfTPtC446Lo9CATPp7PExdkLhHQFoBuY-JMGC7agOHY4hs-Pw@mail.gmail.com/
> 
> Thanks,
> David

--
Thanks and Regards,
Prateek