Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Paul E. McKenney]

Hello, Frederic,

This commit works well, but has the unfortunate side-effect of making
smp_processor_id() complain when used in a preemptible region even
though the kthread has been pinned onto a single CPU by a call to
set_cpus_allowed_ptr().  (Which did return success.)

This isn't a big deal -- I can easily switch to raw_smp_processor_id(),
which is arguably a better choice anyway because it prevents the
complaints from flooding out any real warnings due to error returns
from set_cpus_allowed_ptr() or something else unpinning the kthread.
Which I am in the process of doing:

516e52e9f5ec ("scftorture: Avoid excess warnings")
475d6d49f21d ("refscale: Avoid excess warnings in ref_scale_reader()")

But I figured that I should check to see if this change was in fact
intentional.

							Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Valentin Schneider]

On 10/06/21 10:04, Paul E. McKenney wrote:

Hi,
> Hello, Frederic,
>
> This commit works well, but has the unfortunate side-effect of making
> smp_processor_id() complain when used in a preemptible region even
> though the kthread has been pinned onto a single CPU by a call to
> set_cpus_allowed_ptr().  (Which did return success.)
>

On which tree are you encountering this?

Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:

        /*
         * Kernel threads bound to a single CPU can safely use
         * smp_processor_id():
         */
        if (current->nr_cpus_allowed == 1)
                goto out;

tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:

  570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")

The former shouldn't be affected by Frederic's patch, and the latter should
only cause warnings if the pinned task isn't a "proper" kthread (thus
doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
like UMH which doesn't use kthread_create().

> This isn't a big deal -- I can easily switch to raw_smp_processor_id(),
> which is arguably a better choice anyway because it prevents the
> complaints from flooding out any real warnings due to error returns
> from set_cpus_allowed_ptr() or something else unpinning the kthread.
> Which I am in the process of doing:
>
> 516e52e9f5ec ("scftorture: Avoid excess warnings")
> 475d6d49f21d ("refscale: Avoid excess warnings in ref_scale_reader()")
>
> But I figured that I should check to see if this change was in fact
> intentional.
>
>                                                       Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Paul E. McKenney]

On Thu, Jun 10, 2021 at 07:28:57PM +0100, Valentin Schneider wrote:
> On 10/06/21 10:04, Paul E. McKenney wrote:
> 
> Hi,
> > Hello, Frederic,
> >
> > This commit works well, but has the unfortunate side-effect of making
> > smp_processor_id() complain when used in a preemptible region even
> > though the kthread has been pinned onto a single CPU by a call to
> > set_cpus_allowed_ptr().  (Which did return success.)
> >
> 
> On which tree are you encountering this?

I bisected to this commit in -next tag next-20210609, and this commit
could of course be an innocent bystander caught in the crossfire.

> Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:
> 
>         /*
>          * Kernel threads bound to a single CPU can safely use
>          * smp_processor_id():
>          */
>         if (current->nr_cpus_allowed == 1)
>                 goto out;
> 
> tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:
> 
>   570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")
> 
> The former shouldn't be affected by Frederic's patch, and the latter should
> only cause warnings if the pinned task isn't a "proper" kthread (thus
> doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
> like UMH which doesn't use kthread_create().

And reverting 570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread()
instead of nr_cpus_allowed") causes the kernel to once again be OK with
smp_processor_id(), so thank you!  And apologies to Frederic for the
false alarm.

Added Yejune on CC.  Thoughts?

							Thanx, Paul

> > This isn't a big deal -- I can easily switch to raw_smp_processor_id(),
> > which is arguably a better choice anyway because it prevents the
> > complaints from flooding out any real warnings due to error returns
> > from set_cpus_allowed_ptr() or something else unpinning the kthread.
> > Which I am in the process of doing:
> >
> > 516e52e9f5ec ("scftorture: Avoid excess warnings")
> > 475d6d49f21d ("refscale: Avoid excess warnings in ref_scale_reader()")
> >
> > But I figured that I should check to see if this change was in fact
> > intentional.
> >
> >                                                       Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Valentin Schneider]

