From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gaetan.rivet@6wind.com>
Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com
 [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id 522691B21E
 for <dev@dpdk.org>; Wed, 14 Feb 2018 14:52:42 +0100 (CET)
Received: by mail-wr0-f195.google.com with SMTP id k32so16758wrk.4
 for <dev@dpdk.org>; Wed, 14 Feb 2018 05:52:42 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=6wind-com.20150623.gappssmtp.com; s=20150623;
 h=date:from:to:cc:subject:message-id:references:mime-version
 :content-disposition:content-transfer-encoding:in-reply-to
 :user-agent; bh=xZ7+92GzMCasCKXel5IIGs9vJG7z38i2k6FKuKBtMEw=;
 b=Xvy45XE1+ftsXOoPSoLF82cUPY/8MvV+xHSjyQ2jGfoc5tQwtZ87vhW/53Q5hKnpyc
 XyIVgom7Lftq0oxv7AbxWCVK4qh93U6ow+acwSG8vaUPMuAfHlKSpA4mzAMrNjSpn/Jo
 H736NfFkSPLYO7AZZdtnkGfkgWNLOtndl59uD5lmjWRiBPK8Wi2dQgzyg7qVveapsUli
 4qTGVyq0f1IM5iotLhH2pUEPji59WXrr2pbmOVHr25EzPGylLbVtcRs5jZ2w0W7AhPFb
 OF6WpxrJw4xqaAHONEEhFKij2ejE32TuiAEtz275gUBVCmXVvPG1sU7Y9yRR80Vgxabn
 0QGw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20161025;
 h=x-gm-message-state:date:from:to:cc:subject:message-id:references
 :mime-version:content-disposition:content-transfer-encoding
 :in-reply-to:user-agent;
 bh=xZ7+92GzMCasCKXel5IIGs9vJG7z38i2k6FKuKBtMEw=;
 b=JwHbNvFXPot24LulKUqAM9niYh85qIRSeMAf8ZOOMBmN7tzX/md89Pbx6AjNlZG+A1
 bNitmt95OFcdf3twgGHFunankt5pn4fD7nnTQYJGf53+DBUK8ZDY/OgE8tB87ZGMuAEa
 Bn3AFAKP5R0wqDwVHidinKzw02rOatCLczfb/w8a9N00/YsJpI5syu2I3egeNpKblZX1
 wZjfn7/jHWHeABsoaKHmyYBC5+uD4IYVxj0210MHiLBI0aHcRCpEbPCrWEPMJ5MMvkJT
 v31DTXg/FakE8yK9CoVN/i7s1CNOURdhvH7GNiOsGv5SLoqHgzqf+AfrvHEQSPjeALN4
 wS1g==
X-Gm-Message-State: APf1xPBR6UsHSsHG9wsChBKHYszS5YKopqAxS9UDX7uCMr4S/e1x0gmQ
 qImcB0d2jKl8RKwQ5cZrGG5G6kRX
X-Google-Smtp-Source: AH8x226LS++/b1hM5+HwUQzhjsqNFV2L4Kl/7Z6bht4jK7J/rBokUt4SAdq/XWL+00v1tgnPeZwxsA==
X-Received: by 10.223.199.12 with SMTP id k12mr4777797wrg.261.1518616361723;
 Wed, 14 Feb 2018 05:52:41 -0800 (PST)
Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com.
 [62.23.145.78])
 by smtp.gmail.com with ESMTPSA id i11sm7021958wre.36.2018.02.14.05.52.40
 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);
 Wed, 14 Feb 2018 05:52:40 -0800 (PST)
Date: Wed, 14 Feb 2018 14:52:27 +0100
From: =?iso-8859-1?Q?Ga=EBtan?= Rivet <gaetan.rivet@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: dev@dpdk.org
Message-ID: <20180214135227.o7lddtw5jmnnec7u@bidouze.vm.6wind.com>
References: <1518562772-29823-1-git-send-email-matan@mellanox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1518562772-29823-1-git-send-email-matan@mellanox.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-dev] [PATCH] net/failsafe: fix Rx interrupt reinstallation
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 14 Feb 2018 13:52:42 -0000

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