> -----Original Message-----
> From: Wu, Jingjing <jingjing...@intel.com>
> Sent: Thursday, May 25, 2023 11:59 AM
> To: Xing, Beilei <beilei.x...@intel.com>
> Cc: dev@dpdk.org; Liu, Mingxia <mingxia....@intel.com>; Wang, Xiao W
> <xiao.w.w...@intel.com>
> Subject: RE: [PATCH v3 05/10] net/cpfl: support hairpin queue setup and
> release
>
> >
> > +static int
> > +cpfl_rx_hairpin_bufq_setup(struct rte_eth_dev *dev, struct idpf_rx_queue
> *bufq,
> > + uint16_t logic_qid, uint16_t nb_desc) {
> > + struct cpfl_vport *cpfl_vport =
> > + (struct cpfl_vport *)dev->data->dev_private;
> > + struct idpf_vport *vport = &cpfl_vport->base;
> > + struct idpf_adapter *adapter = vport->adapter;
> > + struct rte_mempool *mp;
> > + char pool_name[RTE_MEMPOOL_NAMESIZE];
> > +
> > + mp = cpfl_vport->p2p_mp;
> > + if (!mp) {
> > + snprintf(pool_name, RTE_MEMPOOL_NAMESIZE,
> "p2p_mb_pool_%u",
> > + dev->data->port_id);
> > + mp = rte_pktmbuf_pool_create(pool_name,
> CPFL_P2P_NB_MBUF,
> > CPFL_P2P_CACHE_SIZE,
> > + 0, CPFL_P2P_MBUF_SIZE, dev-
> >device-
> > >numa_node);
> > + if (!mp) {
> > + PMD_INIT_LOG(ERR, "Failed to allocate mbuf pool for
> p2p");
> > + return -ENOMEM;
> > + }
> > + cpfl_vport->p2p_mp = mp;
> > + }
> > +
> > + bufq->mp = mp;
> > + bufq->nb_rx_desc = nb_desc;
> > + bufq->queue_id = cpfl_hw_qid_get(cpfl_vport-
> > >p2p_q_chunks_info.rx_buf_start_qid, logic_qid);
> > + bufq->port_id = dev->data->port_id;
> > + bufq->adapter = adapter;
> > + bufq->rx_buf_len = CPFL_P2P_MBUF_SIZE -
> RTE_PKTMBUF_HEADROOM;
> > +
> > + bufq->sw_ring = rte_zmalloc("sw ring",
> > + sizeof(struct rte_mbuf *) * nb_desc,
> > + RTE_CACHE_LINE_SIZE);
>
> Is sw_ring required in p2p case? It has been never used right?
> Please also check the sw_ring in tx queue.
Yes, it should be removed.
>
> > + if (!bufq->sw_ring) {
> > + PMD_INIT_LOG(ERR, "Failed to allocate memory for SW ring");
> > + return -ENOMEM;
> > + }
> > +
> > + bufq->q_set = true;
> > + bufq->ops = &def_rxq_ops;
> > +
> > + return 0;
> > +}
> > +
> > +int
> > +cpfl_rx_hairpin_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> > + uint16_t nb_desc,
> > + const struct rte_eth_hairpin_conf *conf) {
> > + struct cpfl_vport *cpfl_vport = (struct cpfl_vport *)dev->data-
> >dev_private;
> > + struct idpf_vport *vport = &cpfl_vport->base;
> > + struct idpf_adapter *adapter_base = vport->adapter;
> > + uint16_t logic_qid = cpfl_vport->nb_p2p_rxq;
> > + struct cpfl_rxq_hairpin_info *hairpin_info;
> > + struct cpfl_rx_queue *cpfl_rxq;
> > + struct idpf_rx_queue *bufq1 = NULL;
> > + struct idpf_rx_queue *rxq;
> > + uint16_t peer_port, peer_q;
> > + uint16_t qid;
> > + int ret;
> > +
> > + if (vport->rxq_model == VIRTCHNL2_QUEUE_MODEL_SINGLE) {
> > + PMD_INIT_LOG(ERR, "Only spilt queue model supports hairpin
> queue.");
> > + return -EINVAL;
> > + }
> > +
> > + if (conf->peer_count != 1) {
> > + PMD_INIT_LOG(ERR, "Can't support Rx hairpin queue peer
> count %d",
> > conf->peer_count);
> > + return -EINVAL;
> > + }
> > +
> > + peer_port = conf->peers[0].port;
> > + peer_q = conf->peers[0].queue;
> > +
> > + if (nb_desc % CPFL_ALIGN_RING_DESC != 0 ||
> > + nb_desc > CPFL_MAX_RING_DESC ||
> > + nb_desc < CPFL_MIN_RING_DESC) {
> > + PMD_INIT_LOG(ERR, "Number (%u) of receive descriptors is
> invalid",
> > nb_desc);
> > + return -EINVAL;
> > + }
> > +
> > + /* Free memory if needed */
> > + if (dev->data->rx_queues[queue_idx]) {
> > + cpfl_rx_queue_release(dev->data->rx_queues[queue_idx]);
> > + dev->data->rx_queues[queue_idx] = NULL;
> > + }
> > +
> > + /* Setup Rx description queue */
> > + cpfl_rxq = rte_zmalloc_socket("cpfl hairpin rxq",
> > + sizeof(struct cpfl_rx_queue),
> > + RTE_CACHE_LINE_SIZE,
> > + SOCKET_ID_ANY);
> > + if (!cpfl_rxq) {
> > + PMD_INIT_LOG(ERR, "Failed to allocate memory for rx queue
> data
> > structure");
> > + return -ENOMEM;
> > + }
> > +
> > + rxq = &cpfl_rxq->base;
> > + hairpin_info = &cpfl_rxq->hairpin_info;
> > + rxq->nb_rx_desc = nb_desc * 2;
> > + rxq->queue_id =
> > +cpfl_hw_qid_get(cpfl_vport->p2p_q_chunks_info.rx_start_qid,
> > logic_qid);
> > + rxq->port_id = dev->data->port_id;
> > + rxq->adapter = adapter_base;
> > + rxq->rx_buf_len = CPFL_P2P_MBUF_SIZE -
> RTE_PKTMBUF_HEADROOM;
> > + hairpin_info->hairpin_q = true;
> > + hairpin_info->peer_txp = peer_port;
> > + hairpin_info->peer_txq_id = peer_q;
> > +
> > + if (conf->manual_bind != 0)
> > + cpfl_vport->p2p_manual_bind = true;
> > + else
> > + cpfl_vport->p2p_manual_bind = false;
> > +
> > + /* setup 1 Rx buffer queue for the 1st hairpin rxq */
> > + if (logic_qid == 0) {
> > + bufq1 = rte_zmalloc_socket("hairpin rx bufq1",
> > + sizeof(struct idpf_rx_queue),
> > + RTE_CACHE_LINE_SIZE,
> > + SOCKET_ID_ANY);
> > + if (!bufq1) {
> > + PMD_INIT_LOG(ERR, "Failed to allocate memory for
> hairpin Rx
> > buffer queue 1.");
> > + ret = -ENOMEM;
> > + goto err_alloc_bufq1;
> > + }
> > + qid = 2 * logic_qid;
>
> Inside the brace ( if (logic_qid=0) {} ), the logic_qid should be zero,
> right? What
> is the purpose of doing qid = 2* logic_qid?
The if condition should be refined.
>
> > + ret = cpfl_rx_hairpin_bufq_setup(dev, bufq1, qid, nb_desc);
> > + if (ret) {
> > + PMD_INIT_LOG(ERR, "Failed to setup hairpin Rx buffer
> queue 1");
> > + ret = -EINVAL;
> > + goto err_setup_bufq1;
> > + }
> > + cpfl_vport->p2p_rx_bufq = bufq1;
> > + }
> > +
> > + rxq->bufq1 = cpfl_vport->p2p_rx_bufq;
> > + rxq->bufq2 = NULL;
> > +
>
> cpfl_vport->p2p_rx_bufq is allocated in this function. But haven't seen where
> it
> will be released.
It will be released in cpfl_rx_queue_release function.
> And in cpfl_rx_hairpin_bufq_reset the rxq->bufq1 will be assigned to NULL.
> Will queue release miss this?
>
> > + cpfl_vport->nb_p2p_rxq++;
> > + rxq->q_set = true;
> > + dev->data->rx_queues[queue_idx] = cpfl_rxq;
> > +
> > + return 0;
> > +
> > +err_setup_bufq1:
> > + rte_free(bufq1);
> > +err_alloc_bufq1:
> > + rte_free(rxq);
> > +
> > + return ret;
> > +}
> > +
