DPDK patches and discussions
 help / color / mirror / Atom feed
From: Yongseok Koh <yskoh@mellanox.com>
To: Shahaf Shuler <shahafs@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 3/4] net/mlx5: remove device register remap
Date: Tue, 9 Apr 2019 19:36:51 +0000	[thread overview]
Message-ID: <0731AA80-EA07-4F93-B5DA-35FD442F0FDC@mellanox.com> (raw)
Message-ID: <20190409193651.mp3qOWpWPj4UQCtJnrgNLaL1mHkZcA4cgMvJVMbiEhc@z> (raw)
In-Reply-To: <AM0PR0502MB379559A3B4BB7509C74ACE9AC32C0@AM0PR0502MB3795.eurprd05.prod.outlook.com>


> On Apr 7, 2019, at 10:48 PM, Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> Hi Koh,
> 
> See small comments below. Same for mlx4 patch.
> 
> 
> Friday, April 5, 2019 4:34 AM, Yongseok Koh:
>> Subject: [dpdk-dev] [PATCH v3 3/4] net/mlx5: remove device register remap
>> 
>> UAR (User Access Region) register does not need to be remapped for
>> primary process but it should be remapped only for secondary process. UAR
>> register table is in the process private structure in rte_eth_devices[],
>> 	(struct mlx5_proc_priv *)rte_eth_devices[port_id].process_private
>> 
>> The actual UAR table follows the data structure and the table is used for both
>> Tx and Rx.
>> 
>> For Tx, BlueFlame in UAR is used to ring the doorbell. MLX5_TX_BFREG(txq)
>> is defined to get a register for the txq. Processes access its own private data
>> to acquire the register from the UAR table.
>> 
>> For Rx, the doorbell in UAR is required in arming CQ event. However, it is a
>> known issue that the register isn't remapped for secondary process.
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5.c         | 198 +++++-----------------------------------
>> drivers/net/mlx5/mlx5.h         |  15 ++-
>> drivers/net/mlx5/mlx5_ethdev.c  |  17 ++++
>> drivers/net/mlx5/mlx5_rxtx.h    |  11 ++-
>> drivers/net/mlx5/mlx5_trigger.c |   6 --
>> drivers/net/mlx5/mlx5_txq.c     | 180 ++++++++++++++++++++++-------------
>> -
> 
> [...]
> 
>> /**
>> @@ -1182,12 +1010,32 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>> 	}
>> 	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
>> 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>> +		struct mlx5_proc_priv *ppriv;
>> +		size_t ppriv_size;
>> +
>> 		eth_dev = rte_eth_dev_attach_secondary(name);
>> 		if (eth_dev == NULL) {
>> 			DRV_LOG(ERR, "can not attach rte ethdev");
>> 			rte_errno = ENOMEM;
>> 			return NULL;
>> 		}
>> +		priv = eth_dev->data->dev_private;
>> +		/*
>> +		 * UAR register table follows the process private structure.
>> +		 * BlueFlame registers for Tx queues come first and registers
>> +		 * for Rx queues follows.
>> +		 */
>> +		ppriv_size = sizeof(struct mlx5_proc_priv) +
>> +			     (priv->rxqs_n + priv->txqs_n) * sizeof(void *);
> 
> Why you add also the rxqs_n? why not only the txqs?

I wanted to make a room for the registers for arming Rx CQ, which will be fixed
soon. But, I think it would be better to avoid confusion in this patch. Will remove.

