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 54DD8A04F2; Mon, 6 Jan 2020 00:36:16 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7A9E41D166; Mon, 6 Jan 2020 00:36:15 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 7B9D81D159 for ; Mon, 6 Jan 2020 00:36:13 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jan 2020 15:36:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,400,1571727600"; d="scan'208";a="217266536" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga008.fm.intel.com with ESMTP; 05 Jan 2020 15:36:11 -0800 Received: from fmsmsx161.amr.corp.intel.com (10.18.125.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 15:36:11 -0800 Received: from FMSEDG001.ED.cps.intel.com (10.1.192.133) by FMSMSX161.amr.corp.intel.com (10.18.125.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 15:36:11 -0800 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.171) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 5 Jan 2020 15:36:11 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cC0faYf/+6ur7bXv3gGGeZV5jHd5mUFyKfd7FZn5KgxK/fGQzCjXKHmJrii+liIfhv0BjmkP5unDQZpEGTBcpLu24UMoncGCSKfBdFB1kpaEtVXhWHQT9+bZcLiGEwhKwh01OQsP8zjse0F4sL54molzoRn/Ydh/i9oQuhpiK5AyFpZoAM3sFdWOoz9+v5l6zvpB4+/D2PE9pZ4HQATlua1BzREPJoyht+iNX1f4hOfU+48dhO5msxYN5iaSck1Z8wkRRvSLmBdfgOBdCwg8s6Yl2Y/tIdwzAhev2I2Fb0RunKo9c+IkGIDzBIGDuXObNL/QxWKjPPQzCOG/Ync/rg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=n5HxnLlsiELgJRIl2giCVNPu5y/WzWlf+iSINnZBIx8=; b=J/vYrHBEi7FTp96Scozo4VP3TVkKQEw3mu4pNbT3CJKVNVCuMEEU7CXYWjY41+dOKBZ1pIVd0SG70pDWZ+WbhYJu67pfr9MUQJEONVAYUjOAZEvuBSqwqgB003nPuxyERtgUzP+TteJSRSoT9WkLrYT+MMTdIAWV2KIuvJ1v//pw0Lv/jmud1oSfy8uTER3vEHdkMVxQMrxWe+J3pQL5N8sfzuddK6xbGXrAMm8evjC70ZD3mHjh1pJt/C5qpiHnwKAROXeS725aTAnLg5tlgCJAMq6LuR419LpF3TTZnhu/HzjsbfQOpchjLF4D76vTCIJxRNkWNFJZM8J1k3djNg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=n5HxnLlsiELgJRIl2giCVNPu5y/WzWlf+iSINnZBIx8=; b=i+cxTLus9qA4ec6/dDAOPs43ghlUXG0GhsZh4fRNlWSsf9Xmh08ixf/fjBEZNEqsHqv0NkEVSThUDkTUTg9smaUSuddlFUYjfNI+45V1T/rj4LwH5kaURhtiY7tTCUxO4uwzIHbud6YUifnhlUGQEpgVP1Lv7v+Zla2jr/lKBj8= Received: from SN6PR11MB2558.namprd11.prod.outlook.com (52.135.94.19) by SN6PR11MB3133.namprd11.prod.outlook.com (52.135.125.142) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2602.11; Sun, 5 Jan 2020 23:36:09 +0000 Received: from SN6PR11MB2558.namprd11.prod.outlook.com ([fe80::4d86:362a:13c3:8386]) by SN6PR11MB2558.namprd11.prod.outlook.com ([fe80::4d86:362a:13c3:8386%7]) with mapi id 15.20.2602.015; Sun, 5 Jan 2020 23:36:09 +0000 From: "Ananyev, Konstantin" To: "Di, ChenxuX" , "dev@dpdk.org" CC: "Yang, Qiming" Thread-Topic: [dpdk-dev] [PATCH v6 3/4] net/ixgbe: cleanup Tx buffers Thread-Index: AQHVvvUp6LlAIfvCjkGBZ/4yGRUggKfSnF5QgAYOaoCABAZmYA== Date: Sun, 5 Jan 2020 23:36:09 +0000 Message-ID: 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: <3B926E44943CB04AA3A39AC16328CE39B9262D@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDI0ODQ3ZDYtMWU3Mi00YzViLTg0MzAtYjA2YWNmYTJlMDZiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiQkNPKzZRRzRQQTJLdEo3WkFlWjdWb0xqamtOM0F3ZmhrYTl1VWcyRmZmM29aRmZJNVViXC9Lc2VaR3lQNzRYMTMifQ== dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 x-ctpclassification: CTP_NT authentication-results: spf=none (sender IP is ) smtp.mailfrom=konstantin.ananyev@intel.com; x-originating-ip: [192.198.151.163] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 8081f120-7c73-4820-0d63-08d792380ad5 x-ms-traffictypediagnostic: SN6PR11MB3133: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-forefront-prvs: 027367F73D x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(346002)(376002)(136003)(396003)(39850400004)(366004)(199004)(189003)(478600001)(26005)(7696005)(4326008)(9686003)(6506007)(107886003)(2906002)(33656002)(186003)(55016002)(86362001)(5660300002)(71200400001)(110136005)(76116006)(66946007)(66446008)(64756008)(52536014)(66476007)(66556008)(8676002)(81156014)(316002)(8936002)(81166006); DIR:OUT; SFP:1102; SCL:1; SRVR:SN6PR11MB3133; H:SN6PR11MB2558.namprd11.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: NN99ekLsKlQqieCpl3M/8iM2+j2+AifTsoeyCkRXbt4eW8amNB/JLEbEKE7wkP8q7FjWz/xuajjzY//NpedT+1MVT+aTjnUMNLuSxR1upAovG7MwhLo+x6AijM5O3/D/BUbPWrWVqDFkZEpuP824hQouuYxyzFiFB9OTvaLk+ECHh/Lw3EbsmIE50VxibrUDVKaW4m8embawNiD8Md4QAZtMLbj1pMLrA1KSo6RJpjoLBcAAG3sOBR6skMbwLl6p/cxvKX8GCIBaf+BjrUtBfRPMlpZydHk9HSEfHz5qyWpsnfsXkPrlFy2wGimZCmWXvsSN/HCxXvev9+/U0CQUcVgMfgOyEx6VXyJMfEY8zJcy9f/u+Ont2xWS8UuEnL9hgnoZqyQWUgCuEaqmVK1kikGW1sKJ4VpRgLee2pD+sJS3OrmLEy/+9ISaYYKEEqrF Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 8081f120-7c73-4820-0d63-08d792380ad5 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jan 2020 23:36:09.0774 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: at6hLIxw2zYwQgZIKFNt0uTlQ7uPME/rNs2WVuqK8LhhAIFrUlOYZt4abAnUZ5pKCXC1djqcHGvYmCuaHXfnzbM6RjZ5Lr8GcQj2XTz0S6Q= X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB3133 X-OriginatorOrg: intel.com 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" > > > Add support to the ixgbe driver for the API rte_eth_tx_done_cleanup t= o > > > force free consumed buffers on Tx ring. > > > > > > Signed-off-by: Chenxu Di > > > --- > > > drivers/net/ixgbe/ixgbe_ethdev.c | 2 + > > > drivers/net/ixgbe/ixgbe_rxtx.c | 116 +++++++++++++++++++++++++++++= ++ > > > drivers/net/ixgbe/ixgbe_rxtx.h | 2 + > > > 3 files changed, 120 insertions(+) > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > > index 2c6fd0f13..0091405db 100644 > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > > @@ -601,6 +601,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops > > > =3D { .udp_tunnel_port_add =3D ixgbe_dev_udp_tunnel_port_add, > > > .udp_tunnel_port_del =3D ixgbe_dev_udp_tunnel_port_del, > > > .tm_ops_get =3D ixgbe_tm_ops_get, > > > +.tx_done_cleanup =3D ixgbe_tx_done_cleanup, > > > > Don't see how we can have one tx_done_cleanup() for different tx functi= ons? > > Vector and scalar TX path use different format for sw_ring[] entries. > > Also offload and simile TX paths use different method to track used/fre= e > > descriptors, and use different functions to free them: > > offload uses tx_entry next_id, last_id plus txq. last_desc_cleaned, whi= le simple > > TX paths use tx_next_dd. > > >=20 > This patches will be not include function for Vector, and I will update m= y code to > Make it work for offload and simple . > > > > > }; > > > > > > /* > > > @@ -649,6 +650,7 @@ static const struct eth_dev_ops ixgbevf_eth_dev_o= ps > > =3D { > > > .reta_query =3D ixgbe_dev_rss_reta_query, > > > .rss_hash_update =3D ixgbe_dev_rss_hash_update, > > > .rss_hash_conf_get =3D ixgbe_dev_rss_hash_conf_get, > > > +.tx_done_cleanup =3D ixgbe_tx_done_cleanup, > > > }; > > > > > > /* store statistics names and its offset in stats structure */ diff > > > --git a/drivers/net/ixgbe/ixgbe_rxtx.c > > > b/drivers/net/ixgbe/ixgbe_rxtx.c index fa572d184..520b9c756 100644 > > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > > > @@ -2306,6 +2306,122 @@ ixgbe_tx_queue_release_mbufs(struct > > > ixgbe_tx_queue *txq) } } > > > > > > +int ixgbe_tx_done_cleanup(void *q, uint32_t free_cnt) > > > > That seems to work only for offload(full) TX path (ixgbe_xmit_pkts). > > Simple(fast) path seems not covered by this function. > > >=20 > Same as above >=20 > > > +{ > > > +struct ixgbe_tx_queue *txq =3D (struct ixgbe_tx_queue *)q; struct > > > +ixgbe_tx_entry *sw_ring; volatile union ixgbe_adv_tx_desc *txr; > > > +uint16_t tx_first; /* First segment analyzed. */ > > > +uint16_t tx_id; /* Current segment being processed. */ > > > +uint16_t tx_last; /* Last segment in the current packet. */ uint16_= t > > > +tx_next; /* First segment of the next packet. */ int count; > > > + > > > +if (txq =3D=3D NULL) > > > +return -ENODEV; > > > + > > > +count =3D 0; > > > +sw_ring =3D txq->sw_ring; > > > +txr =3D txq->tx_ring; > > > + > > > +/* > > > + * 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_ring= '? > > >=20 > The tx_tail is the last sent packet on the sw_ring. While the xmit_cleanu= p 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. >=20 >=20 >=20 > > Another question why do you need to write your own functions? > > Why can't you reuse existing ixgbe_xmit_cleanup() for full(offload) pat= h 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. > > >=20 > The reason why I don't reuse existing function is that they all free seve= ral mbufs > While the free_cnt of the API rte_eth_tx_done_cleanup() is the number of = packets. > It also need to be done that check which mbuffs are from the same packet. At first, I don't see anything bad if tx_done_cleanup() will free only some= segments from the packet. As long as it is safe - there is no problem with 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 TXDs 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 (without 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_clea= nup() 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; It is not correct to expect that IXGBE_TXD_STAT_DD will be set on last TXD = 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. So I strongly recommend to reuse ixgbe_xmit_cleanup() here. It would be much less error prone and will help to avoid code duplication. Konstantin=20 >=20 >=20 > > >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