From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id EB4E4A057C;
	Thu, 26 Mar 2020 07:50:24 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id B9AC5378E;
	Thu, 26 Mar 2020 07:50:23 +0100 (CET)
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
 by dpdk.org (Postfix) with ESMTP id 441BC2AB
 for <dev@dpdk.org>; Thu, 26 Mar 2020 07:50:22 +0100 (CET)
IronPort-SDR: 8aq6+w3T7K16bb4xaLeLKcUQ4tpVFXHgELI2h73jzFdAcNAo0tYjYsLrhUwxJ2CR08BZPcE8+6
 /Zj/0PUxZq9Q==
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga001.jf.intel.com ([10.7.209.18])
 by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
 25 Mar 2020 23:50:21 -0700
IronPort-SDR: noHvcdpvpF3scrI68Z/TMZBYFPSdC7OZpCfRDFoPW0ETByqqBOpBGk4758SCgogJx5RPs5QrDk
 H2998kiiDLvQ==
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.72,307,1580803200"; d="scan'208";a="326469937"
Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206])
 by orsmga001.jf.intel.com with ESMTP; 25 Mar 2020 23:50:20 -0700
Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by
 FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS)
 id 14.3.439.0; Wed, 25 Mar 2020 23:50:20 -0700
Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by
 SHSMSX601.ccr.corp.intel.com (10.109.6.141) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.1713.5; Thu, 26 Mar 2020 14:50:18 +0800
Received: from shsmsx603.ccr.corp.intel.com ([10.109.6.143]) by
 SHSMSX603.ccr.corp.intel.com ([10.109.6.143]) with mapi id 15.01.1713.004;
 Thu, 26 Mar 2020 14:50:18 +0800
From: "Wu, Jingjing" <jingjing.wu@intel.com>
To: "Rong, Leyi" <leyi.rong@intel.com>, "Zhang, Qi Z" <qi.z.zhang@intel.com>, 
 "Ye, Xiaolong" <xiaolong.ye@intel.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, "Rong, Leyi" <leyi.rong@intel.com>
Thread-Topic: [dpdk-dev] [PATCH 04/12] net/iavf: flexible Rx descriptor
 support	in normal path
Thread-Index: AQHV+2i1L3r/jpMxsEm7aEt00HRIl6haeMLg
Date: Thu, 26 Mar 2020 06:50:18 +0000
Message-ID: <486c8c3962414a8d97e08f2e9a53e54d@intel.com>
References: <20200316074603.10998-1-leyi.rong@intel.com>
 <20200316074603.10998-5-leyi.rong@intel.com>
In-Reply-To: <20200316074603.10998-5-leyi.rong@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [10.239.127.36]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 04/12] net/iavf: flexible Rx descriptor
	support	in normal path
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

One general comment:
Looks like there are exact same code comparing with legacy rx, such as logi=
c to update tail, multi-segments loop.
It will be good if all the common code can be wrapped and use for multi rec=
v functions

[...]

+/* Get the number of used descriptors of a rx queue for flexible RXD */=20
+uint32_t iavf_dev_rxq_count_flex_rxd(struct rte_eth_dev *dev, uint16_t=20
+queue_id) { #define IAVF_RXQ_SCAN_INTERVAL 4
+	volatile union iavf_rx_flex_desc *rxdp;
+	struct iavf_rx_queue *rxq;
+	uint16_t desc =3D 0;
+
+	rxq =3D dev->data->rx_queues[queue_id];
+	rxdp =3D (volatile union iavf_rx_flex_desc *)&rxq->rx_ring[rxq->rx_tail];
+	while ((desc < rxq->nb_rx_desc) &&
+	       rte_le_to_cpu_16(rxdp->wb.status_error0) &
+	       (1 << IAVF_RX_FLEX_DESC_STATUS0_DD_S)) {
+		/* Check the DD bit of a rx descriptor of each 4 in a group,
+		 * to avoid checking too frequently and downgrading performance
+		 * too much.
+		 */
+		desc +=3D IAVF_RXQ_SCAN_INTERVAL;
+		rxdp +=3D IAVF_RXQ_SCAN_INTERVAL;
+		if (rxq->rx_tail + desc >=3D rxq->nb_rx_desc)
+			rxdp =3D (volatile union iavf_rx_flex_desc *)
+				&(rxq->rx_ring[rxq->rx_tail +
+					desc - rxq->nb_rx_desc]);
+	}
+
+	return desc;
+}

