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
