Re: [PATCH] sched: hoist ASSERT_EXCLUSIVE_WRITER(p->on_rq) above WRITE_ONCE

[Phil Auld <pauld@redhat.com>]

On Fri, Nov 15, 2024 at 10:58:47AM +0100 Peter Zijlstra wrote:
> On Thu, Nov 14, 2024 at 02:20:56PM -0500, Phil Auld wrote:
> 
> > I don't know. I don't think it matters much since the assert is really
> > independent of the actual write. Like I said it makes sense to have it
> > first to me but others may see it as just moving code around for no strong
> > reason.  Peter may or may not decide to pick this one up. Other "mis-ordered"
> > uses are in code maintained by different folks.
> > 
> > You can see if anyone else weighs in...
> 
> So I'm not entirely about this patch... :-)
>

I'm not entirely either :)


> Per commit b55945c500c5 ("sched: Fix pick_next_task_fair() vs try_to_wake_up() race")
> we can see that this placement is not equivalent.
>

Subtle. That kind of thing (someone else acting on the write) was my
hesitation but I couldn't quite wrap my head around it.

That's why we have other people weigh in ...

Thanks!

> Placing it before the store means that nobody else will store to it
> until you've done your store.
> 
> Placing it after means that nobody else will attempt the store,
> irrespective of the store you just did.

Presumably until some barrier or release at least, right?

I've got to dig into the kcsan stuff a bit more. It looks really
interesting.


Cheers,
Phil

> 
> That is, the ASSERT after the store is a stronger assert.
> 
> In case of activate_task(), we mark the task as on_rq, and this state is
> protected by rq->lock (which we hold), so there must not be any stores
> for as long as we hold that lock. So after is the right place.
> 
> In case of deactivate_task(), this is a hand-off, but the handoff
> doesn't happen until after dequeue_task() and set_task_cpu(). So at this
> point, again, nobody should be modifying it.
> 
> 

--