On 10/06/21 13:17, Paul E. McKenney wrote:
> On Thu, Jun 10, 2021 at 07:28:57PM +0100, Valentin Schneider wrote:
>> On 10/06/21 10:04, Paul E. McKenney wrote:
>>
>> Hi,
>> > Hello, Frederic,
>> >
>> > This commit works well, but has the unfortunate side-effect of making
>> > smp_processor_id() complain when used in a preemptible region even
>> > though the kthread has been pinned onto a single CPU by a call to
>> > set_cpus_allowed_ptr().  (Which did return success.)
>> >
>>
>> On which tree are you encountering this?
>
> I bisected to this commit in -next tag next-20210609, and this commit
> could of course be an innocent bystander caught in the crossfire.
>
>> Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:
>>
>>         /*
>>          * Kernel threads bound to a single CPU can safely use
>>          * smp_processor_id():
>>          */
>>         if (current->nr_cpus_allowed == 1)
>>                 goto out;
>>
>> tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:
>>
>>   570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")
>>
>> The former shouldn't be affected by Frederic's patch, and the latter should
>> only cause warnings if the pinned task isn't a "proper" kthread (thus
>> doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
>> like UMH which doesn't use kthread_create().
>
> And reverting 570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread()
> instead of nr_cpus_allowed") causes the kernel to once again be OK with
> smp_processor_id(), so thank you!  And apologies to Frederic for the
> false alarm.
>
> Added Yejune on CC.  Thoughts?
>

The way I see 570a752b7a9b is that, if a task is pinned to a single CPU but
doesn't have PF_NO_SETAFFINITY, then userspace can unpin it. This means it
ought to have entered check_preemption_disabled() with preemption disabled
- right now it may be pinned, but that can change at any minute, and
whatever code it is running needs to cope with that.

Could you share some details on which tasks you are hitting this with?

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Frederic Weisbecker]

On Thu, Jun 10, 2021 at 01:17:13PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 10, 2021 at 07:28:57PM +0100, Valentin Schneider wrote:
> > On 10/06/21 10:04, Paul E. McKenney wrote:
> > 
> > Hi,
> > > Hello, Frederic,
> > >
> > > This commit works well, but has the unfortunate side-effect of making
> > > smp_processor_id() complain when used in a preemptible region even
> > > though the kthread has been pinned onto a single CPU by a call to
> > > set_cpus_allowed_ptr().  (Which did return success.)
> > >
> > 
> > On which tree are you encountering this?
> 
> I bisected to this commit in -next tag next-20210609, and this commit
> could of course be an innocent bystander caught in the crossfire.
> 
> > Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:
> > 
> >         /*
> >          * Kernel threads bound to a single CPU can safely use
> >          * smp_processor_id():
> >          */
> >         if (current->nr_cpus_allowed == 1)
> >                 goto out;
> > 
> > tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:
> > 
> >   570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")
> > 
> > The former shouldn't be affected by Frederic's patch, and the latter should
> > only cause warnings if the pinned task isn't a "proper" kthread (thus
> > doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
> > like UMH which doesn't use kthread_create().
> 
> And reverting 570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread()
> instead of nr_cpus_allowed") causes the kernel to once again be OK with
> smp_processor_id(), so thank you!  And apologies to Frederic for the
> false alarm.
> 
> Added Yejune on CC.  Thoughts?

There is also that:

      15faafc6b449777a85c0cf82dd8286c293fed4eb ("sched,init: Fix DEBUG_PREEMPT
      vs early boot")

Not sure if that will help but just in case.

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Paul E. McKenney]

