Fail-safe dev_start() operation can be called by both the application and the hot-plug alarm mechanism. The installation of Rx interrupt are triggered from dev_start() in any time it is called while actually the Rx interrupt should be installed only by the application calls. So, each plug-in event causes reinstallation which causes memory leak. Trigger the Rx interrupt installation only for application calls. Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") Signed-off-by: Matan Azrad <matan@mellanox.com> --- drivers/net/failsafe/failsafe_ops.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c index 057e435..bbbd335 100644 --- a/drivers/net/failsafe/failsafe_ops.c +++ b/drivers/net/failsafe/failsafe_ops.c @@ -181,10 +181,12 @@ int ret; fs_lock(dev, 0); - ret = failsafe_rx_intr_install(dev); - if (ret) { - fs_unlock(dev, 0); - return ret; + if (PRIV(dev)->alarm_lock == 0) { + ret = failsafe_rx_intr_install(dev); + if (ret) { + fs_unlock(dev, 0); + return ret; + } } FOREACH_SUBDEV(sdev, i, dev) { if (sdev->state != DEV_ACTIVE) -- 1.9.5
Hi Matan, On Tue, Feb 13, 2018 at 10:59:32PM +0000, Matan Azrad wrote: > Fail-safe dev_start() operation can be called by both the application > and the hot-plug alarm mechanism. > > The installation of Rx interrupt are triggered from dev_start() in any > time it is called while actually the Rx interrupt should be installed > only by the application calls. > > So, each plug-in event causes reinstallation which causes memory leak. > > Trigger the Rx interrupt installation only for application calls. > > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") > > Signed-off-by: Matan Azrad <matan@mellanox.com> > --- > drivers/net/failsafe/failsafe_ops.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > index 057e435..bbbd335 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -181,10 +181,12 @@ > int ret; > > fs_lock(dev, 0); > - ret = failsafe_rx_intr_install(dev); > - if (ret) { > - fs_unlock(dev, 0); > - return ret; > + if (PRIV(dev)->alarm_lock == 0) { I dislike having to rely on unrelated context of execution to decide a code-path. I'd prefer to make interrupt installation dependent on the interrupt state instead. I think it should be possible to forbid reinstallation within failsafe_rx_intr_install directly, e.g. diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c index f6ff04dc8..46c3aa5f2 100644 --- a/drivers/net/failsafe/failsafe_intr.c +++ b/drivers/net/failsafe/failsafe_intr.c @@ -523,7 +523,8 @@ failsafe_rx_intr_install(struct rte_eth_dev *dev) const struct rte_intr_conf *const intr_conf = &priv->dev->data->dev_conf.intr_conf; - if (intr_conf->rxq == 0) + if (intr_conf->rxq == 0 || + dev->intr_handle != NULL) return 0; if (fs_rx_intr_vec_install(priv) < 0) return -rte_errno; This way the logic is self-dependent and the check limited to this component. There might be better way to do this, it's only an example to explain my point. > + ret = failsafe_rx_intr_install(dev); > + if (ret) { > + fs_unlock(dev, 0); > + return ret; > + } > } > FOREACH_SUBDEV(sdev, i, dev) { > if (sdev->state != DEV_ACTIVE) > -- > 1.9.5 > -- Gaëtan Rivet 6WIND
Hi Gaetan
Agree, will send V2.
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, February 14, 2018 3:52 PM
> To: Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/failsafe: fix Rx interrupt reinstallation
>
> Hi Matan,
>
> On Tue, Feb 13, 2018 at 10:59:32PM +0000, Matan Azrad wrote:
> > Fail-safe dev_start() operation can be called by both the application
> > and the hot-plug alarm mechanism.
> >
> > The installation of Rx interrupt are triggered from dev_start() in any
> > time it is called while actually the Rx interrupt should be installed
> > only by the application calls.
> >
> > So, each plug-in event causes reinstallation which causes memory leak.
> >
> > Trigger the Rx interrupt installation only for application calls.
> >
> > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode")
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> > drivers/net/failsafe/failsafe_ops.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/failsafe/failsafe_ops.c
> > b/drivers/net/failsafe/failsafe_ops.c
> > index 057e435..bbbd335 100644
> > --- a/drivers/net/failsafe/failsafe_ops.c
> > +++ b/drivers/net/failsafe/failsafe_ops.c
> > @@ -181,10 +181,12 @@
> > int ret;
> >
> > fs_lock(dev, 0);
> > - ret = failsafe_rx_intr_install(dev);
> > - if (ret) {
> > - fs_unlock(dev, 0);
> > - return ret;
> > + if (PRIV(dev)->alarm_lock == 0) {
>
> I dislike having to rely on unrelated context of execution to decide a code-
> path.
>
> I'd prefer to make interrupt installation dependent on the interrupt state
> instead.
>
> I think it should be possible to forbid reinstallation within
> failsafe_rx_intr_install directly, e.g.
>
> diff --git a/drivers/net/failsafe/failsafe_intr.c
> b/drivers/net/failsafe/failsafe_intr.c
> index f6ff04dc8..46c3aa5f2 100644
> --- a/drivers/net/failsafe/failsafe_intr.c
> +++ b/drivers/net/failsafe/failsafe_intr.c
> @@ -523,7 +523,8 @@ failsafe_rx_intr_install(struct rte_eth_dev *dev)
> const struct rte_intr_conf *const intr_conf =
> &priv->dev->data->dev_conf.intr_conf;
>
> - if (intr_conf->rxq == 0)
> + if (intr_conf->rxq == 0 ||
> + dev->intr_handle != NULL)
> return 0;
> if (fs_rx_intr_vec_install(priv) < 0)
> return -rte_errno;
>
> This way the logic is self-dependent and the check limited to this component.
>
> There might be better way to do this, it's only an example to explain my
> point.
>
> > + ret = failsafe_rx_intr_install(dev);
> > + if (ret) {
> > + fs_unlock(dev, 0);
> > + return ret;
> > + }
> > }
> > FOREACH_SUBDEV(sdev, i, dev) {
> > if (sdev->state != DEV_ACTIVE)
> > --
> > 1.9.5
> >
>
> --
> Gaëtan Rivet
> 6WIND
Fail-safe dev_start() operation can be called by both the application and the hot-plug alarm mechanism. The installation of Rx interrupt are triggered from dev_start() in any time it is called while actually the Rx interrupt should be installed only by the application calls. So, each plug-in event causes reinstallation which causes memory leak and spoils the fail-safe Rx interrupt mechanism. Trigger the Rx interrupt installation only when it does not exist. Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") Signed-off-by: Matan Azrad <matan@mellanox.com> --- drivers/net/failsafe/failsafe_intr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c index f6ff04d..6b7f9c1 100644 --- a/drivers/net/failsafe/failsafe_intr.c +++ b/drivers/net/failsafe/failsafe_intr.c @@ -523,7 +523,7 @@ void failsafe_rx_intr_uninstall_subdevice(struct sub_device *sdev) const struct rte_intr_conf *const intr_conf = &priv->dev->data->dev_conf.intr_conf; - if (intr_conf->rxq == 0) + if (intr_conf->rxq == 0 || dev->intr_handle != NULL) return 0; if (fs_rx_intr_vec_install(priv) < 0) return -rte_errno; -- 1.9.5
On Wed, Feb 14, 2018 at 02:47:26PM +0000, Matan Azrad wrote:
> Fail-safe dev_start() operation can be called by both the application
> and the hot-plug alarm mechanism.
>
> The installation of Rx interrupt are triggered from dev_start() in any
> time it is called while actually the Rx interrupt should be installed
> only by the application calls.
>
> So, each plug-in event causes reinstallation which causes memory leak
> and spoils the fail-safe Rx interrupt mechanism.
>
> Trigger the Rx interrupt installation only when it does not exist.
>
> Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode")
>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
--
Gaëtan Rivet
6WIND
On Wed, Feb 14, 2018 at 04:00:13PM +0100, Gaëtan Rivet wrote:
> On Wed, Feb 14, 2018 at 02:47:26PM +0000, Matan Azrad wrote:
> > Fail-safe dev_start() operation can be called by both the application
> > and the hot-plug alarm mechanism.
> >
> > The installation of Rx interrupt are triggered from dev_start() in any
> > time it is called while actually the Rx interrupt should be installed
> > only by the application calls.
> >
> > So, each plug-in event causes reinstallation which causes memory leak
> > and spoils the fail-safe Rx interrupt mechanism.
> >
> > Trigger the Rx interrupt installation only when it does not exist.
> >
> > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode")
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
Actually no!
There is a mistake in the patch, you disabled the uninstall, instead of
the installation.
--
Gaëtan Rivet
6WIND
Hi Gaetan From: Gaëtan Rivet, Sent: Wednesday, February 14, 2018 5:01 PM > On Wed, Feb 14, 2018 at 04:00:13PM +0100, Gaëtan Rivet wrote: > > On Wed, Feb 14, 2018 at 02:47:26PM +0000, Matan Azrad wrote: > > > Fail-safe dev_start() operation can be called by both the > > > application and the hot-plug alarm mechanism. > > > > > > The installation of Rx interrupt are triggered from dev_start() in > > > any time it is called while actually the Rx interrupt should be > > > installed only by the application calls. > > > > > > So, each plug-in event causes reinstallation which causes memory > > > leak and spoils the fail-safe Rx interrupt mechanism. > > > > > > Trigger the Rx interrupt installation only when it does not exist. > > > > > > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") > > > > > > Signed-off-by: Matan Azrad <matan@mellanox.com> > > > > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com> > > Actually no! > > There is a mistake in the patch, you disabled the uninstall, instead of the > installation. > No Gaetan, I think it is in the install. Please recheck maybe by applying. > -- > Gaëtan Rivet > 6WIND
On Wed, Feb 14, 2018 at 04:01:29PM +0100, Gaëtan Rivet wrote: > On Wed, Feb 14, 2018 at 04:00:13PM +0100, Gaëtan Rivet wrote: > > On Wed, Feb 14, 2018 at 02:47:26PM +0000, Matan Azrad wrote: > > > Fail-safe dev_start() operation can be called by both the application > > > and the hot-plug alarm mechanism. > > > > > > The installation of Rx interrupt are triggered from dev_start() in any > > > time it is called while actually the Rx interrupt should be installed > > > only by the application calls. > > > > > > So, each plug-in event causes reinstallation which causes memory leak > > > and spoils the fail-safe Rx interrupt mechanism. > > > > > > Trigger the Rx interrupt installation only when it does not exist. > > > > > > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") > > > > > > Signed-off-by: Matan Azrad <matan@mellanox.com> > > > > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com> > > Actually no! > > There is a mistake in the patch, you disabled the uninstall, instead of > the installation. Okay, this is weird. > > > Fail-safe dev_start() operation can be called by both the application > > > and the hot-plug alarm mechanism. > > > > > > The installation of Rx interrupt are triggered from dev_start() in any > > > time it is called while actually the Rx interrupt should be installed > > > only by the application calls. > > > > > > So, each plug-in event causes reinstallation which causes memory leak > > > and spoils the fail-safe Rx interrupt mechanism. > > > > > > Trigger the Rx interrupt installation only when it does not exist. > > > > > > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode") > > > > > > Signed-off-by: Matan Azrad <matan@mellanox.com> > > > --- > > > drivers/net/failsafe/failsafe_intr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/failsafe/failsafe_intr.c b/drivers/net/failsafe/failsafe_intr.c > > > index f6ff04d..6b7f9c1 100644 > > > --- a/drivers/net/failsafe/failsafe_intr.c > > > +++ b/drivers/net/failsafe/failsafe_intr.c > > > @@ -523,7 +523,7 @@ void failsafe_rx_intr_uninstall_subdevice(struct sub_device *sdev) Here the context is incorrect, this is not within failsafe_rx_intr_uninstall_subdevice, so the fix is correct. Confirming my ack then, this seems like a format-patch bug or something. > > > const struct rte_intr_conf *const intr_conf = > > > &priv->dev->data->dev_conf.intr_conf; > > > > > > - if (intr_conf->rxq == 0) > > > + if (intr_conf->rxq == 0 || dev->intr_handle != NULL) > > > return 0; > > > if (fs_rx_intr_vec_install(priv) < 0) > > > return -rte_errno; > > > -- > > > 1.9.5 -- Gaëtan Rivet 6WIND
14/02/2018 16:00, Gaëtan Rivet:
> On Wed, Feb 14, 2018 at 02:47:26PM +0000, Matan Azrad wrote:
> > Fail-safe dev_start() operation can be called by both the application
> > and the hot-plug alarm mechanism.
> >
> > The installation of Rx interrupt are triggered from dev_start() in any
> > time it is called while actually the Rx interrupt should be installed
> > only by the application calls.
> >
> > So, each plug-in event causes reinstallation which causes memory leak
> > and spoils the fail-safe Rx interrupt mechanism.
> >
> > Trigger the Rx interrupt installation only when it does not exist.
> >
> > Fixes: 9e0360aebf23 ("net/failsafe: register as Rx interrupt mode")
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
Applied, thanks