Hi Peter,
On Fri, Jul 09, 2021 at 02:57:10PM +0200 Peter Zijlstra wrote:
On Thu, Jul 08, 2021 at 10:54:58AM -0400, Phil Auld wrote:
Sorry... I don't have a nice diagram. I'm still looking at what all those macros actually mean on the various architectures.
Don't worry about other architectures, lets focus on Power, because that's the case where you can reprouce funnies. Now Power only has 2 barrier ops (not quite true, but close enough for all this):
SYNC is the full barrier
LWSYNC is a TSO like barrier
Pretty much everything (LOAD-ACQUIRE, STORE-RELEASE, WMB, RMB) uses LWSYNC. Only MB result in SYNC.
Power is 'funny' because their spinlocks are weaker than everybody else's, but AFAICT that doesn't seem relevant here.
Thanks.
Using what you have above I get the same thing. It looks like it should be ordered but in practice it's not, and ordering it "more" as I did in the patch, fixes it.
And you're running Linus' tree, not some franken-kernel from RHT, right? As asked in that other email, can you try with just the WMB added? I really don't believe that RMB you added can make a difference.
So, no. Right now the reproducer is on the franken-kernel :(
As far as I can tell the relevant code paths (schedule, barriers, wakeup etc) are all current and the same. I traced through your diagram and it all matches exactly.
I have a suspicion that Linus's tree may hide it. I believe this is tickled by NFS io, which I _think_ is effected by the unboud workqueue changes that may make it less likely to do the wakeup on a different cpu. But that's just speculation.
The issue is that the systems under test here are in a partner's lab to which I have no direct access.
I will try to get an upstream build on there, if possible, as soon as I can.
Also, can you try with TTWU_QUEUE disabled (without any additional barriers added), that simplifies the wakeup path a lot.
Will do.
Is it possible that the bit field is causing some of the assumptions about ordering in those various macros to be off?
*should* not matter...
prev->sched_contributes_to_load = X;
smp_store_release(&prev->on_cpu, 0); asm("LWSYNC" : : : "memory"); WRITE_ONCE(prev->on_cpu, 0);
due to that memory clobber, the compiler must emit whatever stores are required for the bitfield prior to the LWSYNC.
I notice in all the comments about smp_mb__after_spinlock etc, it's always WRITE_ONCE/READ_ONCE on the variables in question but we can't do that with the bit field.
Yeah, but both ->on_rq and ->sched_contributes_to_load are 'normal' stores. That said, given that ttwu() does a READ_ONCE() on ->on_rq, we should match that with WRITE_ONCE()...
So I think we should do the below, but I don't believe it'll make a difference. Let me stare more.
I'm out of the office for the next week+ so don't stare to hard. I'll try to get the tests you asked for as soon as I get back in the (home) office.
I'm not sure the below will make a difference either, but will try it too.
Thanks again for the help. And sorry for the timing.
Cheers, Phil
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index ca9a523c9a6c..da93551b298d 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1973,12 +1973,12 @@ void activate_task(struct rq *rq, struct task_struct *p, int flags) { enqueue_task(rq, p, flags);
- p->on_rq = TASK_ON_RQ_QUEUED;
- WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);
} void deactivate_task(struct rq *rq, struct task_struct *p, int flags) {
- p->on_rq = (flags & DEQUEUE_SLEEP) ? 0 : TASK_ON_RQ_MIGRATING;
- WRITE_ONCE(p->on_rq, (flags & DEQUEUE_SLEEP) ? 0 : TASK_ON_RQ_MIGRATING);
dequeue_task(rq, p, flags); } @@ -5662,11 +5662,11 @@ static bool try_steal_cookie(int this, int that) if (p->core_occupation > dst->idle->core_occupation) goto next;
p->on_rq = TASK_ON_RQ_MIGRATING;
WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
p->on_rq = TASK_ON_RQ_QUEUED;
WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);resched_curr(dst);