Re: [Xen-devel] [PATCH 5/9] x86/IRQ: fix locking around vector management

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

On Mon, Apr 29, 2019 at 05:25:03AM -0600, Jan Beulich wrote:
> All of __{assign,bind,clear}_irq_vector() manipulate struct irq_desc
> fields, and hence ought to be called with the descriptor lock held in
> addition to vector_lock. This is currently the case for only
> set_desc_affinity() and destroy_irq(), which also clarifies what the

AFAICT set_desc_affinity is called from set_ioapic_affinity_irq which in
turn is called from setup_ioapic_dest without holding the desc lock.
Is this fine because that's only used a boot time?

> nesting behavior between the locks has to be. Reflect the new
> expectation by having these functions all take a descriptor as
> parameter instead of an interrupt number.
> 
> Drop one of the two leading underscores from all three functions at
> the same time.
> 
> There's one case left where descriptors get manipulated with just
> vector_lock held: setup_vector_irq() assumes its caller to acquire
> vector_lock, and hence can't itself acquire the descriptor locks (wrong
> lock order). I don't currently see how to address this.

Can you take the desc lock and vector lock for each irq in the second
loop of setup_vector_irq and remove the vector locking from the caller?

That might be inefficient, but it's just done for CPU initialization.

AFAICT the first loop of setup_vector_irq doesn't require any locking
since it's per-cpu initialization.

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Change looks good to me:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> 
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -27,6 +27,7 @@
>  #include <public/physdev.h>
>  
>  static int parse_irq_vector_map_param(const char *s);
> +static void _clear_irq_vector(struct irq_desc *desc);
>  
>  /* opt_noirqbalance: If true, software IRQ balancing/affinity is disabled. */
>  bool __read_mostly opt_noirqbalance;
> @@ -112,13 +113,12 @@ static void trace_irq_mask(u32 event, in
>      trace_var(event, 1, sizeof(d), &d);
>  }
>  
> -static int __init __bind_irq_vector(int irq, int vector, const cpumask_t 
> *cpu_mask)
> +static int __init _bind_irq_vector(struct irq_desc *desc, int vector,

I wouldn't be opposed to adding ASSERTs here (and in
_{assign,bind,clear}_irq_vector, set_desc_affinity and destroy_irq)
to check for lock correctness.

Thanks, Roger.

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