Re: [dpdk-dev] [PATCH v4 1/4] raw/ntb: setup ntb queue

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

Hi

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Monday, September 23, 2019 10:50
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wiles, Keith <keith.wiles@intel.com>;
> Maslekar, Omkar <omkar.maslekar@intel.com>; Liang, Cunming
> <cunming.liang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v4 1/4] raw/ntb: setup ntb queue
> 
> <...>
> > +static void
> > +ntb_rxq_release(struct ntb_rx_queue *rxq) {
> > +	if (!rxq) {
> > +		NTB_LOG(ERR, "Pointer to rxq is NULL");
> > +		return;
> > +	}
> > +
> > +	ntb_rxq_release_mbufs(rxq);
> > +
> > +	rte_free(rxq->sw_ring);
> > +	rte_free(rxq);
> It' better to free rxq out of this function, as the point of param "rxq" cannot be
> set to NULL in this func.
OK.
> 
> > +}
> > +
> > +static int
> > +ntb_rxq_setup(struct rte_rawdev *dev,
> > +	      uint16_t qp_id,
> > +	      rte_rawdev_obj_t queue_conf)
> > +{
> > +	struct ntb_queue_conf *rxq_conf = queue_conf;
> > +	struct ntb_hw *hw = dev->dev_private;
> > +	struct ntb_rx_queue *rxq;
> > +
> > +	/* Allocate the rx queue data structure */
> > +	rxq = rte_zmalloc_socket("ntb rx queue",
> > +				 sizeof(struct ntb_rx_queue),
> > +				 RTE_CACHE_LINE_SIZE,
> > +				 dev->socket_id);
> > +	if (!rxq) {
> > +		NTB_LOG(ERR, "Failed to allocate memory for "
> > +			    "rx queue data structure.");
> > +		return -ENOMEM;
> Need to free rxq here.
It only allocates memory. And this error means allocate failure and rxq == NULL. So no need to free here.

> <...>
> 
> > +static void
> > +ntb_txq_release(struct ntb_tx_queue *txq)
> >  {
> > +	if (!txq) {
> > +		NTB_LOG(ERR, "Pointer to txq is NULL");
> > +		return;
> > +	}
> > +
> > +	ntb_txq_release_mbufs(txq);
> > +
> > +	rte_free(txq->sw_ring);
> > +	rte_free(txq);
> The same as above "ntb_rxq_release".
OK.
> 
> <...>
> 
> > +static int
> > +ntb_queue_setup(struct rte_rawdev *dev,
> > +		uint16_t queue_id,
> > +		rte_rawdev_obj_t queue_conf)
> > +{
> > +	struct ntb_hw *hw = dev->dev_private;
> > +	int ret;
> > +
> > +	if (queue_id > hw->queue_pairs)
> Should be ">=" ?
Yes.
> 
> > +		return -EINVAL;
> > +
> > +	ret = ntb_txq_setup(dev, queue_id, queue_conf);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ntb_rxq_setup(dev, queue_id, queue_conf);
> > +
> > +	return ret;
> > +}
> > +
> >  static int
> > -ntb_queue_release(struct rte_rawdev *dev __rte_unused,
> > -		  uint16_t queue_id __rte_unused)
> > +ntb_queue_release(struct rte_rawdev *dev, uint16_t queue_id)
> >  {
> > +	struct ntb_hw *hw = dev->dev_private;
> > +	struct ntb_tx_queue *txq;
> > +	struct ntb_rx_queue *rxq;
> > +
> > +	if (queue_id > hw->queue_pairs)
> Should be ">=" ?
> 
> > +		return -EINVAL;
> > +
> > +	txq = hw->tx_queues[queue_id];
> > +	rxq = hw->rx_queues[queue_id];
> > +	ntb_txq_release(txq);
> > +	ntb_rxq_release(rxq);
> > +
> >  	return 0;
> >  }
> >
> > @@ -234,6 +470,77 @@ ntb_queue_count(struct rte_rawdev *dev)
> >  	return hw->queue_pairs;
> >  }
> >
> > +static int
> > +ntb_queue_init(struct rte_rawdev *dev, uint16_t qp_id) {
> > +	struct ntb_hw *hw = dev->dev_private;
> > +	struct ntb_rx_queue *rxq = hw->rx_queues[qp_id];
> > +	struct ntb_tx_queue *txq = hw->tx_queues[qp_id];
> > +	volatile struct ntb_header *local_hdr;
> > +	struct ntb_header *remote_hdr;
> > +	uint16_t q_size = hw->queue_size;
> > +	uint32_t hdr_offset;
> > +	void *bar_addr;
> > +	uint16_t i;
> > +
> > +	if (hw->ntb_ops->get_peer_mw_addr == NULL) {
> > +		NTB_LOG(ERR, "Failed to get mapped peer addr.");
> Would it be better to log as "XX ops is not supported" to keep consistent as
> others?
OK.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Put queue info into the start of shared memory. */
> > +	hdr_offset = hw->hdr_size_per_queue * qp_id;
> > +	local_hdr = (volatile struct ntb_header *)
> > +		    ((size_t)hw->mz[0]->addr + hdr_offset);
> > +	bar_addr = (*hw->ntb_ops->get_peer_mw_addr)(dev, 0);
> > +	if (bar_addr == NULL)
> > +		return -EINVAL;
> > +	remote_hdr = (struct ntb_header *)
> > +		     ((size_t)bar_addr + hdr_offset);
> > +
> > +	/* rxq init. */
> > +	rxq->rx_desc_ring = (struct ntb_desc *)
> > +			    (&remote_hdr->desc_ring);
> > +	rxq->rx_used_ring = (volatile struct ntb_used *)
> > +			    (&local_hdr->desc_ring[q_size]);
> > +	rxq->avail_cnt = &remote_hdr->avail_cnt;
> > +	rxq->used_cnt = &local_hdr->used_cnt;
> > +
> > +	for (i = 0; i < rxq->nb_rx_desc - 1; i++) {
> > +		struct rte_mbuf *mbuf = rte_mbuf_raw_alloc(rxq->mpool);
> > +		if (unlikely(!mbuf)) {
> > +			NTB_LOG(ERR, "Failed to allocate mbuf for RX");
> Need release mbufs allocated here or in " ntb_dev_start".
OK.
> 
> <...>
> 
> > +	hw->hdr_size_per_queue = RTE_ALIGN(sizeof(struct ntb_header) +
> > +				hw->queue_size * sizeof(struct ntb_desc) +
> > +				hw->queue_size * sizeof(struct ntb_used),
> > +				RTE_CACHE_LINE_SIZE);
> hw->hdr_size_per_queue is internal information, why put the assignment in
> ntb_dev_info_get?
Because the calculation needs the app to pass params (queue size and queue number). And the app needs the result to populate mempool and then configure to driver.
There is no else place  where can do the calculation.
> 
> > +	info->ntb_hdr_size = hw->hdr_size_per_queue * hw->queue_pairs;
> >  }
> >
> >  static int
> > -ntb_dev_configure(const struct rte_rawdev *dev __rte_unused,
> > -		  rte_rawdev_obj_t config __rte_unused)
> > +ntb_dev_configure(const struct rte_rawdev *dev, rte_rawdev_obj_t
> > +config)
> >  {
> > +	struct ntb_dev_config *conf = config;
> > +	struct ntb_hw *hw = dev->dev_private;
> > +	int ret;
> > +
> > +	hw->queue_pairs	= conf->num_queues;
> > +	hw->queue_size = conf->queue_size;
> > +	hw->used_mw_num = conf->mz_num;
> > +	hw->mz = conf->mz_list;
> > +	hw->rx_queues = rte_zmalloc("ntb_rx_queues",
> > +			sizeof(struct ntb_rx_queue *) * hw->queue_pairs, 0);
> > +	hw->tx_queues = rte_zmalloc("ntb_tx_queues",
> > +			sizeof(struct ntb_tx_queue *) * hw->queue_pairs, 0);
> > +
> > +	/* Start handshake with the peer. */
> > +	ret = ntb_handshake_work(dev);
> > +	if (ret < 0)
> Free?
OK.
> 
> > +		return ret;
> > +
> >  	return 0;
> >  }
> >
> > @@ -337,21 +637,52 @@ static int
> >  ntb_dev_start(struct rte_rawdev *dev)  {
> >  	struct ntb_hw *hw = dev->dev_private;
> > -	int ret, i;
> > +	uint32_t peer_base_l, peer_val;
> > +	uint64_t peer_base_h;
> > +	uint32_t i;
> > +	int ret;
> >
> > -	/* TODO: init queues and start queues. */
> > +	if (!hw->link_status || !hw->peer_dev_up)
> > +		return -EINVAL;
> >
> > -	/* Map memory of bar_size to remote. */
> > -	hw->mz = rte_zmalloc("struct rte_memzone *",
> > -			     hw->mw_cnt * sizeof(struct rte_memzone *), 0);
> > -	for (i = 0; i < hw->mw_cnt; i++) {
> > -		ret = ntb_set_mw(dev, i, hw->mw_size[i]);
> > +	for (i = 0; i < hw->queue_pairs; i++) {
> > +		ret = ntb_queue_init(dev, i);
> >  		if (ret) {
> > -			NTB_LOG(ERR, "Fail to set mw.");
> > +			NTB_LOG(ERR, "Failed to init queue.");
> Free when error.
OK.
> 
> <...>
> 
> > +struct ntb_used {
> > +	uint16_t len;     /* buffer length */
> > +#define NTB_FLAG_EOP    1 /* end of packet */
> Better to
> > +	uint16_t flags;   /* flags */
> > +};
> > +
> > +struct ntb_rx_entry {
> > +	struct rte_mbuf *mbuf;
> > +};
> > +
> > +struct ntb_rx_queue {
> > +	struct ntb_desc *rx_desc_ring;
> > +	volatile struct ntb_used *rx_used_ring;
> > +	uint16_t *avail_cnt;
> > +	volatile uint16_t *used_cnt;
> > +	uint16_t last_avail;
> > +	uint16_t last_used;
> > +	uint16_t nb_rx_desc;
> > +
> > +	uint16_t rx_free_thresh;
> > +
> > +	struct rte_mempool *mpool; /**< mempool for mbuf allocation */
> Generally comments: comments in internal header doesn't need to be wrapped
> with "/**< */", keep consistent in one file would be nice.
OK.