[29/30] Documentation: tracing: add TIF_NEED_RESCHED_LAZY

  Document various combinations of resched flags.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Originally-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 Documentation/trace/ftrace.rst | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
  

On Mon, 12 Feb 2024 21:55:53 -0800
Ankur Arora <ankur.a.arora@oracle.com> wrote:

> Document various combinations of resched flags.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Originally-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>  Documentation/trace/ftrace.rst | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> index 7e7b8ec17934..7f20c0bae009 100644
> --- a/Documentation/trace/ftrace.rst
> +++ b/Documentation/trace/ftrace.rst
> @@ -1036,8 +1036,12 @@ explains which is which.
>  		be printed here.
>  
>    need-resched:
> -	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED is set,
> +	- 'B' all three, TIF_NEED_RESCHED, TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
> +	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED are set,
> +	- 'L' both TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
> +	- 'b' both TIF_NEED_RESCHED and TIF_NEED_RESCHED_LAZY are set,
>  	- 'n' only TIF_NEED_RESCHED is set,
> +	- 'l' only TIF_NEED_RESCHED_LAZY is set,
>  	- 'p' only PREEMPT_NEED_RESCHED is set,
>  	- '.' otherwise.
>  

I wonder if we should also add this information in /sys/kernel/tracing/README
so that it is easier to find on a machine.

-- Steve
  

Steven Rostedt <rostedt@goodmis.org> writes:

> On Mon, 12 Feb 2024 21:55:53 -0800
> Ankur Arora <ankur.a.arora@oracle.com> wrote:
>
>> Document various combinations of resched flags.
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Originally-by: Thomas Gleixner <tglx@linutronix.de>
>> Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>>  Documentation/trace/ftrace.rst | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
>> index 7e7b8ec17934..7f20c0bae009 100644
>> --- a/Documentation/trace/ftrace.rst
>> +++ b/Documentation/trace/ftrace.rst
>> @@ -1036,8 +1036,12 @@ explains which is which.
>>  		be printed here.
>>
>>    need-resched:
>> -	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED is set,
>> +	- 'B' all three, TIF_NEED_RESCHED, TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>> +	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED are set,
>> +	- 'L' both TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>> +	- 'b' both TIF_NEED_RESCHED and TIF_NEED_RESCHED_LAZY are set,
>>  	- 'n' only TIF_NEED_RESCHED is set,
>> +	- 'l' only TIF_NEED_RESCHED_LAZY is set,
>>  	- 'p' only PREEMPT_NEED_RESCHED is set,
>>  	- '.' otherwise.
>>
>
> I wonder if we should also add this information in /sys/kernel/tracing/README
> so that it is easier to find on a machine.

Yeah, there is a problem with the discovery. Seems a little out of place
in tracing/README though.

