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

[Phil Auld <pauld@redhat.com>]

On Thu, Nov 14, 2024 at 07:01:13PM +0000 Jon Kohler wrote:
> 
> 
> > On Nov 14, 2024, at 1:57 PM, Phil Auld <pauld@redhat.com> wrote:
> > 
> > !-------------------------------------------------------------------|
> >  CAUTION: External Email
> > 
> > |-------------------------------------------------------------------!
> > 
> > On Thu, Nov 14, 2024 at 09:53:52AM -0700 Jon Kohler wrote:
> >> In {activate|deactivate}_task(), hoist ASSERT_EXCLUSIVE_WRITER() to be
> >> above WRITE_ONCE(p->on_rq), which matches the ordering listed in the
> >> KCSAN documentation, kcsan-checks.h code comments, and the usage
> >> pattern we already have in __block_task().
> >> 
> >> Signed-off-by: Jon Kohler <jon@nutanix.com>
> >> ---
> >> kernel/sched/core.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index a1c353a62c56..80a04c36b495 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2066,16 +2066,16 @@ void activate_task(struct rq *rq, struct task_struct *p, int flags)
> >> 
> >> enqueue_task(rq, p, flags);
> >> 
> >> - WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);
> >> ASSERT_EXCLUSIVE_WRITER(p->on_rq);
> >> + WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);
> >> }
> >> 
> >> void deactivate_task(struct rq *rq, struct task_struct *p, int flags)
> >> {
> >> SCHED_WARN_ON(flags & DEQUEUE_SLEEP);
> >> 
> >> - WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
> >> ASSERT_EXCLUSIVE_WRITER(p->on_rq);
> >> + WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
> >> 
> >> /*
> >> * Code explicitly relies on TASK_ON_RQ_MIGRATING begin set *before*
> >> -- 
> >> 2.43.0
> >> 
> >> 
> > 
> > This looks fine to me and it makes sense to have the assert before the
> > write.  A quick grep showed that this is by no means a universal pattern
> > at the moment.
> > 
> 
> I’d have to imaging having the assert before must be the right way to
> do this, just from a logic control flow perspective. I’m happy to fix ’the
> others', or do you think I should let them sit there?
>

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...

Cheers,
Phil

> > 
> > Reviewed-by: Phil Auld <pauld@redhat.com>
> > 
> > 
> > Cheers,
> > Phil
> > 
> > -- 
> > 
> 

--