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

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

On Thu, Feb 13, 2020 at 11:12:12AM +0100, Jan Beulich wrote:
> On 12.02.2020 17:49, Roger Pau Monne wrote:
> > @@ -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'm not overly happy to see a need introduced to do cleanup like
> this one, but at least missing a path is a debug-build problem
> only.
> 
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -57,6 +57,30 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
> >  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
> >  static cpumask_t scratch_cpu0mask;
> >  
> > +#ifndef NDEBUG
> > +cpumask_t *scratch_cpumask(const char *fn)
> 
> Please don't pass an argument that you can deduce, and then
> provide even more meaningful data:
> 
> > +{
> > +    static DEFINE_PER_CPU(const char *, scratch_cpumask_use);
> > +
> > +    /*
> > +     * Scratch cpumask cannot be used in IRQ context, or else we would 
> > have to
> > +     * make sure all users have interrupts disabled while using the scratch
> > +     * mask.
> > +     */
> > +    BUG_ON(in_irq());
> > +
> > +    if ( fn && unlikely(this_cpu(scratch_cpumask_use)) )
> > +    {
> > +        printk("%s: scratch CPU mask already in use by %s\n",
> > +              fn, this_cpu(scratch_cpumask_use));
> 
> Use __builtin_return_address(0) here, which will allow
> identifying which of perhaps multiple uses in a function is
> the offending one.

Will change.

> 
> Also, why in smpboot.c instead of in smp.c? This isn't a
> boot or CPU-hot-online related function afaict.

I've added it to smpboot.c because that's where scratch_cpumask is
defined. I could move it to smp.c, but I would prefer to keep the
accessor as close as possible to the declaration.

> 
> Finally, it would seem nice if multiple uses by the same caller
> could be permitted:
> 
>     for ( ... )
>     {
>         if ( ... )
>         {
>             mask = get_scratch_cpumask();
>             ...
>         }
>         else
>         {
>             /* no use of get_scratch_cpumask() */
>             ...
>         }
>     }
> 
>     put_scratch_cpumask();

I have to admit I don't really like this kinds of asymmetric
constructions, what you suggest for example won't be valid if
get_scratch_cpumask took some kind of lock.

Thanks, Roger.

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