From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id ADEAFA04F1; Mon, 6 Jan 2020 10:03:36 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D456F1D5FE; Mon, 6 Jan 2020 10:03:35 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id A97C51D5F9 for ; Mon, 6 Jan 2020 10:03:33 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jan 2020 01:03:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,402,1571727600"; d="scan'208";a="370206882" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 06 Jan 2020 01:03:32 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 6 Jan 2020 01:03:31 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 6 Jan 2020 01:03:31 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.30]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.39]) with mapi id 14.03.0439.000; Mon, 6 Jan 2020 17:03:29 +0800 From: "Di, ChenxuX" To: "Ananyev, Konstantin" , "dev@dpdk.org" CC: "Yang, Qiming" Thread-Topic: [dpdk-dev] [PATCH v6 3/4] net/ixgbe: cleanup Tx buffers Thread-Index: AQHVvvUG2XKE65EZ4ECFZ4yLvAYxOqfSHGAAgATnLxCABTo6gIAA/MyA Date: Mon, 6 Jan 2020 09:03:28 +0000 Message-ID: <3B926E44943CB04AA3A39AC16328CE39B93D08@SHSMSX101.ccr.corp.intel.com> References: <20191203055134.72874-1-chenxux.di@intel.com> <20191230093840.17701-1-chenxux.di@intel.com> <20191230093840.17701-4-chenxux.di@intel.com> <3B926E44943CB04AA3A39AC16328CE39B9262D@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v6 3/4] net/ixgbe: cleanup Tx buffers 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Ananyev, Konstantin > Sent: Monday, January 6, 2020 7:36 AM > To: Di, ChenxuX ; dev@dpdk.org > Cc: Yang, Qiming > Subject: RE: [dpdk-dev] [PATCH v6 3/4] net/ixgbe: cleanup Tx buffers >=20 >=20 > > > > Add support to the ixgbe driver for the API > > > > rte_eth_tx_done_cleanup to force free consumed buffers on Tx ring. [snip] > > > > + * tx_tail is the last sent packet on the sw_ring. Goto the end > > > > + * of that packet (the last segment in the packet chain) and > > > > + * then the next segment will be the start of the oldest segment > > > > + * in the sw_ring. > > > > > > Not sure I understand the sentence above. > > > tx_tail is the value of TDT HW register (most recently armed by SW TD= ). > > > last_id is the index of last descriptor for multi-seg packet. > > > next_id is just the index of next descriptor in HW TD ring. > > > How do you conclude that it will be the ' oldest segment in the sw_ri= ng'? > > > > > > > The tx_tail is the last sent packet on the sw_ring. While the > > xmit_cleanup or Tx_free_bufs will be call when the nb_tx_free < > tx_free_thresh . > > So the sw_ring[tx_tail].next_id must be the begin of mbufs which are > > not used or Already freed . then begin the loop until the mbuf is used= and > begin to free them. > > > > > > > > > Another question why do you need to write your own functions? > > > Why can't you reuse existing ixgbe_xmit_cleanup() for full(offload) > > > path and > > > ixgbe_tx_free_bufs() for simple path? > > > Yes, ixgbe_xmit_cleanup() doesn't free mbufs, but at least it could > > > be used to determine finished TX descriptors. > > > Based on that you can you can free appropriate sw_ring[] entries. > > > > > > > The reason why I don't reuse existing function is that they all free > > several mbufs While the free_cnt of the API rte_eth_tx_done_cleanup() i= s the > number of packets. > > It also need to be done that check which mbuffs are from the same packe= t. >=20 > At first, I don't see anything bad if tx_done_cleanup() will free only so= me > segments from the packet. As long as it is safe - there is no problem wit= h that. > I think rte_eth_tx_done_cleanup() operates on mbuf, not packet quantities= . > But in our case I think it doesn't matter, as ixgbe_xmit_cleanup() mark T= XDs as > free only when HW is done with all TXDs for that packet. > As long as there is a way to reuse existing code and avoid duplication (w= ithout > introducing any degradation) - we should use it. > And I think there is a very good opportunity here to reuse existing > ixgbe_xmit_cleanup() for tx_done_cleanup() implementation. > Moreover because your code doesn't follow > ixgbe_xmit_pkts()/ixgbe_xmit_cleanup() > logic and infrastructure, it introduces unnecessary scans over TXD ring, = and in > some cases doesn't work as expected: >=20 > +while (1) { > +tx_last =3D sw_ring[tx_id].last_id; > + > +if (sw_ring[tx_last].mbuf) { > +if (txr[tx_last].wb.status & > +IXGBE_TXD_STAT_DD) { > ... > +} else { > +/* > + * mbuf still in use, nothing left to > + * free. > + */ > +break; >=20 > It is not correct to expect that IXGBE_TXD_STAT_DD will be set on last TX= D for > *every* packet. > We set IXGBE_TXD_CMD_RS bit only on threshold packet last descriptor. > Plus ixgbe_xmit_cleanup() can cleanup TXD wb.status. >=20 > So I strongly recommend to reuse ixgbe_xmit_cleanup() here. > It would be much less error prone and will help to avoid code duplication= . >=20 > Konstantin >=20 At first. The function ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq) will cleanup T= XD wb.status. the number of status cleanuped is always txq->tx_rs_thresh. The API rte_eth_tx_done_cleanup() in rte_eth_dev.h show that=20 @param free_cnt * Maximum number of packets to free. Use 0 to indicate all possible pac= kets * should be freed. Note that a packet may be using multiple mbufs. a number must be set while ixgbe_xmit_cleanup and ixgbe_tx_free_bufs only h= ave one parameter txq. And what should do is not only free buffers and status but also check which= bufs are from=20 One packet and count the packet freed.=20 So I think it can't be implemented that reuse function xmit_cleanup without= change it. And create a new function with the code of xmit_cleanup will cause many dup= lication. Above all , it seem not a perfect idea to reuse ixgbe_xmit_cleanup(). Second. The function in patch is copy from code in igb_rxtx.c. it already updated i= n 2017, The commit id is 8d907d2b79f7a54c809f1c44970ff455fa2865e1. I trust the logic of code is right. Actually it don't complete for ixgbe, i40e and ice, while it don't change t= he value of=20 last_desc_cleaned and tx_next_dd. And it's beginning prefer last_desc_clean= ed or tx_next_dd(for offload or simple) to tx_tail.=20 So, I suggest to use the old function and fix the issue. > > > > > > > >This is the first packet that will be > > > > + * attempted to be freed. > > > > + */ > > > > + > > > > +/* Get last segment in most recently added packet. */ tx_last =3D > > > > +sw_ring[txq->tx_tail].last_id; > > > > + > > > > +/* Get the next segment, which is the oldest segment in ring. */ > > > > +tx_first =3D sw_ring[tx_last].next_id; > > > > + > > > > +/* Set the current index to the first. */ tx_id =3D tx_first; > > > > + > > > > +/* > > > > + * Loop through each packet. For each packet, verify that an > > > > + * mbuf exists and that the last segment is free. If so, free > > > > + * it and move on. > > > > + */ > > > > +while (1) { > > > > +tx_last =3D sw_ring[tx_id].last_id; > > > > + > > > > +if (sw_ring[tx_last].mbuf) { > > > > +if (!(txr[tx_last].wb.status & > > > > +IXGBE_TXD_STAT_DD)) > > > > +break; > > > > + > > > > +/* Get the start of the next packet. */ tx_next =3D > > > > +sw_ring[tx_last].next_id; > > > > + > > > > +/* > > > > + * Loop through all segments in a > > > > + * packet. > > > > + */ > > > > +do { > > > > +rte_pktmbuf_free_seg(sw_ring[tx_id].mbuf); > > > > +sw_ring[tx_id].mbuf =3D NULL; > > > > +sw_ring[tx_id].last_id =3D tx_id; > > > > + > > > > +/* Move to next segment. */ > > > > +tx_id =3D sw_ring[tx_id].next_id; > > > > + > > > > +} while (tx_id !=3D tx_next); > > > > + > > > > +/* > > > > + * Increment the number of packets > > > > + * freed. > > > > + */ > > > > +count++; > > > > + > > > > +if (unlikely(count =3D=3D (int)free_cnt)) break; } else { > > > > +/* > > > > + * There are multiple reasons to be here: > > > > + * 1) All the packets on the ring have been > > > > + * freed - tx_id is equal to tx_first > > > > + * and some packets have been freed. > > > > + * - Done, exit > > > > + * 2) Interfaces has not sent a rings worth of > > > > + * packets yet, so the segment after tail is > > > > + * still empty. Or a previous call to this > > > > + * function freed some of the segments but > > > > + * not all so there is a hole in the list. > > > > + * Hopefully this is a rare case. > > > > + * - Walk the list and find the next mbuf. If > > > > + * there isn't one, then done. > > > > + */ > > > > +if (likely(tx_id =3D=3D tx_first && count !=3D 0)) break; > > > > + > > > > +/* > > > > + * Walk the list and find the next mbuf, if any. > > > > + */ > > > > +do { > > > > +/* Move to next segment. */ > > > > +tx_id =3D sw_ring[tx_id].next_id; > > > > + > > > > +if (sw_ring[tx_id].mbuf) > > > > +break; > > > > + > > > > +} while (tx_id !=3D tx_first); > > > > + > > > > +/* > > > > + * Determine why previous loop bailed. If there > > > > + * is not an mbuf, done. > > > > + */ > > > > +if (sw_ring[tx_id].mbuf =3D=3D NULL) > > > > +break; > > > > +} > > > > +} > > > > + > > > > +return count; > > > > +} > > > > + > > > > static void __attribute__((cold)) ixgbe_tx_free_swring(struct > > > > ixgbe_tx_queue *txq) { diff --git > > > > a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h > > > > index 505d344b9..2c3770af6 100644 > > > > --- a/drivers/net/ixgbe/ixgbe_rxtx.h > > > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.h > > > > @@ -285,6 +285,8 @@ int > > > > ixgbe_rx_vec_dev_conf_condition_check(struct > > > > rte_eth_dev *dev); int ixgbe_rxq_vec_setup(struct ixgbe_rx_queue > > > > *rxq); void ixgbe_rx_queue_release_mbufs_vec(struct > > > > ixgbe_rx_queue *rxq); > > > > > > > > +int ixgbe_tx_done_cleanup(void *txq, uint32_t free_cnt); > > > > + > > > > extern const uint32_t ptype_table[IXGBE_PACKET_TYPE_MAX]; > > > > extern const uint32_t ptype_table_tn[IXGBE_PACKET_TYPE_TN_MAX]; > > > > > > > > -- > > > > 2.17.1 > > > > > >=20