Re: [Xen-devel] [PATCH v5 2/2] x86: add accessors for scratch cpu mask

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 11, 2020 at 05:20:23PM +0100, Jan Beulich wrote:
> On 11.03.2020 16:51, Roger Pau Monné wrote:
> > On Wed, Mar 11, 2020 at 04:37:50PM +0100, Jan Beulich wrote:
> >> On 11.03.2020 16:34, Roger Pau Monné wrote:
> >>> On Fri, Feb 28, 2020 at 01:42:58PM +0100, Jan Beulich wrote:
> >>>> On 28.02.2020 13:07, Roger Pau Monne wrote:
> >>>>> Current usage of the per-CPU scratch cpumask is dangerous since
> >>>>> there's no way to figure out if the mask is already being used except
> >>>>> for manual code inspection of all the callers and possible call paths.
> >>>>>
> >>>>> This is unsafe and not reliable, so introduce a minimal get/put
> >>>>> infrastructure to prevent nested usage of the scratch mask and usage
> >>>>> in interrupt context.
> >>>>>
> >>>>> Move the declaration of scratch_cpumask to smp.c in order to place the
> >>>>> declaration and the accessors as close as possible.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>>>
> >>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>> Ping? This seems to have the required RB, but hasn't been committed.
> >>
> >> While as per the R-b this technically is fine, I continue to be
> >> uncertain whether we actually want to go this far.
> > 
> > If this had been in place 5500d265a2a8fa6 ('x86/smp: use APIC ALLBUT
> > destination shorthand when possible') wouldn't have introduced a
> > bogus usage of the scratch per cpu mask, as the check would have
> > triggered.
> > 
> > After finding that one of my commits introduced a bug I usually do the
> > exercise of trying to figure out which checks or safeguards would have
> > prevented it, and hence came up with this patch.
> > 
> > I would also like to note that this adds 0 overhead to non-debug
> > builds.
> > 
> >> Andrew, as
> >> per a discussion we had when I was pondering whether to commit
> >> this, also looks to have similar concerns (which iirc he said he
> >> had voiced on irc).
> > 
> > Is the concern only related to the fact that you have to use the
> > get/put accessors and thus more lines of code are added, or is there
> > something else?
> 
> Afaic - largely this, along with it making it more likely that
> error paths will be non-trivial (and hence possibly get converted
> to use goto-s). I can't speak for Andrew, of course.

FTR I think being able to programmatically spot misuses of the scratch
cpumask is more important than having clearer error paths. I also
think the changes required to enforce this are not that intrusive, as
I switched all current users of the scratch cpumask and didn't have to
add any labels at all to handle errors.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel