Messages in this thread | | | Date | Wed, 7 Sep 2022 09:23:44 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v3 3/6] sched: Change wait_task_inactive()s match_state |
| |
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Sun, Sep 04, 2022 at 12:44:36PM +0200, Ingo Molnar wrote: > > > > * Peter Zijlstra <peterz@infradead.org> wrote: > > > > > Make wait_task_inactive()'s @match_state work like ttwu()'s @state. > > > > > > That is, instead of an equal comparison, use it as a mask. This allows > > > matching multiple block conditions. > > > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > --- > > > kernel/sched/core.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > --- a/kernel/sched/core.c > > > +++ b/kernel/sched/core.c > > > @@ -3295,7 +3295,7 @@ unsigned long wait_task_inactive(struct > > > * is actually now running somewhere else! > > > */ > > > while (task_running(rq, p)) { > > > - if (match_state && unlikely(READ_ONCE(p->__state) != match_state)) > > > + if (match_state && !(READ_ONCE(p->__state) & match_state)) > > > return 0; > > > > We lose the unlikely annotation there - but I guess it probably never > > really mattered anyway? > > So any wait_task_inactive() caller does want that case to be true, but > the whole match_state precondition mostly wrecks things anyway. If > anything it should've been: > > if (likely(match_state && !(READ_ONCE(p->__state) & match_state))) > return 0; > > but I can't find it in me to care too much here.
Yeah, I agree that this is probably the most likely branch - and default compiler code generation behavior should be pretty close to that to begin with.
Ie. ack on dropping the unlikely() annotation. :-)
Might make sense to add a sentence to the changelog though, in case anyone (like me) is wondering about whether the dropped annotation was intended.
Thanks,
Ingo
| |