On Fri, Jun 11, 2021 at 11:12:29AM +0100, Valentin Schneider wrote:
> On 10/06/21 13:17, Paul E. McKenney wrote:
> > On Thu, Jun 10, 2021 at 07:28:57PM +0100, Valentin Schneider wrote:
> >> On 10/06/21 10:04, Paul E. McKenney wrote:
> >>
> >> Hi,
> >> > Hello, Frederic,
> >> >
> >> > This commit works well, but has the unfortunate side-effect of making
> >> > smp_processor_id() complain when used in a preemptible region even
> >> > though the kthread has been pinned onto a single CPU by a call to
> >> > set_cpus_allowed_ptr().  (Which did return success.)
> >> >
> >>
> >> On which tree are you encountering this?
> >
> > I bisected to this commit in -next tag next-20210609, and this commit
> > could of course be an innocent bystander caught in the crossfire.
> >
> >> Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:
> >>
> >>         /*
> >>          * Kernel threads bound to a single CPU can safely use
> >>          * smp_processor_id():
> >>          */
> >>         if (current->nr_cpus_allowed == 1)
> >>                 goto out;
> >>
> >> tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:
> >>
> >>   570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")
> >>
> >> The former shouldn't be affected by Frederic's patch, and the latter should
> >> only cause warnings if the pinned task isn't a "proper" kthread (thus
> >> doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
> >> like UMH which doesn't use kthread_create().
> >
> > And reverting 570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread()
> > instead of nr_cpus_allowed") causes the kernel to once again be OK with
> > smp_processor_id(), so thank you!  And apologies to Frederic for the
> > false alarm.
> >
> > Added Yejune on CC.  Thoughts?
> >
> 
> The way I see 570a752b7a9b is that, if a task is pinned to a single CPU but
> doesn't have PF_NO_SETAFFINITY, then userspace can unpin it. This means it
> ought to have entered check_preemption_disabled() with preemption disabled
> - right now it may be pinned, but that can change at any minute, and
> whatever code it is running needs to cope with that.

Thank you for catching me up on this topic!

> Could you share some details on which tasks you are hitting this with?

Let's start with ref_scale_reader() in kernel/rcu/refscale.c.  This
is for fine-grained in-kernel benchmarking, so it really wants kthreads
running this function to be pinned.

I took a look at kthread_bind(), but it is not intended to be called by
the kthread itself.  Looking elsewhere in the kernel, it looks like I
just do this right after invoking set_cpus_allowed_ptr():

	current->flags != PF_NO_SETAFFINITY;

Or am I missing a better way to handle this?

							Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Valentin Schneider]

On 11/06/21 06:52, Paul E. McKenney wrote:
> On Fri, Jun 11, 2021 at 11:12:29AM +0100, Valentin Schneider wrote:
>>
>> The way I see 570a752b7a9b is that, if a task is pinned to a single CPU but
>> doesn't have PF_NO_SETAFFINITY, then userspace can unpin it. This means it
>> ought to have entered check_preemption_disabled() with preemption disabled
>> - right now it may be pinned, but that can change at any minute, and
>> whatever code it is running needs to cope with that.
>
> Thank you for catching me up on this topic!
>
>> Could you share some details on which tasks you are hitting this with?
>
> Let's start with ref_scale_reader() in kernel/rcu/refscale.c.  This
> is for fine-grained in-kernel benchmarking, so it really wants kthreads
> running this function to be pinned.
>
> I took a look at kthread_bind(), but it is not intended to be called by
> the kthread itself.  Looking elsewhere in the kernel, it looks like I
> just do this right after invoking set_cpus_allowed_ptr():
>
>       current->flags != PF_NO_SETAFFINITY;
>
> Or am I missing a better way to handle this?

Looking at ref_scale_reader(), ISTM the initial configuration (affinity,
niceness) should be done by its parent thread, not by itself. i.e.:

  p = kthread_create(ref_scale_reader);
  kthread_bind(p, cpu); // Does the pinning + sets PF_NO_SETAFFINITY
  set_user_nice(p, MAX_NICE);
  wake_up_process(p);

(kthread_create_on_cpu() is also an option)

From what I can see, torture_create_kthread() immediately wakes the
newly-created kthread, we'd need a version that calls kthread_create()
instead of kthread_run() for the above. Would that be an issue?

>
>                                                       Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Paul E. McKenney]

