> On Apr 7, 2019, at 10:48 PM, Shahaf Shuler <[email protected]> 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 <[email protected]>
>> ---
>> 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
