linux-kernel - Re: [PATCH 03/30] thread_info: tif_need_resched() now takes resched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87le7ekct9.fsf@oracle.com>
Date: Tue, 20 Feb 2024 14:09:38 -0800
From: Ankur Arora <ankur.a.arora@...cle.com>
To: Mark Rutland <mark.rutland@....com>
Cc: Ankur Arora <ankur.a.arora@...cle.com>, linux-kernel@...r.kernel.org,
        tglx@...utronix.de, peterz@...radead.org,
        torvalds@...ux-foundation.org, paulmck@...nel.org,
        akpm@...ux-foundation.org, luto@...nel.org, bp@...en8.de,
        dave.hansen@...ux.intel.com, hpa@...or.com, mingo@...hat.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org, willy@...radead.org,
        mgorman@...e.de, jpoimboe@...nel.org, jgross@...e.com,
        andrew.cooper3@...rix.com, bristot@...nel.org,
        mathieu.desnoyers@...icios.com, geert@...ux-m68k.org,
        glaubitz@...sik.fu-berlin.de, anton.ivanov@...bridgegreys.com,
        mattst88@...il.com, krypton@...ich-teichert.org, rostedt@...dmis.org,
        David.Laight@...lab.com, richard@....at, mjguzik@...il.com,
        jon.grimm@....com, bharata@....com, raghavendra.kt@....com,
        boris.ostrovsky@...cle.com, konrad.wilk@...cle.com,
        Arnd Bergmann
 <arnd@...db.de>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 03/30] thread_info: tif_need_resched() now takes
 resched_t as param


Mark Rutland <mark.rutland@....com> writes:

> On Wed, Feb 14, 2024 at 08:08:30PM -0800, Ankur Arora wrote:
>>
>> Mark Rutland <mark.rutland@....com> writes:
>>
>> > On Mon, Feb 12, 2024 at 09:55:27PM -0800, Ankur Arora wrote:
>> >> tif_need_resched() now takes a resched_t parameter to decide the
>> >> immediacy of the need-resched.
>> >
>> > I see at the end of the series, most callers pass a constant:
>> >
>> > [mark@...rids:~/src/linux]% git grep -w tif_need_resched
>> > arch/s390/include/asm/preempt.h:        return !--S390_lowcore.preempt_count && tif_need_resched(NR_now);
>> > arch/s390/include/asm/preempt.h:                        tif_need_resched(NR_now));
>> > include/asm-generic/preempt.h:  return !--*preempt_count_ptr() && tif_need_resched(NR_now);
>> > include/asm-generic/preempt.h:                  tif_need_resched(NR_now));
>> > include/linux/preempt.h:        if (tif_need_resched(NR_now)) \
>> > include/linux/sched.h:  return unlikely(tif_need_resched(NR_now));
>> > include/linux/sched.h:          unlikely(tif_need_resched(NR_lazy));
>> > include/linux/thread_info.h:static __always_inline bool tif_need_resched(resched_t rs)
>> > include/linux/thread_info.h:     * With !PREEMPT_AUTO tif_need_resched(NR_lazy) is defined
>> > kernel/entry/common.c:          if (tif_need_resched(NR_now))
>> > kernel/sched/debug.c:   nr = tif_need_resched(NR_now) ? "need_resched" : "need_resched_lazy";
>> > kernel/trace/trace.c:   if (tif_need_resched(NR_now))
>> > kernel/trace/trace.c:   if (tif_need_resched(NR_lazy))
>> >
>> > I think it'd be clearer if we had tif_need_resched_now() and
>> > tif_need_resched_lazy() wrappers rather than taking a parameter. I think that
>> > if we did similar elsewhere (e.g. {set,test}_tsk_need_resched_{now,lazy}()),
>> > it'd be a bit cleaner overall, since we can special-case the lazy behaviour
>> > more easily/clearly.
>>
>> So, we have three need-resched interfaces:
>>
>> 1. need_resched(), need_resched_lazy()
>>  These are used all over non-core (and idle) code, and I don't
>>  see a case where the user would find it useful to dynamically
>>  choose one or the other.
>>  So, here two separate interfaces, need_resched()/need_resched_lazy()
>>  make sense.
>>
>> 2. tif_need_resched()
>>  This is mostly used from preempt.h or scheduler adjacent code to drive
>>  preemption and at least in current uses, the resched_t param is a
>>  compile time constant.
>>
>>  I think the scheduler might find it useful in some cases to parametrize
>>  it (ex. maybe the scheduler knows how long which bit has been set for
>>  over long and wants to pass that on to resched_latency_warn().)
>>
>>  But that's a contrived example. I think this one would be fine
>>  either way. Will try it out and see which (tif_need_resched(rs),
>>  or tif_need_resched_now()/tif_need_resched_lazy()) seems cleaner.
>>
>> 3. *_tsk_need_resched()
>>  This is is used almost entirely from the scheduler and RCU.
>>
>>  One place where I found the ability to parametrize quite useful
>>  was __resched_curr(). So this I would like to keep.
>>
>> All of that said, and I wonder if we need these new interfaces at all.
>> Most of the code only uses the NR_now interface. Only the scheduler and
>> the entry code need to distinguish between lazy and eager.
>> (Plus, this way lazy and eager becomes an implementation detail which
>> doesn't need to be known outside the scheduler. Which is also kind of
>> the point of PREEMPT_AUTO.)
>>
>> Say something like the patch below (and similar for tif_need_resched(),
>> need_resched() etc.)
>>
>> What do you think?
>>
>> Ankur
>>
>> ---------
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 58e6ea7572a0..b836b238b117 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1953,7 +1953,7 @@ static inline bool test_tsk_thread_flag(struct task_struct *tsk, int flag)
>>   * tif_resched(NR_now). Add a check in the helpers below to ensure
>>   * we don't touch the tif_reshed(NR_now) bit unnecessarily.
>>   */
>> -static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> +static inline void __set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>>  {
>>  	if (IS_ENABLED(CONFIG_PREEMPT_AUTO) || rs == NR_now)
>>  		set_tsk_thread_flag(tsk, tif_resched(rs));
>> @@ -1964,6 +1964,11 @@ static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>>  		BUG();
>>  }
>>
>> +static inline void set_tsk_need_resched(struct task_struct *tsk, resched_t rs)
>> +{
>> +	__set_tsk_need_resched(tsk, NR_now);
>> +}
>
> I assume for this (and test_tsk_need_resched() below), you mean to drop the
> resched_t argument, i.e. this should be:
>
> 	static inline void set_tsk_need_resched(struct task_struct *tsk)
> 	{
> 		__set_tsk_need_resched(tsk, NR_now);
> 	}
>
> Assuming so, this looks good to me!

Yup. Great.

--
ankur