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

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

[Jon Kohler]

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

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

[Phil Auld]

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.


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


Cheers,
Phil

-- 

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@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?

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

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

[Phil Auld]

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

-- 

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

[Peter Zijlstra]

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

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

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.

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.

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

[Phil Auld]

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

-- 

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

[Peter Zijlstra]

On Fri, Nov 15, 2024 at 09:26:58AM -0500, Phil Auld wrote:
> 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 :)

Hehe, typing is forever hard :-)