How about something like this? Though this isn't really a model of clarity.

    --- a/kernel/trace/trace.c
    +++ b/kernel/trace/trace.c
    @@ -4292,7 +4292,7 @@ static void print_lat_help_header(struct seq_file *m)
    {
            seq_puts(m, "#                    _------=> CPU#            \n"
                    "#                   / _-----=> irqs-off/BH-disabled\n"
    -                   "#                  | / _----=> need-resched    \n"
    +                   "#                  | / _----=> need-resched  [ l: lazy, n: now, p: preempt, b: l|n, L: l|p, N: n|p, B: l|n|p ]\n"
                    "#                  || / _---=> hardirq/softirq \n"
                    "#                  ||| / _--=> preempt-depth   \n"
                    "#                  |||| / _-=> migrate-disable \n"


Also, haven't looked at trace-cmd. Anything I should be sending a patch
out for?

Thanks

--
ankur
  

On Wed, 21 Feb 2024 15:22:20 -0800
Ankur Arora <ankur.a.arora@oracle.com> wrote:


> > I wonder if we should also add this information in /sys/kernel/tracing/README
> > so that it is easier to find on a machine.  
> 
> Yeah, there is a problem with the discovery. Seems a little out of place
> in tracing/README though.
> 
> How about something like this? Though this isn't really a model of clarity.

Could work, but I would also have it check the configs that are set in the
kernel, and only show the options that are available with the current
configs that are enabled in the kernel.

> 
>     --- a/kernel/trace/trace.c
>     +++ b/kernel/trace/trace.c
>     @@ -4292,7 +4292,7 @@ static void print_lat_help_header(struct seq_file *m)
>     {
>             seq_puts(m, "#                    _------=> CPU#            \n"
>                     "#                   / _-----=> irqs-off/BH-disabled\n"
>     -                   "#                  | / _----=> need-resched    \n"
>     +                   "#                  | / _----=> need-resched  [ l: lazy, n: now, p: preempt, b: l|n, L: l|p, N: n|p, B: l|n|p ]\n"
>                     "#                  || / _---=> hardirq/softirq \n"
>                     "#                  ||| / _--=> preempt-depth   \n"
>                     "#                  |||| / _-=> migrate-disable \n"
> 
> 
> Also, haven't looked at trace-cmd. Anything I should be sending a patch
> out for?

There's really nothing that explains it. But that probably should be fixed.

-- Steve
  

On Wed, Feb 21, 2024 at 04:43:34PM -0500, Steven Rostedt wrote:
> On Mon, 12 Feb 2024 21:55:53 -0800
> Ankur Arora <ankur.a.arora@oracle.com> wrote:
> 
> > Document various combinations of resched flags.
> > 
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Originally-by: Thomas Gleixner <tglx@linutronix.de>
> > Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
> > Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> > ---
> >  Documentation/trace/ftrace.rst | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> > index 7e7b8ec17934..7f20c0bae009 100644
> > --- a/Documentation/trace/ftrace.rst
> > +++ b/Documentation/trace/ftrace.rst
> > @@ -1036,8 +1036,12 @@ explains which is which.
> >  		be printed here.
> >  
> >    need-resched:
> > -	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED is set,
> > +	- 'B' all three, TIF_NEED_RESCHED, TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
> > +	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED are set,
> > +	- 'L' both TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
> > +	- 'b' both TIF_NEED_RESCHED and TIF_NEED_RESCHED_LAZY are set,
> >  	- 'n' only TIF_NEED_RESCHED is set,
> > +	- 'l' only TIF_NEED_RESCHED_LAZY is set,
> >  	- 'p' only PREEMPT_NEED_RESCHED is set,

One thing I was curious about in current code, under which situation will
"only PREEMPT_NEED_RESCHED is set" case happen? All the code paths I see set
the PREEMPT_NEED_RESCHED when then TIF has already been set. That kind of
makes sense, why enter the scheduler on preempt_enable() unless there was a
reason to, and TIF should have recorded that reason.

So in other words, if 'p' above cannot happen, then it could be removed as a
potential need-resched stat. If it can happen, I'd love to learn in which
case?

Also considering all users of set_tsk_need_resched() also call the
set_preempt_need_resched() shortly after, should we add a helper to squash
the 2 calls and simplify callers?

There are some "special cases" where only TIF flag need be set (such as setting
rescheduling from an interrupt or when rescheduling a remote CPU). For those,
they can directly call the set_tsk_need_resched() like they do now.

thanks,

 - Joel
  

Joel Fernandes <joel@joelfernandes.org> writes:

> On Wed, Feb 21, 2024 at 04:43:34PM -0500, Steven Rostedt wrote:
>> On Mon, 12 Feb 2024 21:55:53 -0800
>> Ankur Arora <ankur.a.arora@oracle.com> wrote:
>>
>> > Document various combinations of resched flags.
>> >
>> > Cc: Steven Rostedt <rostedt@goodmis.org>
>> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
>> > Cc: Jonathan Corbet <corbet@lwn.net>
>> > Originally-by: Thomas Gleixner <tglx@linutronix.de>
>> > Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
>> > Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> > ---
>> >  Documentation/trace/ftrace.rst | 6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
>> > index 7e7b8ec17934..7f20c0bae009 100644
>> > --- a/Documentation/trace/ftrace.rst
>> > +++ b/Documentation/trace/ftrace.rst
>> > @@ -1036,8 +1036,12 @@ explains which is which.
>> >  		be printed here.
>> >
>> >    need-resched:
>> > -	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED is set,
>> > +	- 'B' all three, TIF_NEED_RESCHED, TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>> > +	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED are set,
>> > +	- 'L' both TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>> > +	- 'b' both TIF_NEED_RESCHED and TIF_NEED_RESCHED_LAZY are set,
>> >  	- 'n' only TIF_NEED_RESCHED is set,
>> > +	- 'l' only TIF_NEED_RESCHED_LAZY is set,
>> >  	- 'p' only PREEMPT_NEED_RESCHED is set,
>
> One thing I was curious about in current code, under which situation will
> "only PREEMPT_NEED_RESCHED is set" case happen? All the code paths I see set
> the PREEMPT_NEED_RESCHED when then TIF has already been set. That kind of
> makes sense, why enter the scheduler on preempt_enable() unless there was a
> reason to, and TIF should have recorded that reason.

True. And, the only place where we clear PREEMPT_NEED_RESCHED is in
__schedule() after clearing the TIF_NEED_RESCHED* flags.

> So in other words, if 'p' above cannot happen, then it could be removed as a
> potential need-resched stat. If it can happen, I'd love to learn in which
> case?

Yeah, AFAICT the only case we might see it alone is in case of a bug.

> Also considering all users of set_tsk_need_resched() also call the
> set_preempt_need_resched() shortly after, should we add a helper to squash
> the 2 calls and simplify callers?
>
Just to explicitly lay out the reason for these being separate interfaces:
set_tsk_need_resched() can be set from a remote CPU, while
set_preempt_need_resched() (or its folding version) is only meant to be
used on the local CPU.

> There are some "special cases" where only TIF flag need be set (such as setting
> rescheduling from an interrupt or when rescheduling a remote CPU). For those,
> they can directly call the set_tsk_need_resched() like they do now.

The remote case, as you say is always handled in the scheduler. So, maybe
it is worth having a set_need_resched_local_cpu() which takes care of
both of these things but more importantly makes clear the limits of this.

That said, these interfaces have very few users (just RCU, and the s390
page fault handler) and both of these are fairly sophisticated users, so
not sure yet another interface in this area is worth adding.

Thanks

--
ankur
  

On 3/1/2024 10:09 PM, Ankur Arora wrote:
> 
> Joel Fernandes <joel@joelfernandes.org> writes:
> 
>> On Wed, Feb 21, 2024 at 04:43:34PM -0500, Steven Rostedt wrote:
>>> On Mon, 12 Feb 2024 21:55:53 -0800
>>> Ankur Arora <ankur.a.arora@oracle.com> wrote:
>>>
>>>> Document various combinations of resched flags.
>>>>
>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>> Originally-by: Thomas Gleixner <tglx@linutronix.de>
>>>> Link: https://lore.kernel.org/lkml/87jzshhexi.ffs@tglx/
>>>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>>>> ---
>>>>  Documentation/trace/ftrace.rst | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
>>>> index 7e7b8ec17934..7f20c0bae009 100644
>>>> --- a/Documentation/trace/ftrace.rst
>>>> +++ b/Documentation/trace/ftrace.rst
>>>> @@ -1036,8 +1036,12 @@ explains which is which.
>>>>  		be printed here.
>>>>
>>>>    need-resched:
>>>> -	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED is set,
>>>> +	- 'B' all three, TIF_NEED_RESCHED, TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>>>> +	- 'N' both TIF_NEED_RESCHED and PREEMPT_NEED_RESCHED are set,
>>>> +	- 'L' both TIF_NEED_RESCHED_LAZY and PREEMPT_NEED_RESCHED are set,
>>>> +	- 'b' both TIF_NEED_RESCHED and TIF_NEED_RESCHED_LAZY are set,
>>>>  	- 'n' only TIF_NEED_RESCHED is set,
>>>> +	- 'l' only TIF_NEED_RESCHED_LAZY is set,
>>>>  	- 'p' only PREEMPT_NEED_RESCHED is set,
>>
>> One thing I was curious about in current code, under which situation will
>> "only PREEMPT_NEED_RESCHED is set" case happen? All the code paths I see set
>> the PREEMPT_NEED_RESCHED when then TIF has already been set. That kind of
>> makes sense, why enter the scheduler on preempt_enable() unless there was a
>> reason to, and TIF should have recorded that reason.
> 
> True. And, the only place where we clear PREEMPT_NEED_RESCHED is in
> __schedule() after clearing the TIF_NEED_RESCHED* flags.

Thanks for taking a look.

>> So in other words, if 'p' above cannot happen, then it could be removed as a
>> potential need-resched stat. If it can happen, I'd love to learn in which
>> case?
> 
> Yeah, AFAICT the only case we might see it alone is in case of a bug.

Thanks for confirming as well. Steve, are you Ok with a patch to remove 'p'? Or
you probably want to leave it alone in case something changes in the future? I'd
rather it be removed.

>> Also considering all users of set_tsk_need_resched() also call the
>> set_preempt_need_resched() shortly after, should we add a helper to squash
>> the 2 calls and simplify callers?
>>
> Just to explicitly lay out the reason for these being separate interfaces:
> set_tsk_need_resched() can be set from a remote CPU, while
> set_preempt_need_resched() (or its folding version) is only meant to be
> used on the local CPU.

Yes, agreed.

>> There are some "special cases" where only TIF flag need be set (such as setting
>> rescheduling from an interrupt or when rescheduling a remote CPU). For those,
>> they can directly call the set_tsk_need_resched() like they do now.
> 
> The remote case, as you say is always handled in the scheduler. So, maybe
> it is worth having a set_need_resched_local_cpu() which takes care of
> both of these things but more importantly makes clear the limits of this.
> 
> That said, these interfaces have very few users (just RCU, and the s390
> page fault handler) and both of these are fairly sophisticated users, so
> not sure yet another interface in this area is worth adding.
> 

Sounds good, though there are quite a few RCU users and I do remember in the
past, that during a code review I pointed out that both needed to be set. Not
just one. :) I wonder what Paul McKenney thinks about this as well ;-)

thanks,

 - Joel