[v3,2/3] eal: add ack interrupt API
Checks
Commit Message
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 <ndabilpuram@marvell.com>
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
Tested-by: Shahed Shaikh <shshaikh@marvell.com>
---
v3:
* Update note on new api
v2:
* No change
lib/librte_eal/common/include/rte_interrupts.h | 24 ++++++++
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, 115 insertions(+)
Comments
On 18-Jul-19 9:46 AM, Nithin Dabilpuram wrote:
> 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 <ndabilpuram@marvell.com>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Tested-by: Shahed Shaikh <shshaikh@marvell.com>
> ---
<snip>
> */
> 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 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.
I've added this comment in previous revision's discussion, but i'll copy
it here as well.
Does the user of this API even cares about these details? I would think
that it would be easier to just mandate calling this function at the end
of each interrupt callback, regardless of which interrupt mode is used.
IMO internal details (about no-ops and such) are better explained in the
implementation, not in the public-facing API header.
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Thursday, July 18, 2019 6:43 PM
> To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Hyong Youb Kim
> <hyonkim@cisco.com>; David Marchand <david.marchand@redhat.com>;
> Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Bruce Richardson <bruce.richardson@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; John Daley
> <johndale@cisco.com>; Shahed Shaikh <shshaikh@marvell.com>;
> dev@dpdk.org
> Subject: [EXT] Re: [dpdk-dev] [PATCH v3 2/3] eal: add ack interrupt API
> On 18-Jul-19 9:46 AM, Nithin Dabilpuram wrote:
> > 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 <ndabilpuram@marvell.com>
> > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > Tested-by: Shahed Shaikh <shshaikh@marvell.com>
> > ---
>
> <snip>
>
> > */
> > 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 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.
>
> I've added this comment in previous revision's discussion, but i'll copy it here
> as well.
>
> Does the user of this API even cares about these details? I would think that it
> would be easier to just mandate calling this function at the end of each
> interrupt callback, regardless of which interrupt mode is used.
>
> IMO internal details (about no-ops and such) are better explained in the
> implementation, not in the public-facing API header.
+1 to move the comment under @note to implementation.
On 7/18/2019 6:57 PM, Jerin Jacob Kollanukkaran wrote:
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Thursday, July 18, 2019 6:43 PM
>> To: Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Hyong Youb Kim
>> <hyonkim@cisco.com>; David Marchand <david.marchand@redhat.com>;
>> Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> <ferruh.yigit@intel.com>; Bruce Richardson <bruce.richardson@intel.com>
>> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; John Daley
>> <johndale@cisco.com>; Shahed Shaikh <shshaikh@marvell.com>;
>> dev@dpdk.org
>> Subject: [EXT] Re: [dpdk-dev] [PATCH v3 2/3] eal: add ack interrupt API
>> On 18-Jul-19 9:46 AM, Nithin Dabilpuram wrote:
>>> 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 <ndabilpuram@marvell.com>
>>> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
>>> Tested-by: Shahed Shaikh <shshaikh@marvell.com>
>>> ---
>> <snip>
>>
>>> */
>>> 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 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.
>> I've added this comment in previous revision's discussion, but i'll copy it here
>> as well.
>>
>> Does the user of this API even cares about these details? I would think that it
>> would be easier to just mandate calling this function at the end of each
>> interrupt callback, regardless of which interrupt mode is used.
>>
>> IMO internal details (about no-ops and such) are better explained in the
>> implementation, not in the public-facing API header.
> +1 to move the comment under @note to implementation.
Ok. I'll just move this note alone to implementation of function in next
revision.
>
@@ -118,6 +118,30 @@ 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 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.
+ *
+ * @param intr_handle
+ * pointer to the interrupt handle.
+ *
+ * @return
+ * - On success, zero.
+ * - On failure, a negative value.
+ */
+__rte_experimental
+int rte_intr_ack(const struct rte_intr_handle *intr_handle);
+
#ifdef __cplusplus
}
#endif
@@ -387,6 +387,15 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
return 0;
}
+int
+rte_intr_ack(const struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV)
+ return 0;
+
+ return -1;
+}
+
static void
eal_intr_process_interrupts(struct kevent *events, int nfds)
{
@@ -197,6 +197,35 @@ vfio_disable_intx(const struct rte_intr_handle *intr_handle) {
return 0;
}
+/* unmask/ack legacy (INTx) interrupts */
+static int
+vfio_ack_intx(const struct rte_intr_handle *intr_handle)
+{
+ struct vfio_irq_set *irq_set;
+ char irq_set_buf[IRQ_SET_BUF_LEN];
+ int len, ret;
+
+ len = sizeof(struct vfio_irq_set);
+
+ /* unmask INTx */
+ irq_set = (struct vfio_irq_set *) irq_set_buf;
+ memset(irq_set, 0, len);
+ irq_set->argsz = len;
+ irq_set->count = 1;
+ irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK;
+ irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
+ irq_set->start = 0;
+
+ ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+ if (ret) {
+ RTE_LOG(ERR, EAL, "Error unmasking INTx interrupts for fd %d\n",
+ intr_handle->fd);
+ return -1;
+ }
+ return 0;
+}
+
/* enable MSI interrupts */
static int
vfio_enable_msi(const struct rte_intr_handle *intr_handle) {
@@ -694,6 +723,58 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
}
int
+rte_intr_ack(const struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV)
+ return 0;
+
+ if (!intr_handle || intr_handle->fd < 0 || intr_handle->uio_cfg_fd < 0)
+ return -1;
+
+ switch (intr_handle->type) {
+ /* Both acking and disabling are same for UIO */
+ case RTE_INTR_HANDLE_UIO:
+ if (uio_intr_enable(intr_handle))
+ return -1;
+ break;
+ case RTE_INTR_HANDLE_UIO_INTX:
+ if (uio_intx_intr_enable(intr_handle))
+ return -1;
+ break;
+ /* not used at this moment */
+ case RTE_INTR_HANDLE_ALARM:
+ return -1;
+#ifdef VFIO_PRESENT
+ /* Since VFIO_MSIX is implicitly acked
+ * unlike INTx, we report success
+ */
+ case RTE_INTR_HANDLE_VFIO_MSIX:
+ case RTE_INTR_HANDLE_VFIO_MSI:
+ return 0;
+ case RTE_INTR_HANDLE_VFIO_LEGACY:
+ if (vfio_ack_intx(intr_handle))
+ return -1;
+ break;
+#ifdef HAVE_VFIO_DEV_REQ_INTERFACE
+ case RTE_INTR_HANDLE_VFIO_REQ:
+ return -1;
+#endif
+#endif
+ /* not used at this moment */
+ case RTE_INTR_HANDLE_DEV_EVENT:
+ return -1;
+ /* unknown handle type */
+ default:
+ RTE_LOG(ERR, EAL,
+ "Unknown handle type of fd %d\n",
+ intr_handle->fd);
+ return -1;
+ }
+
+ return 0;
+}
+
+int
rte_intr_disable(const struct rte_intr_handle *intr_handle)
{
if (intr_handle && intr_handle->type == RTE_INTR_HANDLE_VDEV)
@@ -407,4 +407,5 @@ EXPERIMENTAL {
rte_lcore_to_cpu_id;
rte_mcfg_timer_lock;
rte_mcfg_timer_unlock;
+ rte_intr_ack;
};