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

[Jon Kohler]

> On Nov 14, 2024, at 1:57 PM, Phil Auld <pauld@xxxxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>> 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?

> 
> Reviewed-by: Phil Auld <pauld@xxxxxxxxxx>
> 
> 
> Cheers,
> Phil
> 
> -- 
>