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

["Wu, Jingjing" <jingjing.wu@intel.com>]

<...>
> +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.

> +}
> +
> +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.
<...>

> +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".

<...>

> +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 ">=" ?

> +		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?

> +		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".

<...>

> +	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?

> +	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?

> +		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.

<...>

> +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.