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? > + ppriv = rte_malloc_socket("mlx5_proc_priv", ppriv_size, > + RTE_CACHE_LINE_SIZE, > + dpdk_dev->numa_node); > + if (!ppriv) { > + rte_errno = ENOMEM; > + return NULL; > + } > + ppriv->uar_table_sz = ppriv_size; > + eth_dev->process_private = ppriv; > eth_dev->device = dpdk_dev; > eth_dev->dev_ops = &mlx5_dev_sec_ops; > /* Receive command fd from primary process */ @@ -1195,7 > +1043,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev, > if (err < 0) > return NULL; > /* Remap UAR for Tx queues. */ > - err = mlx5_tx_uar_remap(eth_dev, err); > + err = mlx5_tx_uar_init_secondary(eth_dev, err); > if (err) > return NULL; > /* > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > 699c8fcf6d..1ac4ad71b1 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -97,8 +97,6 @@ struct mlx5_shared_data { > /* Global spinlock for primary and secondary processes. */ > int init_done; /* Whether primary has done initialization. */ > unsigned int secondary_cnt; /* Number of secondary processes > init'd. */ > - void *uar_base; > - /* Reserved UAR address space for TXQ UAR(hw doorbell) mapping. > */ > struct mlx5_dev_list mem_event_cb_list; > rte_rwlock_t mem_event_rwlock; > }; > @@ -106,8 +104,6 @@ struct mlx5_shared_data { > /* Per-process data structure, not visible to other processes. */ struct > mlx5_local_data { > int init_done; /* Whether a secondary has done initialization. */ > - void *uar_base; > - /* Reserved UAR address space for TXQ UAR(hw doorbell) mapping. > */ > }; > > extern struct mlx5_shared_data *mlx5_shared_data; @@ -282,6 +278,17 > @@ struct mlx5_ibv_shared { > struct mlx5_ibv_shared_port port[]; /* per device port data array. */ > }; > > +/* Per-process private structure. */ > +struct mlx5_proc_priv { > + size_t uar_table_sz; > + /* Size of UAR register table. */ > + void *uar_table[]; > + /* Table of UAR registers for each process. */ }; > + > +#define MLX5_PROC_PRIV(port_id) \ > + ((struct mlx5_proc_priv *)rte_eth_devices[port_id].process_private) > + > struct mlx5_priv { > LIST_ENTRY(mlx5_priv) mem_event_cb; > /**< Called by memory event callback. */ diff --git > a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index > 9ae9dddd3c..42297f11c9 100644 > --- a/drivers/net/mlx5/mlx5_ethdev.c > +++ b/drivers/net/mlx5/mlx5_ethdev.c > @@ -382,6 +382,8 @@ int > mlx5_dev_configure(struct rte_eth_dev *dev) { > struct mlx5_priv *priv = dev->data->dev_private; > + struct mlx5_proc_priv *ppriv; > + size_t ppriv_size; > unsigned int rxqs_n = dev->data->nb_rx_queues; > unsigned int txqs_n = dev->data->nb_tx_queues; > unsigned int i; > @@ -450,6 +452,21 @@ mlx5_dev_configure(struct rte_eth_dev *dev) > if (++j == rxqs_n) > j = 0; > } > + /* > + * 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 *); Ditto. > + 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. > }; > > +#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? Would you consider to prefetch the addr before the db logic just to be on the safe side? > volatile uint64_t *src = ((volatile uint64_t *)wqe); > > rte_cio_wmb(); > diff --git a/drivers/net/mlx5/mlx5_trigger.c > b/drivers/net/mlx5/mlx5_trigger.c index 7c1e5594d6..b7fde35758 100644 > --- a/drivers/net/mlx5/mlx5_trigger.c > +++ b/drivers/net/mlx5/mlx5_trigger.c > @@ -58,12 +58,6 @@ mlx5_txq_start(struct rte_eth_dev *dev) > goto error; > } > } > - ret = mlx5_tx_uar_remap(dev, priv->sh->ctx->cmd_fd); > - if (ret) { > - /* Adjust index for rollback. */ > - i = priv->txqs_n - 1; > - goto error; > - } > return 0; > error: > ret = rte_errno; /* Save rte_errno before cleanup. */ diff --git > a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c index > 4bd08cb035..5fb1761955 100644 > --- a/drivers/net/mlx5/mlx5_txq.c > +++ b/drivers/net/mlx5/mlx5_txq.c > @@ -229,13 +229,99 @@ mlx5_tx_queue_release(void *dpdk_txq) > } > } > > +/** > + * Initialize Tx UAR registers for primary process. > + * > + * @param txq_ctrl > + * Pointer to Tx queue control structure. > + */ > +static void > +txq_uar_init(struct mlx5_txq_ctrl *txq_ctrl) { > + struct mlx5_priv *priv = txq_ctrl->priv; > + struct mlx5_proc_priv *ppriv = MLX5_PROC_PRIV(PORT_ID(priv)); > + > + assert(rte_eal_process_type() == RTE_PROC_PRIMARY); > + assert(ppriv); > + ppriv->uar_table[txq_ctrl->txq.idx] = txq_ctrl->bf_reg; #ifndef > +RTE_ARCH_64 > + struct mlx5_priv *priv = txq_ctrl->priv; > + struct mlx5_txq_data *txq = &txq_ctrl->txq; > + unsigned int lock_idx; > + /* Assign an UAR lock according to UAR page number */ > + lock_idx = (txq_ctrl->uar_mmap_offset / page_size) & > + MLX5_UAR_PAGE_NUM_MASK; > + txq->uar_lock = &priv->uar_lock[lock_idx]; #endif } > > /** > - * Mmap TX UAR(HW doorbell) pages into reserved UAR address space. > - * Both primary and secondary process do mmap to make UAR address > - * aligned. > + * Remap UAR register of a Tx queue for secondary process. > * > - * @param[in] dev > + * Remapped address is stored at the table in the process private > +structure of > + * the device, indexed by queue index. > + * > + * @param txq_ctrl > + * Pointer to Tx queue control structure. > + * @param fd > + * Verbs file descriptor to map UAR pages. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +txq_uar_init_secondary(struct mlx5_txq_ctrl *txq_ctrl, int fd) { > + struct mlx5_priv *priv = txq_ctrl->priv; > + struct mlx5_proc_priv *ppriv = MLX5_PROC_PRIV(PORT_ID(priv)); > + struct mlx5_txq_data *txq = &txq_ctrl->txq; > + void *addr; > + uintptr_t uar_va; > + uintptr_t offset; > + const size_t page_size = sysconf(_SC_PAGESIZE); > + > + assert(ppriv); > + /* > + * As rdma-core, UARs are mapped in size of OS page > + * size. Ref to libmlx5 function: mlx5_init_context() > + */ > + uar_va = (uintptr_t)txq_ctrl->bf_reg; > + offset = uar_va & (page_size - 1); /* Offset in page. */ > + addr = mmap(NULL, page_size, PROT_WRITE, MAP_SHARED, fd, > + txq_ctrl->uar_mmap_offset); > + if (addr == MAP_FAILED) { > + DRV_LOG(ERR, > + "port %u mmap failed for BF reg of txq %u", > + txq->port_id, txq->idx); > + rte_errno = ENXIO; > + return -rte_errno; > + } > + addr = RTE_PTR_ADD(addr, offset); > + ppriv->uar_table[txq->idx] = addr; > + return 0; > +} > + > +/** > + * Unmap UAR register of a Tx queue for secondary process. > + * > + * @param txq_ctrl > + * Pointer to Tx queue control structure. > + */ > +static void > +txq_uar_uninit_secondary(struct mlx5_txq_ctrl *txq_ctrl) { > + struct mlx5_proc_priv *ppriv = MLX5_PROC_PRIV(PORT_ID(txq_ctrl- > >priv)); > + const size_t page_size = sysconf(_SC_PAGESIZE); > + void *addr; > + > + addr = ppriv->uar_table[txq_ctrl->txq.idx]; > + munmap(RTE_PTR_ALIGN_FLOOR(addr, page_size), page_size); } > + > +/** > + * Initialize Tx UAR registers for secondary process. > + * > + * @param dev > * Pointer to Ethernet device. > * @param fd > * Verbs file descriptor to map UAR pages. > @@ -244,81 +330,36 @@ mlx5_tx_queue_release(void *dpdk_txq) > * 0 on success, a negative errno value otherwise and rte_errno is set. > */ > int > -mlx5_tx_uar_remap(struct rte_eth_dev *dev, int fd) > +mlx5_tx_uar_init_secondary(struct rte_eth_dev *dev, int fd) > { > struct mlx5_priv *priv = dev->data->dev_private; > - unsigned int i, j; > - uintptr_t pages[priv->txqs_n]; > - unsigned int pages_n = 0; > - uintptr_t uar_va; > - uintptr_t off; > - void *addr; > - void *ret; > struct mlx5_txq_data *txq; > struct mlx5_txq_ctrl *txq_ctrl; > - int already_mapped; > - size_t page_size = sysconf(_SC_PAGESIZE); > -#ifndef RTE_ARCH_64 > - unsigned int lock_idx; > -#endif > + unsigned int i; > + int ret; > > - memset(pages, 0, priv->txqs_n * sizeof(uintptr_t)); > - /* > - * As rdma-core, UARs are mapped in size of OS page size. > - * Use aligned address to avoid duplicate mmap. > - * Ref to libmlx5 function: mlx5_init_context() > - */ > + assert(rte_eal_process_type() == RTE_PROC_SECONDARY); > for (i = 0; i != priv->txqs_n; ++i) { > if (!(*priv->txqs)[i]) > continue; > txq = (*priv->txqs)[i]; > txq_ctrl = container_of(txq, struct mlx5_txq_ctrl, txq); > assert(txq->idx == (uint16_t)i); > - /* UAR addr form verbs used to find dup and offset in page. > */ > - uar_va = (uintptr_t)txq_ctrl->bf_reg_orig; > - off = uar_va & (page_size - 1); /* offset in page. */ > - uar_va = RTE_ALIGN_FLOOR(uar_va, page_size); /* page > addr. */ > - already_mapped = 0; > - for (j = 0; j != pages_n; ++j) { > - if (pages[j] == uar_va) { > - already_mapped = 1; > - break; > - } > - } > - /* new address in reserved UAR address space. */ > - addr = RTE_PTR_ADD(mlx5_shared_data->uar_base, > - uar_va & (uintptr_t)(MLX5_UAR_SIZE - 1)); > - if (!already_mapped) { > - pages[pages_n++] = uar_va; > - /* fixed mmap to specified address in reserved > - * address space. > - */ > - ret = mmap(addr, page_size, > - PROT_WRITE, MAP_FIXED | MAP_SHARED, > fd, > - txq_ctrl->uar_mmap_offset); > - if (ret != addr) { > - /* fixed mmap have to return same address > */ > - DRV_LOG(ERR, > - "port %u call to mmap failed on UAR" > - " for txq %u", > - dev->data->port_id, txq->idx); > - rte_errno = ENXIO; > - return -rte_errno; > - } > - } > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) /* save > once */ > - txq_ctrl->txq.bf_reg = RTE_PTR_ADD((void *)addr, > off); > - else > - assert(txq_ctrl->txq.bf_reg == > - RTE_PTR_ADD((void *)addr, off)); > -#ifndef RTE_ARCH_64 > - /* Assign a UAR lock according to UAR page number */ > - lock_idx = (txq_ctrl->uar_mmap_offset / page_size) & > - MLX5_UAR_PAGE_NUM_MASK; > - txq->uar_lock = &priv->uar_lock[lock_idx]; > -#endif > + ret = txq_uar_init_secondary(txq_ctrl, fd); > + if (ret) > + goto error; > } > return 0; > +error: > + /* Rollback. */ > + do { > + if (!(*priv->txqs)[i]) > + continue; > + txq = (*priv->txqs)[i]; > + txq_ctrl = container_of(txq, struct mlx5_txq_ctrl, txq); > + txq_uar_uninit_secondary(txq_ctrl); > + } while (i--); > + return -rte_errno; > } > > /** > @@ -507,7 +548,6 @@ mlx5_txq_ibv_new(struct rte_eth_dev *dev, > uint16_t idx) > txq_data->wqes = qp.sq.buf; > txq_data->wqe_n = log2above(qp.sq.wqe_cnt); > txq_data->qp_db = &qp.dbrec[MLX5_SND_DBR]; > - txq_ctrl->bf_reg_orig = qp.bf.reg; > txq_data->cq_db = cq_info.dbrec; > txq_data->cqes = > (volatile struct mlx5_cqe (*)[]) > @@ -521,6 +561,8 @@ mlx5_txq_ibv_new(struct rte_eth_dev *dev, > uint16_t idx) > txq_ibv->qp = tmpl.qp; > txq_ibv->cq = tmpl.cq; > rte_atomic32_inc(&txq_ibv->refcnt); > + txq_ctrl->bf_reg = qp.bf.reg; > + txq_uar_init(txq_ctrl); > if (qp.comp_mask & MLX5DV_QP_MASK_UAR_MMAP_OFFSET) { > txq_ctrl->uar_mmap_offset = qp.uar_mmap_offset; > DRV_LOG(DEBUG, "port %u: uar_mmap_offset 0x%lx", @@ - > 778,6 +820,7 @@ mlx5_txq_new(struct rte_eth_dev *dev, uint16_t idx, > uint16_t desc, > tmpl->priv = priv; > tmpl->socket = socket; > tmpl->txq.elts_n = log2above(desc); > + tmpl->txq.port_id = dev->data->port_id; > tmpl->txq.idx = idx; > txq_set_params(tmpl); > DRV_LOG(DEBUG, "port %u device_attr.max_qp_wr is %d", @@ - > 836,15 +879,12 @@ mlx5_txq_release(struct rte_eth_dev *dev, uint16_t idx) > { > struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_txq_ctrl *txq; > - size_t page_size = sysconf(_SC_PAGESIZE); > > if (!(*priv->txqs)[idx]) > return 0; > txq = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl, txq); > if (txq->ibv && !mlx5_txq_ibv_release(txq->ibv)) > txq->ibv = NULL; > - munmap((void *)RTE_ALIGN_FLOOR((uintptr_t)txq->txq.bf_reg, > page_size), > - page_size); > if (rte_atomic32_dec_and_test(&txq->refcnt)) { > txq_free_elts(txq); > mlx5_mr_btree_free(&txq->txq.mr_ctrl.cache_bh); > -- > 2.11.0