No much difference between iavf_dev_rxq_count. Why do we need a new one? DD=
 bit is located in the same place, right?
Can we merge those two functions to one? =20
+
 /* Get the number of used descriptors of a rx queue */  uint32_t  iavf_dev=
_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id) @@ -1795,6 +2264,10 =
@@ iavf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id)
=20
 	rxq =3D dev->data->rx_queues[queue_id];
 	rxdp =3D &rxq->rx_ring[rxq->rx_tail];
+
+	if (rxq->rxdid =3D=3D IAVF_RXDID_COMMS_OVS_1)
+		return iavf_dev_rxq_count_flex_rxd(dev, queue_id);
+
 	while ((desc < rxq->nb_rx_desc) &&
 	       ((rte_le_to_cpu_64(rxdp->wb.qword1.status_error_len) &
 		 IAVF_RXD_QW1_STATUS_MASK) >> IAVF_RXD_QW1_STATUS_SHIFT) & @@ -1813,6 +2=
286,31 @@ iavf_dev_rxq_count(struct rte_eth_dev *dev, uint16_t queue_id)
 	return desc;
 }
=20
+int
+iavf_dev_rx_desc_status_flex_rxd(void *rx_queue, uint16_t offset) {
+	volatile union iavf_rx_flex_desc *rxdp;
+	struct iavf_rx_queue *rxq =3D rx_queue;
+	uint32_t desc;
+
+	if (unlikely(offset >=3D rxq->nb_rx_desc))
+		return -EINVAL;
+
+	if (offset >=3D rxq->nb_rx_desc - rxq->nb_rx_hold)
+		return RTE_ETH_RX_DESC_UNAVAIL;
+
+	desc =3D rxq->rx_tail + offset;
+	if (desc >=3D rxq->nb_rx_desc)
+		desc -=3D rxq->nb_rx_desc;
+
+	rxdp =3D (volatile union iavf_rx_flex_desc *)&rxq->rx_ring[desc];
+	if (rte_le_to_cpu_16(rxdp->wb.status_error0) &
+		(1 << IAVF_RX_FLEX_DESC_STATUS0_DD_S))
+		return RTE_ETH_RX_DESC_DONE;
+
+	return RTE_ETH_RX_DESC_AVAIL;
+}
+

Similar comments as above.

[......]=20

  { @@ -569,6 +596,20 @@ iavf_configure_queues(struct iavf_adapter *adapter=
)
 			vc_qp->rxq.ring_len =3D rxq[i]->nb_rx_desc;
 			vc_qp->rxq.dma_ring_addr =3D rxq[i]->rx_ring_phys_addr;
 			vc_qp->rxq.databuffer_size =3D rxq[i]->rx_buf_len;
+
+			if (vf->vf_res->vf_cap_flags &
+			    VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC &&
+			    vf->supported_rxdid & BIT(IAVF_RXDID_COMMS_OVS_1)) {
+				vc_qp->rxq.rxdid =3D IAVF_RXDID_COMMS_OVS_1;
+				rxq[i]->rxdid =3D IAVF_RXDID_COMMS_OVS_1;
Because this function is used to construct virtchnl message for configure q=
ueues.
The rxdid in rxq[i] should be set before this function is called. How about=
 to move this line assignment to iavf_dev_rx_queue_setup?
+				PMD_DRV_LOG(NOTICE, "request RXDID =3D=3D %d in "
+					    "Queue[%d]", vc_qp->rxq.rxdid, i);
+			} else {
+				vc_qp->rxq.rxdid =3D IAVF_RXDID_LEGACY_1;
+				rxq[i]->rxdid =3D IAVF_RXDID_LEGACY_1;
Same as above.

+				PMD_DRV_LOG(NOTICE, "request RXDID =3D=3D %d in "
+					    "Queue[%d]", vc_qp->rxq.rxdid, i);
+			}
 		}
 	}
=20
--
2.17.1