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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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.

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. :-)

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

Attachment: signature.asc
Description: This is a digitally signed message part