Re: [PATCH] net/failsafe: fix interrupt handle leak

[David Marchand <david.marchand@redhat.com>]

On Fri, Apr 29, 2022 at 3:56 PM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>
> On 3/24/2022 3:09 PM, David Marchand wrote:
> > A intr_handle is being allocated as a hack to get a (proxy) eventfd from
> > the Linux interrupt implementation.
> > But this handle is never freed.
> >
> > Remove this convoluted hack and create an eventfd in Linux case.
> >
> > Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >   drivers/net/failsafe/failsafe_ops.c | 32 ++++++++++-------------------
> >   1 file changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> > index 55e21d635c..2c23d0e70a 100644
> > --- a/drivers/net/failsafe/failsafe_ops.c
> > +++ b/drivers/net/failsafe/failsafe_ops.c
> > @@ -6,6 +6,9 @@
> >   #include <stdbool.h>
> >   #include <stdint.h>
> >   #include <unistd.h>
> > +#ifdef RTE_EXEC_ENV_LINUX
> > +#include <sys/eventfd.h>
> > +#endif
> >
> >   #include <rte_debug.h>
> >   #include <rte_atomic.h>
> > @@ -387,28 +390,11 @@ fs_rx_queue_setup(struct rte_eth_dev *dev,
> >               const struct rte_eth_rxconf *rx_conf,
> >               struct rte_mempool *mb_pool)
> >   {
> > -     /*
> > -      * FIXME: Add a proper interface in rte_eal_interrupts for
> > -      * allocating eventfd as an interrupt vector.
> > -      * For the time being, fake as if we are using MSIX interrupts,
> > -      * this will cause rte_intr_efd_enable to allocate an eventfd for us.
> > -      */
> > -     struct rte_intr_handle *intr_handle;
> >       struct sub_device *sdev;
> >       struct rxq *rxq;
> >       uint8_t i;
> >       int ret;
> >
> > -     intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
> > -     if (intr_handle == NULL)
> > -             return -ENOMEM;
> > -
> > -     if (rte_intr_type_set(intr_handle, RTE_INTR_HANDLE_VFIO_MSIX))
> > -             return -rte_errno;
> > -
> > -     if (rte_intr_efds_index_set(intr_handle, 0, -1))
> > -             return -rte_errno;
> > -
> >       fs_lock(dev, 0);
> >       if (rx_conf->rx_deferred_start) {
> >               FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> > @@ -442,12 +428,16 @@ fs_rx_queue_setup(struct rte_eth_dev *dev,
> >       rxq->info.nb_desc = nb_rx_desc;
> >       rxq->priv = PRIV(dev);
> >       rxq->sdev = PRIV(dev)->subs;
> > -     ret = rte_intr_efd_enable(intr_handle, 1);
> > -     if (ret < 0) {
> > +#ifdef RTE_EXEC_ENV_LINUX
> > +     rxq->event_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> > +     if (rxq->event_fd < 0) {
> > +             ERROR("Failed to create an eventfd: %s", strerror(errno));
> >               fs_unlock(dev, 0);
> > -             return ret;
> > +             return -errno;
> >       }
> > -     rxq->event_fd = rte_intr_efds_index_get(intr_handle, 0);
> > +#else
> > +     rxq->event_fd = -1;
> > +#endif
>
> How this impacts the BSD? I don't know if driver used on BSD but
> technically it looks supported.
>
> @Gaetan, any objection to the change?

There was no feedback for months, can we get this merged?
Thanks.


-- 
David Marchand