From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?iso-8859-1?Q?N=E9lio?= Laranjeiro <nelio.laranjeiro@6wind.com>
Subject: Re: [PATCH v1 1/2] net/mlx5: change eth device reference
 for secondary process
Date: Fri, 25 Aug 2017 09:32:31 +0200
Message-ID: <20170825073231.GT4544@autoinstall.dev.6wind.com>
References: <20170824140341.95471-1-xuemingl@mellanox.com>
 <20170825065213.GR4544@autoinstall.dev.6wind.com>
 <DB3PR05MB29772D4B90F0B5DEEE5EC4AAC9B0@DB3PR05MB297.eurprd05.prod.outlook.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: "Xueming(Steven) Li" <xuemingl@mellanox.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wr0-f173.google.com (mail-wr0-f173.google.com
 [209.85.128.173]) by dpdk.org (Postfix) with ESMTP id E0E947CFC
 for <dev@dpdk.org>; Fri, 25 Aug 2017 09:32:41 +0200 (CEST)
Received: by mail-wr0-f173.google.com with SMTP id k46so472417wre.0
 for <dev@dpdk.org>; Fri, 25 Aug 2017 00:32:41 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <DB3PR05MB29772D4B90F0B5DEEE5EC4AAC9B0@DB3PR05MB297.eurprd05.prod.outlook.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://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: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Fri, Aug 25, 2017 at 07:15:50AM +0000, Xueming(Steven) Li wrote:
> Nelio, thanks, comments inline.
>[...]
> > >  static int
> > > -priv_set_link(struct priv *priv, int up)
> > > +mlx5_dev_set_link(struct rte_eth_dev *dev, int up)
> > >  {
> > > -	struct rte_eth_dev *dev = priv->dev;
> > > +	struct priv *priv = dev->data->dev_private;
> > >  	int err;
> > >
> > 
> > This function should lock/unclock priv.
> This is a static function, caller function do the lock/unlock. 
> Is there naming convention here? Mlx5_* is outpost interfaces that normally
> require lock/unlock priv?

Yes there is a naming convention following the patterns:
 - priv_...(struct *priv priv, ...): no locks inside.
 - priv_dev_...(struct *priv priv, struct rte_eth_dev *dev, ...): no locks
   inside.
 - mlx5_...(struct rte_eth_dev *dev, ...): should lock any access to struct priv
   and to priv_*().

> > >  	if (up) {
> > >  		err = priv_set_flags(priv, ~IFF_UP, IFF_UP);
> > >  		if (err)
> > >  			return err;
> > > -		priv_select_tx_function(priv);
> > > -		priv_select_rx_function(priv);
> > > +		mlx5_dev_select_tx_function(dev);
> > > +		mlx5_dev_select_rx_function(dev);
> > 
> > This also induce that those function mlx5_dev_select_rx/tx_function() should
> > be renamed to:
> >  priv_dev_select_rx/tx_function(struct *priv, struct rte_eth_dev *dev, ...)
> > 
> > this will avoid the multiple lock/unlocks inside the functions.
> So priv_* are lock-free functions?

priv_*() assume the lock have been done by the caller.

Hope it helps.

Thanks,

-- 
Nélio Laranjeiro
6WIND