DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>, dev@dpdk.org
Cc: kumaras@chelsio.com, surendra@chelsio.com, nirranjan@chelsio.com,
	indranil@chelsio.com
Subject: Re: [dpdk-dev] [PATCH 1/7] cxgbe: fix secondary process initialization
Date: Mon, 5 Feb 2018 17:23:36 +0000	[thread overview]
Message-ID: <58e0e745-bdba-e5a5-98a0-68bc1698e3c2@intel.com> (raw)
In-Reply-To: <86269c17d76550025a5eb1a60202c7052f8e6ddf.1517685185.git.rahul.lakkireddy@chelsio.com>

On 2/4/2018 6:06 AM, Rahul Lakkireddy wrote:
> From: Kumar Sanghvi <kumaras@chelsio.com>
> 
> rte_eth_dev_allocate already assigns the eth_dev_data. So,
> don't allocate it separately as part of probe function. And we
> save this eth_dev_data as part of txq structure.
> 
> Attach to rte_eth_dev devices allocated by Primary process for
> Ports other than Port-0 in the secondary process.
> 
> Fixes: 8318984927ff ("cxgbe: add pmd skeleton")
> Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> ---
>  doc/guides/nics/features/cxgbe.ini |  1 +
>  drivers/net/cxgbe/base/adapter.h   |  1 +
>  drivers/net/cxgbe/cxgbe_ethdev.c   | 28 +++++++++++++++++++++++-----
>  drivers/net/cxgbe/cxgbe_main.c     | 11 ++---------
>  drivers/net/cxgbe/sge.c            |  5 +++--
>  5 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/doc/guides/nics/features/cxgbe.ini b/doc/guides/nics/features/cxgbe.ini
> index 3d0fde2fd..1b9d81ffb 100644
> --- a/doc/guides/nics/features/cxgbe.ini
> +++ b/doc/guides/nics/features/cxgbe.ini
> @@ -24,6 +24,7 @@ Basic stats          = Y
>  Stats per queue      = Y
>  EEPROM dump          = Y
>  Registers dump       = Y
> +Multiprocess aware   = Y
>  BSD nic_uio          = Y
>  Linux UIO            = Y
>  Linux VFIO           = Y
> diff --git a/drivers/net/cxgbe/base/adapter.h b/drivers/net/cxgbe/base/adapter.h
> index f2057af14..0c1a1be25 100644
> --- a/drivers/net/cxgbe/base/adapter.h
> +++ b/drivers/net/cxgbe/base/adapter.h
> @@ -267,6 +267,7 @@ struct sge_eth_tx_stats {	/* Ethernet tx queue statistics */
>  struct sge_eth_txq {                   /* state for an SGE Ethernet Tx queue */
>  	struct sge_txq q;
>  	struct rte_eth_dev *eth_dev;   /* port that this queue belongs to */
> +	struct rte_eth_dev_data *data;

Why you need to store a local copy of data? You have to store/know eth_dev, and
I believe it is always better idea to access data through eth_dev. Perhaps we
even should hide this access with macro etc. Not sure about motivation of this.

>  	struct sge_eth_tx_stats stats; /* queue statistics */
>  	rte_spinlock_t txq_lock;
>  
> diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
> index 5cd260f48..6d56f3c1b 100644
> --- a/drivers/net/cxgbe/cxgbe_ethdev.c
> +++ b/drivers/net/cxgbe/cxgbe_ethdev.c
> @@ -1004,14 +1004,32 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
>  	eth_dev->dev_ops = &cxgbe_eth_dev_ops;
>  	eth_dev->rx_pkt_burst = &cxgbe_recv_pkts;
>  	eth_dev->tx_pkt_burst = &cxgbe_xmit_pkts;
> +	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>  
> -	/* for secondary processes, we don't initialise any further as primary
> -	 * has already done this work.
> +	/* for secondary processes, we attach to ethdevs allocated by primary
> +	 * and do minimal initialization.
>  	 */
> -	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +		int i;
> +
> +		for (i = 1; i < MAX_NPORTS; i++) {
> +			struct rte_eth_dev *rest_eth_dev;
> +			char namei[RTE_ETH_NAME_MAX_LEN];
> +
> +			snprintf(namei, sizeof(namei), "cxgbe%d", i);
> +			rest_eth_dev = rte_eth_dev_attach_secondary(namei);
> +			if (rest_eth_dev) {
> +				rest_eth_dev->device = &pci_dev->device;
> +				rest_eth_dev->dev_ops =
> +					eth_dev->dev_ops;
> +				rest_eth_dev->rx_pkt_burst =
> +					eth_dev->rx_pkt_burst;
> +				rest_eth_dev->tx_pkt_burst =
> +					eth_dev->tx_pkt_burst;
> +			}
> +		}

You shouldn't need this update, old version seems better.

If a secondary process is running call stack is like:
eth_cxgbe_pci_probe
  rte_eth_dev_pci_generic_probe
    rte_eth_dev_pci_allocate
      rte_eth_dev_attach_secondary
    eth_cxgbe_dev_init

rte_eth_dev_attach_secondary() already called in the patch, all need to be done
in eth_cxgbe_dev_init() which already does.

Unless I am missing something this update shouldn't happen.


>  		return 0;
> -
> -	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> +	}
>  
>  	snprintf(name, sizeof(name), "cxgbeadapter%d", eth_dev->data->port_id);
>  	adapter = rte_zmalloc(name, sizeof(*adapter), 0);
> diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
> index 28db6c061..c502fadf7 100644
> --- a/drivers/net/cxgbe/cxgbe_main.c
> +++ b/drivers/net/cxgbe/cxgbe_main.c
> @@ -1266,8 +1266,6 @@ int cxgbe_probe(struct adapter *adapter)
>  
>  	for_each_port(adapter, i) {
>  		char name[RTE_ETH_NAME_MAX_LEN];
> -		struct rte_eth_dev_data *data = NULL;
> -		const unsigned int numa_node = rte_socket_id();
>  
>  		pi = &adapter->port[i];
>  		pi->adapter = adapter;
> @@ -1293,13 +1291,8 @@ int cxgbe_probe(struct adapter *adapter)
>  		if (!pi->eth_dev)
>  			goto out_free;
>  
> -		data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);
> -		if (!data)
> -			goto out_free;
> -
> -		data->port_id = adapter->eth_dev->data->port_id + i;
> -
> -		pi->eth_dev->data = data;

+1 to remove allocating "data" which is already comes with eth_dev.

We allocate them for virtual PMDs because they are local to the process, not
shared between DPDK processes.

Overall I am not sure about cxgbe_probe(), which has been called by
eth_cxgbe_dev_init() and eth_cxgbe_dev_init() already knows the allocated
eth_dev, but cxgbe_probe() ignores it and allocates a new one, I am not able to
understand why.

> +		pi->eth_dev->data->port_id =
> +			adapter->eth_dev->data->port_id + i;

And this one, why updating internal port_id value, this looks wrong, how
multi-process works with this and if there is another nic in the system now
port_id is no more unique.

Perhaps you have a unique hardware/use case, it helps a lot if you can describe
it a little.

>  
>  allocate_mac:
>  		pi->eth_dev->device = &adapter->pdev->device;
> diff --git a/drivers/net/cxgbe/sge.c b/drivers/net/cxgbe/sge.c
> index 3d5aa596f..6ff8bc46b 100644
> --- a/drivers/net/cxgbe/sge.c
> +++ b/drivers/net/cxgbe/sge.c
> @@ -1095,7 +1095,7 @@ int t4_eth_xmit(struct sge_eth_txq *txq, struct rte_mbuf *mbuf,
>  	u32 wr_mid;
>  	u64 cntrl, *end;
>  	bool v6;
> -	u32 max_pkt_len = txq->eth_dev->data->dev_conf.rxmode.max_rx_pkt_len;
> +	u32 max_pkt_len = txq->data->dev_conf.rxmode.max_rx_pkt_len;
>  
>  	/* Reject xmit if queue is stopped */
>  	if (unlikely(txq->flags & EQ_STOPPED))
> @@ -1115,7 +1115,7 @@ int t4_eth_xmit(struct sge_eth_txq *txq, struct rte_mbuf *mbuf,
>  	    (unlikely(m->pkt_len > max_pkt_len)))
>  		goto out_free;
>  
> -	pi = (struct port_info *)txq->eth_dev->data->dev_private;
> +	pi = (struct port_info *)txq->data->dev_private;
>  	adap = pi->adapter;
>  
>  	cntrl = F_TXPKT_L4CSUM_DIS | F_TXPKT_IPCSUM_DIS;
> @@ -1997,6 +1997,7 @@ int t4_sge_alloc_eth_txq(struct adapter *adap, struct sge_eth_txq *txq,
>  	txq->stats.mapping_err = 0;
>  	txq->flags |= EQ_STOPPED;
>  	txq->eth_dev = eth_dev;
> +	txq->data = eth_dev->data;
>  	t4_os_lock_init(&txq->txq_lock);
>  	return 0;
>  }
> 

  reply	other threads:[~2018-02-05 17:23 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-04  6:06 [dpdk-dev] [PATCH 0/7] cxgbe: bug fixes and updates Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 1/7] cxgbe: fix secondary process initialization Rahul Lakkireddy
2018-02-05 17:23   ` Ferruh Yigit [this message]
2018-02-06  9:20     ` Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 2/7] cxgbe: update link state when link speed changes Rahul Lakkireddy
2018-02-05 17:05   ` Ferruh Yigit
2018-02-04  6:06 ` [dpdk-dev] [PATCH 3/7] cxgbe: add support to update RSS hash configuration and key Rahul Lakkireddy
2018-02-05 17:09   ` Ferruh Yigit
2018-02-06  9:22     ` Rahul Lakkireddy
2018-02-06 10:11       ` Ferruh Yigit
2018-02-06 10:38         ` Thomas Monjalon
2018-02-07  7:01           ` Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 4/7] cxgbe: add support to get programmed " Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 5/7] cxgbe: update link Forward Error Correction (FEC) Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 6/7] cxgbe: update link configuration for 32-bit port capability Rahul Lakkireddy
2018-02-04  6:06 ` [dpdk-dev] [PATCH 7/7] cxgbe: rework and use " Rahul Lakkireddy
2018-02-28 18:04 ` [dpdk-dev] [PATCH v2 0/7] cxgbe: bug fixes and updates Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 1/7] cxgbe: rework rte_eth_dev allocation Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 2/7] cxgbe: fix secondary process initialization Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 3/7] cxgbe: add support to update RSS hash configuration and key Rahul Lakkireddy
2018-03-08 13:34     ` Ferruh Yigit
2018-03-10  5:21       ` Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 4/7] cxgbe: add support to get programmed " Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 5/7] cxgbe: update link Forward Error Correction (FEC) Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 6/7] cxgbe: update link configuration for 32-bit port capability Rahul Lakkireddy
2018-02-28 18:04   ` [dpdk-dev] [PATCH v2 7/7] cxgbe: rework and use " Rahul Lakkireddy
2018-03-08 13:44   ` [dpdk-dev] [PATCH v2 0/7] cxgbe: bug fixes and updates Ferruh Yigit
2018-03-08 13:50     ` Ferruh Yigit
2018-03-10  5:24       ` Rahul Lakkireddy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=58e0e745-bdba-e5a5-98a0-68bc1698e3c2@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=indranil@chelsio.com \
    --cc=kumaras@chelsio.com \
    --cc=nirranjan@chelsio.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=surendra@chelsio.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).