Re: [PATCH v12 08/13] sched/core: Push current task from non preferred CPU

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 9/9/26 4:28 AM, Yury Norov wrote:
> On Mon, Sep 07, 2026 at 08:53:19AM +0530, Shrikanth Hegde wrote:
>> Hi Yury, thanks for taking a look.
>>

>>>> +	/* This could take rq lock. So call it before rq lock is taken */
>>>> +	cpu = select_fallback_rq(rq->cpu, p);
>>>> +	rq_lock(rq, &rf);
>>>
>>> If select_fallback_rq() grabs the lock, then when it releases the
>>> lock, there's a window for race between the other process and the
>>> subsequent rq_lock(). Or I misunderstand it?
>>>
>>
>> select_fallback_rq taking lock is for any state change that needs to happen
>> such as fallback to possible CPUs etc.
>>
>> Most of the time it won't grab the rq lock. Even if the task got pulled by load balancer
>> before grabbing the lock, Below (task_rq(p) == rq) will catch that, and it bails out.
>>
>> So it is safe.
> 
> OK... Can you please explain it in the comment above?
> 
> ...

Ok.

> 
>>>> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>>>> index 6c3ad70e58b8..678e44134acf 100644
>>>> --- a/kernel/sched/sched.h
>>>> +++ b/kernel/sched/sched.h
>>>> @@ -1298,6 +1298,8 @@ struct rq {
>>>>    	struct list_head cfs_tasks;
>>>> +	bool			push_task_work_done;
>>>> +
>>>
>>> It should be protected with CONFIG_PREFERRED_CPU. Also, the name
>>> doesn't look correct. You set the variable to 'true' even before
>>> calling the stopper. Maybe need_push_to_npc, or similar?
>>>
>>
>> ok. npc_push_work_pending is probably a better one?
>>> Why did you place it between cfs_tasks and avg_rt? If no specific
>>> reason, maybe place it next to CONFIG_PARAVIRT-guarded fields.
>>>
>>
>> I don't see a common empty space there. I could increase the size.
>>
>>> What about pahole?
>>
>> I did check pahole on powerpc which has 128 byte cachelines.
>>
>> 	int                        online;               /*  4524     4 */
>> 	struct list_head           cfs_tasks;            /*  4528    16 */
>>
>> 	/* XXX 64 bytes hole, try to pack */
>>
>> It was empty space. Now, that i check 64 byte cachelines it may not be the
>> optimal one.
>>
>> I do see, a couple common places for both 64 abd 126 byte cacheline. I believe those
>> are better places. It won't increase the size or cause any existing fields to
>> misalign. It also makes sense to guard it again CONFIG_PREFERRED_CPU. I had not
>> done to avoid ifdefs. But it is used only under it. So i think that makes sense too.
>>
>> 1.
>>
>> 	struct balance_callback *  balance_callback;     /*  3608     8 */
>> 	unsigned char              nohz_idle_balance;    /*  3616     1 */
>> 	unsigned char              idle_balance;         /*  3617     1 */
>>
>> 	/* XXX 6 bytes hole, try to pack */
>> 	long unsigned int          misfit_task_load;     /*  3624     8 */
>>
>>
>> 2.
>> 	unsigned int               ttwu_count;           /*  5276     4 */
>> 	unsigned int               ttwu_local;           /*  5280     4 */
>>
>> 	/* XXX 4 bytes hole, try to pack */
>>
>> 	struct cpuidle_state *     idle_state;           /*  5288     8 */
> 
> The struct rq is highly configurable. Depending on your config,
> the holes will migrate to different places. I'd not rely on just
> 'optimizing holes' problem. Just put the new field next to logically
> related existing fields.
> 
> You've got paravirt-related prev_steal_time and prev_steal_time_rq,
> and you've got the /* For active balancing */ section. Maybe one of
> them?
> 

Ok. Moving it after prev_steal_time_rq.

  #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
         u64                     prev_steal_time_rq;
  #endif
+#ifdef CONFIG_PREFERRED_CPU
+       bool                    npc_push_work_pending;
+#endif


I checked on 128 byte cachelines, it didn't increase the number of cachelines
with the change as well. So we are good there.

base:
         /* size: 5632, cachelines: 44, members: 104 */
         /* sum members: 5022, holes: 15, sum holes: 502 */
with above change:
         /* size: 5632, cachelines: 44, members: 105 */
         /* sum members: 5023, holes: 17, sum holes: 533 */


On 64 byte cachelines too, there is 16 bytes hole a bit below. So it should absorb it
as well. So we are fine there as well.

	u64                        prev_steal_time_rq;   /*  3960     8 */
	/* --- cacheline 62 boundary (3968 bytes) --- */
	long unsigned int          calc_load_update;     /*  3968     8 */
	long int                   calc_load_active;     /*  3976     8 */

	/* XXX 16 bytes hole, try to pack */

	call_single_data_t         hrtick_csd __attribute__((__aligned__(32))); /*  4000    32 */

I will make this change and send out v13 today.
> Thanks,
> Yury