On Wed, Jul 17, 2019 at 03:05:47PM +0000, Hyong Youb Kim (hyonkim) wrote: > > -----Original Message----- > > From: Nithin Kumar Dabilpuram <ndabilpu...@marvell.com> > > Sent: Wednesday, July 17, 2019 11:36 PM > [...] > > > > Subject: [PATCH v2 2/3] eal: add ack interrupt API > > > > > > > > Add new ack interrupt API to avoid using > > > > VFIO_IRQ_SET_ACTION_TRIGGER(rte_intr_enable()) for > > > > acking interrupt purpose for VFIO based interrupt handlers. > > > > This implementation is specific to Linux. > > > > > > > > Using rte_intr_enable() for acking interrupt has below issues > > > > > > > > * Time consuming to do for every interrupt received as it will > > > > free_irq() followed by request_irq() and all other initializations > > > > * A race condition because of a window between free_irq() and > > > > request_irq() with packet reception still on and device still > > > > enabled and would throw warning messages like below. > > > > [158764.159833] do_IRQ: 9.34 No irq handler for vector > > > > > > > > In this patch, rte_intr_ack() is a no-op for VFIO_MSIX/VFIO_MSI > > interrupts > > > > as they are edge triggered and kernel would not mask the interrupt > > before > > > > delivering the event to userspace and we don't need to ack. > > > > > > > > Signed-off-by: Nithin Dabilpuram <ndabilpu...@marvell.com> > > > > Signed-off-by: Jerin Jacob <jer...@marvell.com> > > > > --- > > > > v2: > > > > * No change > > > > > > > > lib/librte_eal/common/include/rte_interrupts.h | 22 +++++++ > > > > lib/librte_eal/freebsd/eal/eal_interrupts.c | 9 +++ > > > > lib/librte_eal/linux/eal/eal_interrupts.c | 81 > > > > ++++++++++++++++++++++++++ > > > > lib/librte_eal/rte_eal_version.map | 1 + > > > > 4 files changed, 113 insertions(+) > > > > > > > > diff --git a/lib/librte_eal/common/include/rte_interrupts.h > > > > b/lib/librte_eal/common/include/rte_interrupts.h > > > > index c1e912c..93b31cd 100644 > > > > --- a/lib/librte_eal/common/include/rte_interrupts.h > > > > +++ b/lib/librte_eal/common/include/rte_interrupts.h > > > > @@ -118,6 +118,28 @@ int rte_intr_enable(const struct rte_intr_handle > > > > *intr_handle); > > > > */ > > > > int rte_intr_disable(const struct rte_intr_handle *intr_handle); > > > > > > > > +/** > > > > + * It acks an interrupt raised for the specified handle. > > > > + * > > > > + * Call this function to ack an interrupt from interrupt > > > > + * handler either from application or driver, so that > > > > + * new interrupts are raised. > > > > + * > > > > + * @note For interrupt handle types VFIO_MSIX and VFIO_MSI, > > > > + * this function is a no-op and returns success without > > > > + * changing anything as kernel doesn't expect > > > > + * them to be acked. > > > > + * > > > [...] > > > > > > Shouldn't we explain that this really is "unmask" but named "ack" because > > > of x and y, and that it is expected at end of INTx handler? Ack does > > > not have a well-defined meaning, whereas everyone knows what unmask > > > means.. > > > > > > > > > Ok. Is the below text fine with you ? Or please suggest. > > > > @note For interrupt handle types VFIO_MSIX and VFIO_MSI, > > this function is a no-op and returns success without > > changing anything as kernel doesn't expect > > them to be acked. > > This needs be used atleast for PCI devices with INTx interrupt > > as kernel before passing on event for INTx triggered interrupt, > > masks the interrupt and expects application to unmask it so that, > > further interrupts can be raised/triggered. This is also due to > > the fact that INTx is level triggered interrupt where as MSI/MSIx > > is not. Ideally this should have been called as intr_unmask() > > representing underlying api, but since unmask operation > > is not supported and not needed for VFIO MSI/MSIx interrupts > > after handling, it is named as ack. > > > > How about this? > > PMD generally calls this function at the end of its IRQ callback. > Internally, it unmasks the interrupt if possible. For INTx, unmasking > is required as the interrupt is auto-masked prior to invoking > callback. For MSI/MSI-X, unmasking is typically not needed as the > interrupt is not auto-masked. In fact, for interrupt handle types > VFIO_MSIX and VFIO_MSI, this function is no-op. > Ok. Thanks. Will add this in next revision.
> Thanks for your effort.. > -Hyong >