[...]
>> +	ppriv = rte_malloc_socket("mlx5_proc_priv", ppriv_size,
>> +				  RTE_CACHE_LINE_SIZE, dev->device-
>>> numa_node);
>> +	if (!ppriv) {
>> +		rte_errno = ENOMEM;
>> +		return -rte_errno;
>> +	}
>> +	ppriv->uar_table_sz = ppriv_size;
>> +	dev->process_private = ppriv;
>> 	return 0;
>> }
>> 
>> diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
>> index 7b58063ceb..5d49892429 100644
>> --- a/drivers/net/mlx5/mlx5_rxtx.h
>> +++ b/drivers/net/mlx5/mlx5_rxtx.h
>> @@ -201,8 +201,8 @@ struct mlx5_txq_data {
>> 	volatile void *wqes; /* Work queue (use volatile to write into). */
>> 	volatile uint32_t *qp_db; /* Work queue doorbell. */
>> 	volatile uint32_t *cq_db; /* Completion queue doorbell. */
>> -	volatile void *bf_reg; /* Blueflame register remapped. */
>> 	struct rte_mbuf *(*elts)[]; /* TX elements. */
>> +	uint16_t port_id; /* Port ID of device. */
>> 	uint16_t idx; /* Queue index. */
>> 	struct mlx5_txq_stats stats; /* TX queue counters. */  #ifndef
>> RTE_ARCH_64 @@ -231,9 +231,12 @@ struct mlx5_txq_ctrl {
>> 	struct mlx5_txq_ibv *ibv; /* Verbs queue object. */
>> 	struct mlx5_priv *priv; /* Back pointer to private data. */
>> 	off_t uar_mmap_offset; /* UAR mmap offset for non-primary
>> process. */
>> -	volatile void *bf_reg_orig; /* Blueflame register from verbs. */
>> +	void *bf_reg; /* BlueFlame register from Verbs. */
> 
> I guess you keep this one in order to get the VA offset for the secondary mapping, right? Because otherwise we can take the bf_reg from the UAR table on the process private.
> 
> If so, better to rename it to uar_page_offset (or other name you like) in order to avoid fields duplication. 

It doesn't have the offset (offset can be calculated when remapping).
It is the original BF register address gotten from QP creation.

>> };
>> 
>> +#define MLX5_TX_BFREG(txq) \
>> +		(MLX5_PROC_PRIV((txq)->port_id)->uar_table[(txq)->idx])
>> +
>> /* mlx5_rxq.c */
>> 
>> extern uint8_t rss_hash_default_key[];
>> @@ -301,7 +304,7 @@ uint64_t mlx5_get_rx_queue_offloads(struct
>> rte_eth_dev *dev);  int mlx5_tx_queue_setup(struct rte_eth_dev *dev,
>> uint16_t idx, uint16_t desc,
>> 			unsigned int socket, const struct rte_eth_txconf
>> *conf);  void mlx5_tx_queue_release(void *dpdk_txq); -int
>> mlx5_tx_uar_remap(struct rte_eth_dev *dev, int fd);
>> +int mlx5_tx_uar_init_secondary(struct rte_eth_dev *dev, int fd);
>> struct mlx5_txq_ibv *mlx5_txq_ibv_new(struct rte_eth_dev *dev, uint16_t
>> idx);  struct mlx5_txq_ibv *mlx5_txq_ibv_get(struct rte_eth_dev *dev,
>> uint16_t idx);  int mlx5_txq_ibv_release(struct mlx5_txq_ibv *txq_ibv); @@
>> -704,7 +707,7 @@ static __rte_always_inline void
>> mlx5_tx_dbrec_cond_wmb(struct mlx5_txq_data *txq, volatile struct
>> mlx5_wqe *wqe,
>> 		       int cond)
>> {
>> -	uint64_t *dst = (uint64_t *)((uintptr_t)txq->bf_reg);
>> +	uint64_t *dst = MLX5_TX_BFREG(txq);
> 
> I guess no perf penalty due to this change right?

I've confirmed no perf drop on x86 and ARM (BlueField).
It might have been smaller than the jitter even if any.

> Would you consider to prefetch the addr before the db logic just to be on the safe side?

As it is not a random/sequential access but accessing the same cacheline
repeatedly, I wouldn't have a prefetch here. Sometimes, prefetch have a little cost.

Will send out a new version just in case you agree and want to merge it.
If you still want to change something, please comment on the new one.


Thanks,
Yongseok

  parent reply	other threads:[~2019-04-09 19:36 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-25 19:36 [dpdk-dev] [PATCH 0/3] net/mlx: " Yongseok Koh
2019-03-25 19:36 ` Yongseok Koh
2019-03-25 19:36 ` [dpdk-dev] [PATCH 1/3] net/mlx5: fix recursive inclusion of header file Yongseok Koh
2019-03-25 19:36   ` Yongseok Koh
2019-03-25 19:36 ` [dpdk-dev] [PATCH 2/3] net/mlx5: remove device register remap Yongseok Koh
2019-03-25 19:36   ` Yongseok Koh
2019-03-25 19:36 ` [dpdk-dev] [PATCH 3/3] net/mlx4: " Yongseok Koh
2019-03-25 19:36   ` Yongseok Koh
2019-04-01 21:22 ` [dpdk-dev] [PATCH v2 0/3] net/mlx: " Yongseok Koh
2019-04-01 21:22   ` Yongseok Koh
2019-04-01 21:22   ` [dpdk-dev] [PATCH v2 1/3] net/mlx5: fix recursive inclusion of header file Yongseok Koh
2019-04-01 21:22     ` Yongseok Koh
2019-04-02  5:39     ` Shahaf Shuler
2019-04-02  5:39       ` Shahaf Shuler
2019-04-01 21:22   ` [dpdk-dev] [PATCH v2 2/3] net/mlx5: remove device register remap Yongseok Koh
2019-04-01 21:22     ` Yongseok Koh
2019-04-02  6:50     ` Shahaf Shuler
2019-04-02  6:50       ` Shahaf Shuler
2019-04-01 21:22   ` [dpdk-dev] [PATCH v2 3/3] net/mlx4: " Yongseok Koh
2019-04-01 21:22     ` Yongseok Koh
2019-04-05  1:33 ` [dpdk-dev] [PATCH v3 0/4] net/mlx: " Yongseok Koh
2019-04-05  1:33   ` Yongseok Koh
2019-04-05  1:33   ` [dpdk-dev] [PATCH v3 1/4] net/mlx5: fix recursive inclusion of header file Yongseok Koh
2019-04-05  1:33     ` Yongseok Koh
2019-04-05  1:33   ` [dpdk-dev] [PATCH v3 2/4] net/mlx5: remove redundant queue index Yongseok Koh
2019-04-05  1:33     ` Yongseok Koh
2019-04-08  5:24     ` Shahaf Shuler
2019-04-08  5:24       ` Shahaf Shuler
2019-04-05  1:33   ` [dpdk-dev] [PATCH v3 3/4] net/mlx5: remove device register remap Yongseok Koh
2019-04-05  1:33     ` Yongseok Koh
2019-04-08  5:48     ` Shahaf Shuler
2019-04-08  5:48       ` Shahaf Shuler
2019-04-09 19:36       ` Yongseok Koh [this message]
2019-04-09 19:36         ` Yongseok Koh
2019-04-05  1:33   ` [dpdk-dev] [PATCH v3 4/4] net/mlx4: " Yongseok Koh
2019-04-05  1:33     ` Yongseok Koh
2019-04-09 23:13 ` [dpdk-dev] [PATCH v4 0/4] net/mlx: " Yongseok Koh
2019-04-09 23:13   ` Yongseok Koh
2019-04-09 23:13   ` [dpdk-dev] [PATCH v4 1/4] net/mlx5: fix recursive inclusion of header file Yongseok Koh
2019-04-09 23:13     ` Yongseok Koh
2019-04-09 23:13   ` [dpdk-dev] [PATCH v4 2/4] net/mlx5: remove redundant queue index Yongseok Koh
2019-04-09 23:13     ` Yongseok Koh
2019-04-09 23:13   ` [dpdk-dev] [PATCH v4 3/4] net/mlx5: remove device register remap Yongseok Koh
2019-04-09 23:13     ` Yongseok Koh
2019-04-10 17:50     ` Ferruh Yigit
2019-04-10 17:50       ` Ferruh Yigit
2019-04-10 19:12       ` Yongseok Koh
2019-04-10 19:12         ` Yongseok Koh
2019-04-09 23:13   ` [dpdk-dev] [PATCH v4 4/4] net/mlx4: " Yongseok Koh
2019-04-09 23:13     ` Yongseok Koh
2019-04-10  6:58   ` [dpdk-dev] [PATCH v4 0/4] net/mlx: " Shahaf Shuler
2019-04-10  6:58     ` Shahaf Shuler
2019-04-10 17:50     ` Ferruh Yigit
2019-04-10 17:50       ` Ferruh Yigit
2019-04-10 18:41 ` [dpdk-dev] [PATCH v5 " Yongseok Koh
2019-04-10 18:41   ` Yongseok Koh
2019-04-10 18:41   ` [dpdk-dev] [PATCH v5 1/4] net/mlx5: fix recursive inclusion of header file Yongseok Koh
2019-04-10 18:41     ` Yongseok Koh
2019-04-10 18:41   ` [dpdk-dev] [PATCH v5 2/4] net/mlx5: remove redundant queue index Yongseok Koh
2019-04-10 18:41     ` Yongseok Koh
2019-04-10 18:41   ` [dpdk-dev] [PATCH v5 3/4] net/mlx5: remove device register remap Yongseok Koh
2019-04-10 18:41     ` Yongseok Koh
2019-04-10 18:41   ` [dpdk-dev] [PATCH v5 4/4] net/mlx4: " Yongseok Koh
2019-04-10 18:41     ` Yongseok Koh
2019-04-11  8:40   ` [dpdk-dev] [PATCH v5 0/4] net/mlx: " Shahaf Shuler
2019-04-11  8:40     ` Shahaf Shuler

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=0731AA80-EA07-4F93-B5DA-35FD442F0FDC@mellanox.com \
    --to=yskoh@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=shahafs@mellanox.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).