From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id DD7AD1DBA for ; Tue, 26 Jun 2018 03:38:26 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2018 18:38:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,272,1526367600"; d="scan'208";a="52876910" Received: from pgsmsx102.gar.corp.intel.com ([10.221.44.80]) by orsmga006.jf.intel.com with ESMTP; 25 Jun 2018 18:38:23 -0700 Received: from pgsmsx103.gar.corp.intel.com ([169.254.2.131]) by PGSMSX102.gar.corp.intel.com ([169.254.6.46]) with mapi id 14.03.0319.002; Tue, 26 Jun 2018 09:38:22 +0800 From: "Zhao1, Wei" To: Olivier Matz , "Zhao1@6wind.com" , "Zhang@6wind.com" , "Zhang, Qi Z" CC: "dev@dpdk.org" , "Lu@6wind.com" , "Lu, Wenzhuo" Thread-Topic: [RFC] net/ixgbe: fix Tx descriptor status api Thread-Index: AQHUDI/QX0Pu05mNjUqIqu1I04IjpaRxwQeQ Date: Tue, 26 Jun 2018 01:38:22 +0000 Message-ID: References: <20180625142057.6296-1-olivier.matz@6wind.com> In-Reply-To: <20180625142057.6296-1-olivier.matz@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jun 2018 01:38:27 -0000 Hi, Olivier Matz Will you commit fix patch for i40e and ixgbe and em? And the code " dd =3D (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - = 1;" Is only proper for tx function ixgbe_xmit_pkts_simple and ixgbe_xmit_pkts_= vec (). But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different fro= m all these two.=20 > -----Original Message----- > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Monday, June 25, 2018 10:21 PM > To: Zhao1@6wind.com; Zhao1, Wei ; > Zhang@6wind.com; Zhang, Qi Z > Cc: dev@dpdk.org; Lu@6wind.com; Lu, Wenzhuo > Subject: [RFC] net/ixgbe: fix Tx descriptor status api >=20 > The Tx descriptor status api was not behaving as expected. This API is us= ed to > inspect the content of the descriptors in the Tx ring to determine the le= ngth > of the Tx queue. >=20 > Since the software advances the tail pointer and the hardware advances th= e > head pointer, the Tx queue is located before txq->tx_tail in the ring. > Therefore, a call to rte_eth_tx_descriptor_status(..., offset=3D20) shoul= d > inspect the 20th descriptor before the tail, not after. >=20 > As before, we still need to take care about only checking descriptors tha= t > have the RS bit. >=20 > Additionally, we can avoid an access to the ring if offset is greater or = equal to > nb_tx_desc - nb_tx_free. >=20 > Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API") > Signed-off-by: Olivier Matz > Signed-off-by: Didier Pallard > --- >=20 > Hi Wei, Hi Qi, >=20 > We also recently found some issues in Tx descriptor status API for ixgbe,= i40, > e1000, igb. I'm preparing a clean patchset for all of them. >=20 > Here is already the patch for ixgbe, please let me know what you think. >=20 > The API comment of rte_eth_tx_descriptor_status() is incorrect and should > be fixed too. The reference descriptor (when offset =3D 0) is not where t= he > next packet will be sent, but where the latest packet has been enqueued. >=20 > Regards, > Olivier >=20 >=20 >=20 > drivers/net/ixgbe/ixgbe_rxtx.c | 45 > +++++++++++++++++++++++++++++++----------- > drivers/net/ixgbe/ixgbe_rxtx.h | 1 + > 2 files changed, 34 insertions(+), 12 deletions(-) >=20 > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxt= x.c > index 3e13d26ae..384587cc6 100644 > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > @@ -2606,10 +2606,15 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev > *dev, > hw->mac.type =3D=3D ixgbe_mac_X540_vf || > hw->mac.type =3D=3D ixgbe_mac_X550_vf || > hw->mac.type =3D=3D ixgbe_mac_X550EM_x_vf || > - hw->mac.type =3D=3D ixgbe_mac_X550EM_a_vf) > + hw->mac.type =3D=3D ixgbe_mac_X550EM_a_vf) { > txq->tdt_reg_addr =3D IXGBE_PCI_REG_ADDR(hw, > IXGBE_VFTDT(queue_idx)); > - else > + txq->tdh_reg_addr =3D IXGBE_PCI_REG_ADDR(hw, > + IXGBE_VFTDH(queue_idx)); > + } else { > txq->tdt_reg_addr =3D IXGBE_PCI_REG_ADDR(hw, > IXGBE_TDT(txq->reg_idx)); > + txq->tdh_reg_addr =3D IXGBE_PCI_REG_ADDR(hw, > + IXGBE_TDH(txq->reg_idx)); > + } >=20 > txq->tx_ring_phys_addr =3D tz->iova; > txq->tx_ring =3D (union ixgbe_adv_tx_desc *) tz->addr; @@ -3140,22 > +3145,38 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t > offset) { > struct ixgbe_tx_queue *txq =3D tx_queue; > volatile uint32_t *status; > - uint32_t desc; > + int32_t desc, dd; >=20 > if (unlikely(offset >=3D txq->nb_tx_desc)) > return -EINVAL; > + if (offset >=3D txq->nb_tx_desc - txq->nb_tx_free) > + return RTE_ETH_TX_DESC_DONE; > + > + desc =3D txq->tx_tail - offset - 1; > + if (desc < 0) > + desc +=3D txq->nb_tx_desc; >=20 > - desc =3D txq->tx_tail + offset; > - /* go to next desc that has the RS bit */ > - desc =3D ((desc + txq->tx_rs_thresh - 1) / txq->tx_rs_thresh) * > - txq->tx_rs_thresh; > - if (desc >=3D txq->nb_tx_desc) { > - desc -=3D txq->nb_tx_desc; > - if (desc >=3D txq->nb_tx_desc) > - desc -=3D txq->nb_tx_desc; > + /* offset is too small, no other way than reading PCI reg */ > + if (unlikely(offset < txq->tx_rs_thresh)) { > + int16_t tx_head, queue_size; > + tx_head =3D ixgbe_read_addr(txq->tdh_reg_addr); > + queue_size =3D txq->tx_tail - tx_head; > + if (queue_size < 0) > + queue_size +=3D txq->nb_tx_desc; > + return queue_size > offset ? RTE_ETH_TX_DESC_FULL : > + RTE_ETH_TX_DESC_DONE; > } >=20 > - status =3D &txq->tx_ring[desc].wb.status; > + /* index of the dd bit to look at */ > + dd =3D (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1; > + > + /* In full featured mode, RS bit is only set in the last descriptor */ > + /* of a multisegments packet */ > + if (!((txq->offloads =3D=3D 0) && > + (txq->tx_rs_thresh >=3D RTE_PMD_IXGBE_TX_MAX_BURST))) > + dd =3D txq->sw_ring[dd].last_id; > + > + status =3D &txq->tx_ring[dd].wb.status; > if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD)) > return RTE_ETH_TX_DESC_DONE; >=20 > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxt= x.h > index 39378f754..384f6324d 100644 > --- a/drivers/net/ixgbe/ixgbe_rxtx.h > +++ b/drivers/net/ixgbe/ixgbe_rxtx.h > @@ -201,6 +201,7 @@ struct ixgbe_tx_queue { > struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring > for vector PMD */ > }; > volatile uint32_t *tdt_reg_addr; /**< Address of TDT register. */ > + volatile uint32_t *tdh_reg_addr; /**< Address of TDH register. */ > uint16_t nb_tx_desc; /**< number of TX descriptors. */ > uint16_t tx_tail; /**< current value of TDT reg. */ > /**< Start freeing TX buffers if there are less free descriptors than > -- > 2.11.0