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

[Ming Lei <ming.lei@redhat.com>]

On Thu, Jul 15, 2021 at 09:27:14AM -0500, Bjorn Helgaas wrote:
> 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@kernel.org>
> > Cc: linux-pci@vger.kernel.org
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  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"?

Yes. When the warning is triggered, memory corruption may have been caused,
not sure if the indication is needed.

> 
>     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.

Yeah. The WARN() here is just to report the bug earlier.


Thanks, 
Ming