Re: [PATCH] genirq/affinity: add helper of irq_affinity_calc_sets

[Bjorn Helgaas]

On Thu, Jul 15, 2021 at 07:18:27PM +0800, Ming Lei wrote:
> When driver requests to allocate irq affinity managed vectors,
> pci_alloc_irq_vectors_affinity() may fallback to single vector
> allocation. In this situation, we don't need to call
> irq_create_affinity_masks for calling into ->calc_sets() for
> avoiding potential memory leak, so add the helper for this purpose.
> 
> Fixes: c66d4bd110a1 ("genirq/affinity: Add new callback for (re)calculating interrupt sets")
> Reported-by: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Cc: Christoph Hellwig <hch@xxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
>  drivers/pci/msi.c         |  3 ++-
>  include/linux/interrupt.h |  7 +++++++
>  kernel/irq/affinity.c     | 29 ++++++++++++++++++-----------
>  3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 9232255c8515..3d6db20d1b2b 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1224,7 +1224,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>  			 * for the single interrupt case.
>  			 */
>  			if (affd)
> -				irq_create_affinity_masks(1, affd);
> +				WARN_ON_ONCE(irq_affinity_calc_sets(1, affd));

Hmmm.  Not sure I like this yet:

  - I prefer required code to be on its own, not hidden inside a
    WARN() (personal preference, I know).

  - WARN() doesn't seem like the right thing here.  I think this
    generates a backtrace but the driver that called this has no
    indication.  Isn't the problem that a .calc_sets() method set
    "affd->nr_sets > IRQ_AFFINITY_MAX_SETS"?

    It looks like those methods are supplied by drivers
    (nvme_calc_irq_sets(), csio_calc_sets()) and it seems like they
    should find out about this somehow.

>  			pci_intx(dev, 1);
>  			return 1;
>  		}
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 2ed65b01c961..c7ff84d60465 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -340,6 +340,7 @@ irq_create_affinity_masks(unsigned int nvec, struct irq_affinity *affd);
>  
>  unsigned int irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  				       const struct irq_affinity *affd);
> +int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd);
>  
>  #else /* CONFIG_SMP */
>  
> @@ -391,6 +392,12 @@ irq_calc_affinity_vectors(unsigned int minvec, unsigned int maxvec,
>  	return maxvec;
>  }
>  
> +static inline int irq_affinity_calc_sets(unsigned int affvecs,
> +					 struct irq_affinity *affd)
> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_SMP */
>  
>  /*
> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
> index 4d89ad4fae3b..735f697d7d15 100644
> --- a/kernel/irq/affinity.c
> +++ b/kernel/irq/affinity.c
> @@ -405,6 +405,23 @@ static void default_calc_sets(struct irq_affinity *affd, unsigned int affvecs)
>  	affd->set_size[0] = affvecs;
>  }
>  
> +int irq_affinity_calc_sets(unsigned int affvecs, struct irq_affinity *affd)
> +{
> +	/*
> +	 * Simple invocations do not provide a calc_sets() callback. Install
> +	 * the generic one.
> +	 */
> +	if (!affd->calc_sets)
> +		affd->calc_sets = default_calc_sets;
> +
> +	/* Recalculate the sets */
> +	affd->calc_sets(affd, affvecs);
> +
> +	if (affd->nr_sets > IRQ_AFFINITY_MAX_SETS)
> +		return -ERANGE;
> +	return 0;
> +}
> +
>  /**
>   * irq_create_affinity_masks - Create affinity masks for multiqueue spreading
>   * @nvecs:	The total number of vectors
> @@ -429,17 +446,7 @@ irq_create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
>  	else
>  		affvecs = 0;
>  
> -	/*
> -	 * Simple invocations do not provide a calc_sets() callback. Install
> -	 * the generic one.
> -	 */
> -	if (!affd->calc_sets)
> -		affd->calc_sets = default_calc_sets;
> -
> -	/* Recalculate the sets */
> -	affd->calc_sets(affd, affvecs);
> -
> -	if (WARN_ON_ONCE(affd->nr_sets > IRQ_AFFINITY_MAX_SETS))
> +	if (WARN_ON_ONCE(irq_affinity_calc_sets(affvecs, affd)))
>  		return NULL;
>  
>  	/* Nothing to assign? */
> -- 
> 2.31.1
>