| Message ID | 161545564302.24868.14477928469038686899.stgit@Wayrath (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: fix for_each_cpu when NR_CPUS=1 | expand |
On 11.03.21 10:40, Dario Faggioli wrote: > When running an hypervisor build with NR_CPUS=1 for_each_cpu does not > take into account whether the bit of the CPU is set or not in the > provided mask. > > This means that whatever we have in the bodies of these loops is always > done once, even if the mask was empty and it should never be done. This > is clearly a bug and was in fact causing an assert to trigger in credit2 > code. > > Removing the special casing of NR_CPUS == 1 makes things work again. > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Ian Jackson <iwj@xenproject.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Julien Grall <julien@xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Wei Liu <wl@xen.org> > --- > I'm not really sure whether this should be 4.15 material. > > It's definitely a bug, IMO. The risk is also pretty low, considering > that no one should really run Xen in this configuration (NR_CPUS=1, I > mean). Which is also the reason why it's probably not really important > that we fix it at this point of the release cycle. > --- > xen/include/xen/cpumask.h | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h > index 256b60b106..e69589fc08 100644 > --- a/xen/include/xen/cpumask.h > +++ b/xen/include/xen/cpumask.h > @@ -368,15 +368,10 @@ static inline void free_cpumask_var(cpumask_var_t mask) > #define FREE_CPUMASK_VAR(m) free_cpumask_var(m) > #endif > > -#if NR_CPUS > 1 > #define for_each_cpu(cpu, mask) \ > for ((cpu) = cpumask_first(mask); \ > (cpu) < nr_cpu_ids; \ > (cpu) = cpumask_next(cpu, mask)) > -#else /* NR_CPUS == 1 */ > -#define for_each_cpu(cpu, mask) \ > - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)(mask)) > -#endif /* NR_CPUS */ Wouldn't it make sense to drop the other NR_CPUS == 1 optimization further down, too? IMO this is adding clutter for no real gain, as NR_CPUS == 1 Xen hypervisor builds aiming at high performance are probably not existing anywhere in the universe. Juergen
On 11.03.2021 10:40, Dario Faggioli wrote: > When running an hypervisor build with NR_CPUS=1 for_each_cpu does not > take into account whether the bit of the CPU is set or not in the > provided mask. > > This means that whatever we have in the bodies of these loops is always > done once, even if the mask was empty and it should never be done. This > is clearly a bug and was in fact causing an assert to trigger in credit2 > code. > > Removing the special casing of NR_CPUS == 1 makes things work again. > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> Doesn't this want a Reported-by: Roger? Reviewed-by: Jan Beulich <jbeulich@suse.com> And FTR I don't really mind the other NR_CPUS == 1 piece of logic to remain there. > I'm not really sure whether this should be 4.15 material. > > It's definitely a bug, IMO. The risk is also pretty low, considering > that no one should really run Xen in this configuration (NR_CPUS=1, I > mean). Which is also the reason why it's probably not really important > that we fix it at this point of the release cycle. I agree; I'll put it in the 4.16 bucket. Jan
On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote: > On 11.03.2021 10:40, Dario Faggioli wrote: > > > > Removing the special casing of NR_CPUS == 1 makes things work again. > > > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> > > Doesn't this want a Reported-by: Roger? > It definitely does! And I even forgot to Cc him... Sorry Roger :-( Will you add it, or do you want me to resubmit with it? > Reviewed-by: Jan Beulich <jbeulich@suse.com> > Thanks > And FTR I don't really mind the other NR_CPUS == 1 piece of logic to > remain there. > Ok. I agree with Juergen that they're totally useless, but at least they're not wrong. Oh, BTW, since you mentioned in your other email the fact that this comes from Linux, I've had a look there and there's a comment in their cpumask.h file, under the NR_CPUS==1 define, looking like this: /* Uniprocessor. Assume all masks are "1". */ https://elixir.bootlin.com/linux/latest/source/include/linux/cpumask.h#L149 Of course, that does not make the fact that for_each_cpu and for_each_cpu_not are identical less weird, and the whole thing still does not make sense, at least not to me. I'm wondering whether or not it is worth to report this to them too, as I have the impression that they just don't care. Thanks and Regards
On Thu, Mar 11, 2021 at 05:21:16PM +0100, Dario Faggioli wrote: > On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote: > > On 11.03.2021 10:40, Dario Faggioli wrote: > > > > > > Removing the special casing of NR_CPUS == 1 makes things work again. > > > > > > Signed-off-by: Dario Faggioli <dfaggioli@suse.com> > > > > Doesn't this want a Reported-by: Roger? > > > It definitely does! And I even forgot to Cc him... Sorry Roger :-( No problem! Thanks for sending the patch. > Will you add it, or do you want me to resubmit with it? > > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > Thanks > > > And FTR I don't really mind the other NR_CPUS == 1 piece of logic to > > remain there. > > > Ok. I agree with Juergen that they're totally useless, but at least > they're not wrong. > > Oh, BTW, since you mentioned in your other email the fact that this > comes from Linux, I've had a look there and there's a comment in their > cpumask.h file, under the NR_CPUS==1 define, looking like this: > > /* Uniprocessor. Assume all masks are "1". */ > > https://elixir.bootlin.com/linux/latest/source/include/linux/cpumask.h#L149 > > Of course, that does not make the fact that for_each_cpu and > for_each_cpu_not are identical less weird, and the whole thing still > does not make sense, at least not to me. > > I'm wondering whether or not it is worth to report this to them too, as > I have the impression that they just don't care. I would report it, worse case they will just ignore, but it would be nice to get it fixed there also, so that someone else doesn't have to discover the same brokenness. Roger.
On 11.03.2021 17:21, Dario Faggioli wrote: > On Thu, 2021-03-11 at 12:28 +0100, Jan Beulich wrote: >> On 11.03.2021 10:40, Dario Faggioli wrote: >>> >>> Removing the special casing of NR_CPUS == 1 makes things work again. >>> >>> Signed-off-by: Dario Faggioli <dfaggioli@suse.com> >> >> Doesn't this want a Reported-by: Roger? >> > It definitely does! And I even forgot to Cc him... Sorry Roger :-( > > Will you add it, or do you want me to resubmit with it? No need to, I've taken note. Jan
Dario Faggioli writes ("[PATCH] xen: fix for_each_cpu when NR_CPUS=1"):
> When running an hypervisor build with NR_CPUS=1 for_each_cpu does not
> take into account whether the bit of the CPU is set or not in the
> provided mask.
>
> This means that whatever we have in the bodies of these loops is always
> done once, even if the mask was empty and it should never be done. This
> is clearly a bug and was in fact causing an assert to trigger in credit2
> code.
>
> Removing the special casing of NR_CPUS == 1 makes things work again.
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> I'm not really sure whether this should be 4.15 material.
>
> It's definitely a bug, IMO. The risk is also pretty low, considering
> that no one should really run Xen in this configuration (NR_CPUS=1, I
> mean). Which is also the reason why it's probably not really important
> that we fix it at this point of the release cycle.
Given that it clearly only affects NR_CPUS==1, I think the risk/reward
tradeoff is unambiguously positive.
> -#if NR_CPUS > 1
> #define for_each_cpu(cpu, mask) \
> for ((cpu) = cpumask_first(mask); \
Just a thought: does cpumask_first work on an empty mask ?
Ian.
On 12.03.2021 15:03, Ian Jackson wrote: > Dario Faggioli writes ("[PATCH] xen: fix for_each_cpu when NR_CPUS=1"): >> -#if NR_CPUS > 1 >> #define for_each_cpu(cpu, mask) \ >> for ((cpu) = cpumask_first(mask); \ > > Just a thought: does cpumask_first work on an empty mask ? I'm sure it does, yes - it'll return a value >= nr_cpu_ids in this case. If it didn't work, NR_CPUS > 1 would also be badly affected. Jan
diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h index 256b60b106..e69589fc08 100644 --- a/xen/include/xen/cpumask.h +++ b/xen/include/xen/cpumask.h @@ -368,15 +368,10 @@ static inline void free_cpumask_var(cpumask_var_t mask) #define FREE_CPUMASK_VAR(m) free_cpumask_var(m) #endif -#if NR_CPUS > 1 #define for_each_cpu(cpu, mask) \ for ((cpu) = cpumask_first(mask); \ (cpu) < nr_cpu_ids; \ (cpu) = cpumask_next(cpu, mask)) -#else /* NR_CPUS == 1 */ -#define for_each_cpu(cpu, mask) \ - for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)(mask)) -#endif /* NR_CPUS */ /* * The following particular system cpumasks and operations manage
When running an hypervisor build with NR_CPUS=1 for_each_cpu does not take into account whether the bit of the CPU is set or not in the provided mask. This means that whatever we have in the bodies of these loops is always done once, even if the mask was empty and it should never be done. This is clearly a bug and was in fact causing an assert to trigger in credit2 code. Removing the special casing of NR_CPUS == 1 makes things work again. Signed-off-by: Dario Faggioli <dfaggioli@suse.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <george.dunlap@citrix.com> Cc: Ian Jackson <iwj@xenproject.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien@xen.org> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Wei Liu <wl@xen.org> --- I'm not really sure whether this should be 4.15 material. It's definitely a bug, IMO. The risk is also pretty low, considering that no one should really run Xen in this configuration (NR_CPUS=1, I mean). Which is also the reason why it's probably not really important that we fix it at this point of the release cycle. --- xen/include/xen/cpumask.h | 5 ----- 1 file changed, 5 deletions(-)