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

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

On Wed, Feb 26, 2020 at 11:36:52AM +0100, Jan Beulich wrote:
> On 24.02.2020 11:46, 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.
> 
> While I can see the reasoning (especially in light of the change
> which did violate to assumption), I'm still uncertain if this isn't
> "over-engineering". Andrew, do you have a clear opinion one way or
> the other here?
> 
> > Move the declaration of scratch_cpumask to smp.c in order to place the
> > declaration and the accessors as close as possible.
> 
> s/declaration/definition/g

Done.

> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -196,7 +196,7 @@ static void _clear_irq_vector(struct irq_desc *desc)
> >  {
> >      unsigned int cpu, old_vector, irq = desc->irq;
> >      unsigned int vector = desc->arch.vector;
> > -    cpumask_t *tmp_mask = this_cpu(scratch_cpumask);
> > +    cpumask_t *tmp_mask = get_scratch_cpumask();
> >  
> >      BUG_ON(!valid_irq_vector(vector));
> >  
> > @@ -223,7 +223,10 @@ static void _clear_irq_vector(struct irq_desc *desc)
> >      trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);
> >  
> >      if ( likely(!desc->arch.move_in_progress) )
> > +    {
> > +        put_scratch_cpumask();
> >          return;
> > +    }
> 
> I think if possible such error path adjustments would better be
> avoided. And this seems feasible here: There are two entirely
> independent used of the scratch mask in this function. You could
> therefore put the mask above from here, and get it again further
> down, or you could leverage a property of the current
> implementation, plus the fact that the 2nd use doesn't involved
> any "real" function calls, and avoid a 2nd get/put altogether.

No, it's very easy to add function calls later on and forget to use
get_scratch_cpumask.

> Of course another question then is whether it is a good property
> of the current model, i.e. whether it wouldn't be better for
> "put" to actually zap the pointer, to prevent subsequent use.

So that put_scratch_cpumask takes the pointer as a parameter and
writes NULL to it?

> > @@ -2531,12 +2536,12 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >      unsigned int irq;
> >      static int warned;
> >      struct irq_desc *desc;
> > +    cpumask_t *affinity = get_scratch_cpumask();
> >  
> >      for ( irq = 0; irq < nr_irqs; irq++ )
> >      {
> >          bool break_affinity = false, set_affinity = true;
> >          unsigned int vector;
> > -        cpumask_t *affinity = this_cpu(scratch_cpumask);
> >  
> >          if ( irq == 2 )
> >              continue;
> > @@ -2640,6 +2645,8 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
> >                     irq, CPUMASK_PR(affinity));
> >      }
> >  
> > +    put_scratch_cpumask();
> 
> Just as a remark, not necessarily as a request to change the code: I
> wonder if down the road this pretty wide scope of "holding" the mask
> isn't going to bite us, when a function called from here (in a range
> of code not actively needing the mask) also may want to use the mask.
> But of course we can make this finer grained at the point where it
> might actually start mattering.

We can always reduce the scope if there's a need for it, until then I
would rather leave this as-is.

> 
> > @@ -3645,12 +3647,17 @@ long do_mmuext_op(
> >                                     mask)) )
> >                  rc = -EINVAL;
> >              if ( unlikely(rc) )
> > +            {
> > +                put_scratch_cpumask();
> >                  break;
> > +            }
> 
> Again, instead of adjusting an error path, how about making this
> have an empty statement (i.e. dropping the break) and ...
> 
> >              if ( op.cmd == MMUEXT_TLB_FLUSH_MULTI )
> 
> ... having this become "else if()"?
> 
> > @@ -4384,6 +4393,9 @@ static int __do_update_va_mapping(
> >          break;
> >      }
> >  
> > +    if ( mask && mask != d->dirty_cpumask )
> > +        put_scratch_cpumask();
> 
> The right side of the && here makes things feel a little fragile for
> me.

What about using:

switch ( flags & ~UVMF_FLUSHTYPE_MASK )
{
case UVMF_LOCAL:
case UVMF_ALL:
    break;

default:
    put_scratch_cpumask();
}

I could also use an if, but I think it's clearer with a switch.

> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -159,13 +159,15 @@ void msi_compose_msg(unsigned vector, const cpumask_t 
> > *cpu_mask, struct msi_msg
> >  
> >      if ( cpu_mask )
> >      {
> > -        cpumask_t *mask = this_cpu(scratch_cpumask);
> > +        cpumask_t *mask;
> >  
> >          if ( !cpumask_intersects(cpu_mask, &cpu_online_map) )
> >              return;
> >  
> > +        mask = get_scratch_cpumask();
> >          cpumask_and(mask, cpu_mask, &cpu_online_map);
> >          msg->dest32 = cpu_mask_to_apicid(mask);
> > +        put_scratch_cpumask();
> >      }
> 
> This, I think, could do with a little more changing:
> 
>     if ( cpu_mask )
>     {
>         cpumask_t *mask = get_scratch_cpumask();
> 
>         cpumask_and(mask, cpu_mask, &cpu_online_map);
>         if ( !cpumask_empty(mask) )
>             msg->dest32 = cpu_mask_to_apicid(mask);
>         put_scratch_cpumask();
>     }
> 
> This way instead of looking twice at two cpumask_t instances, the
> 2nd one involves just one. Thoughts?

LGTM.

Note that this won't exit early however in case masks don't intersect,
and will set the address field with whatever is in msg->dest32.

> > --- a/xen/arch/x86/smp.c
> > +++ b/xen/arch/x86/smp.c
> > @@ -25,6 +25,31 @@
> >  #include <irq_vectors.h>
> >  #include <mach_apic.h>
> >  
> > +DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
> > +
> > +#ifndef NDEBUG
> > +cpumask_t *scratch_cpumask(bool use)
> > +{
> > +    static DEFINE_PER_CPU(void *, scratch_cpumask_use);
> > +
> > +    /*
> > +     * Due to reentrancy scratch cpumask cannot be used in IRQ, #MC or #NMI
> > +     * context.
> > +     */
> > +    BUG_ON(in_irq() || in_mc() || in_nmi());
> > +
> > +    if ( use && unlikely(this_cpu(scratch_cpumask_use)) )
> > +    {
> > +        printk("%p: scratch CPU mask already in use by %p\n",
> > +               __builtin_return_address(0), this_cpu(scratch_cpumask_use));
> 
> __builtin_return_address(0) simply shows another time what ...
> 
> > +        BUG();
> 
> ... this already will show. I'd suggest to drop it. Also I think
> you want %ps here.

Done, thanks.

Roger.

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