On Fri, Jun 11, 2021 at 03:19:29PM +0100, Valentin Schneider wrote:
> On 11/06/21 06:52, Paul E. McKenney wrote:
> > On Fri, Jun 11, 2021 at 11:12:29AM +0100, Valentin Schneider wrote:
> >>
> >> The way I see 570a752b7a9b is that, if a task is pinned to a single CPU but
> >> doesn't have PF_NO_SETAFFINITY, then userspace can unpin it. This means it
> >> ought to have entered check_preemption_disabled() with preemption disabled
> >> - right now it may be pinned, but that can change at any minute, and
> >> whatever code it is running needs to cope with that.
> >
> > Thank you for catching me up on this topic!
> >
> >> Could you share some details on which tasks you are hitting this with?
> >
> > Let's start with ref_scale_reader() in kernel/rcu/refscale.c.  This
> > is for fine-grained in-kernel benchmarking, so it really wants kthreads
> > running this function to be pinned.
> >
> > I took a look at kthread_bind(), but it is not intended to be called by
> > the kthread itself.  Looking elsewhere in the kernel, it looks like I
> > just do this right after invoking set_cpus_allowed_ptr():
> >
> >       current->flags != PF_NO_SETAFFINITY;
> >
> > Or am I missing a better way to handle this?
> 
> Looking at ref_scale_reader(), ISTM the initial configuration (affinity,
> niceness) should be done by its parent thread, not by itself. i.e.:
> 
>   p = kthread_create(ref_scale_reader);
>   kthread_bind(p, cpu); // Does the pinning + sets PF_NO_SETAFFINITY
>   set_user_nice(p, MAX_NICE);
>   wake_up_process(p);
> 
> (kthread_create_on_cpu() is also an option)
> 
> >From what I can see, torture_create_kthread() immediately wakes the
> newly-created kthread, we'd need a version that calls kthread_create()
> instead of kthread_run() for the above. Would that be an issue?

It sounds much simpler for me to just continue using raw_smp_processor_id()
instead of trying to switch back to smp_processor_id().  ;-)

							Thanx, Paul

Re: Question about a8ea6fc9b089 ("sched: Stop PF_NO_SETAFFINITY from being inherited by various init system threads")

[Paul E. McKenney]

On Fri, Jun 11, 2021 at 02:42:12PM +0200, Frederic Weisbecker wrote:
> On Thu, Jun 10, 2021 at 01:17:13PM -0700, Paul E. McKenney wrote:
> > On Thu, Jun 10, 2021 at 07:28:57PM +0100, Valentin Schneider wrote:
> > > On 10/06/21 10:04, Paul E. McKenney wrote:
> > > 
> > > Hi,
> > > > Hello, Frederic,
> > > >
> > > > This commit works well, but has the unfortunate side-effect of making
> > > > smp_processor_id() complain when used in a preemptible region even
> > > > though the kthread has been pinned onto a single CPU by a call to
> > > > set_cpus_allowed_ptr().  (Which did return success.)
> > > >
> > > 
> > > On which tree are you encountering this?
> > 
> > I bisected to this commit in -next tag next-20210609, and this commit
> > could of course be an innocent bystander caught in the crossfire.
> > 
> > > Looking at check_preemption_disabled() and CPU affinity, v5.13-rc5 has:
> > > 
> > >         /*
> > >          * Kernel threads bound to a single CPU can safely use
> > >          * smp_processor_id():
> > >          */
> > >         if (current->nr_cpus_allowed == 1)
> > >                 goto out;
> > > 
> > > tip/sched/core additionally hinges that on PF_NO_SETAFFINITY:
> > > 
> > >   570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread() instead of nr_cpus_allowed")
> > > 
> > > The former shouldn't be affected by Frederic's patch, and the latter should
> > > only cause warnings if the pinned task isn't a "proper" kthread (thus
> > > doesn't have PF_NO_SETAFFINITY)... Exceptions that come to mind are things
> > > like UMH which doesn't use kthread_create().
> > 
> > And reverting 570a752b7a9b ("lib/smp_processor_id: Use is_percpu_thread()
> > instead of nr_cpus_allowed") causes the kernel to once again be OK with
> > smp_processor_id(), so thank you!  And apologies to Frederic for the
> > false alarm.
> > 
> > Added Yejune on CC.  Thoughts?
> 
> There is also that:
> 
>       15faafc6b449777a85c0cf82dd8286c293fed4eb ("sched,init: Fix DEBUG_PREEMPT
>       vs early boot")
> 
> Not sure if that will help but just in case.

Thank you for the pointer!  These tasks start later well after
kthreadd_done, so I believe that I dodged this particular bullet.  ;-)

						Thanx, Paul