Re: [PATCH 1/3] xen/sched: allow rcu work to happen when syncing cpus in core scheduling

[Jürgen Groß <jgross@xxxxxxxx>]

On 07.05.20 20:34, Dario Faggioli wrote:
> On Thu, 2020-04-30 at 17:15 +0200, Juergen Gross wrote:
> > With RCU barriers moved from tasklets to normal RCU processing cpu
> > offlining in core scheduling might deadlock due to cpu
> > synchronization
> > required by RCU processing and core scheduling concurrently.
> >
> > Fix that by bailing out from core scheduling synchronization in case
> > of pending RCU work. Additionally the RCU softirq is now required to
> > be of higher priority than the scheduling softirqs in order to do
> > RCU processing before entering the scheduler again, as bailing out
> > from
> > the core scheduling synchronization requires to raise another softirq
> > SCHED_SLAVE, which would bypass RCU processing again.
> >
> > Reported-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> > Tested-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> In general, I'm fine with this patch and it can have my:
>
> Acked-by: Dario Faggioli <dfaggioli@xxxxxxxx>
>
> I'd ask for one thing, but that doesn't affect the ack, as it's not
> "my" code. :-)
>
> > diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
> > index b4724f5c8b..1f6c4783da 100644
> > --- a/xen/include/xen/softirq.h
> > +++ b/xen/include/xen/softirq.h
> > @@ -4,10 +4,10 @@
> >   /* Low-latency softirqs come first in the following list. */
> >   enum {
> >       TIMER_SOFTIRQ = 0,
> > +    RCU_SOFTIRQ,
> >       SCHED_SLAVE_SOFTIRQ,
> >       SCHEDULE_SOFTIRQ,
> >       NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ,
> > -    RCU_SOFTIRQ,
> >       TASKLET_SOFTIRQ,
> >       NR_COMMON_SOFTIRQS
> >   };
> So, until now, it was kind of intuitive (at least, it was to me :-) )
> that the TIMER_SOFTIRQ, we want it first, and the SCHEDULE one right
> after it. And the comment above the enum ("Low-latency softirqs come
> first in the following list"), although brief, is effective.
>
> With the introduction of SCHED_SLAVE, things became slightly more
> complex, but it still is not too far a reach to figure out the fact
> that we want it to be above SCHEDULE, and the reasons for that.
>
> Now that we're moving RCU from (almost) the very bottom to up here, I
> think we need some more info, there in the code. Sure all the bits and
> pieces are there in the changelogs, but I think it would be rather
> helpful to have them easily available to people trying to understand or
> modifying this code, e.g., with a comment.
That's reasonable.

> I was also thinking that, even better than a comment, would be a
> (build?) BUG_ON if RCU has no smaller value than SCHED_SLAVE and SLAVE.
> Not here, of course, but maybe close to some piece of code that relies
> on this assumption. Something that, if I tomorrow put the SCHED* ones
> on top again, would catch my attention and tell me that I either take
> care of that code path too, or I can't do it.
>
> However, I'm not sure whether, e.g., the other hunk of this patch would
> be a suitable place for something like this. And I can't, out of the
> top of my head, think of a really good place for where to put it.
> Therefore, I'm "only" asking for the comment... but if you (or others)
> have ideas, that'd be cool. :-)
I think the other hunk is exactly where the BUILD_BUG_ON() should be.
And this is a perfect place for the comment, too, as its placement will
explain the context very well.


Juergen