[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YPDqphXYXtkpcfch@T590>
Date: Fri, 16 Jul 2021 10:10:46 +0800
From: Ming Lei <ming.lei@...hat.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] genirq/affinity: add helper of irq_affinity_calc_sets
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@...nel.org>
> > Cc: linux-pci@...r.kernel.org
> > Cc: Christoph Hellwig <hch@....de>
> > Signed-off-by: Ming Lei <ming.lei@...hat.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
Powered by blists - more mailing lists