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 645EDA04DD; Thu, 22 Oct 2020 17:33:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 730FFAA2A; Thu, 22 Oct 2020 17:33:55 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id D4ED0AA21; Thu, 22 Oct 2020 17:33:52 +0200 (CEST) IronPort-SDR: pqKS0Usu01DDy43p6Ufe98CQXlphJdvtTsSWW6EBSsmzuXxw397MTVG9ziru8U/hrtDjrqn2zx zK5tYUwEqwoQ== X-IronPort-AV: E=McAfee;i="6000,8403,9781"; a="229176202" X-IronPort-AV: E=Sophos;i="5.77,404,1596524400"; d="scan'208";a="229176202" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Oct 2020 08:33:47 -0700 IronPort-SDR: JVjuamUCGBRYnV0ODmZI6nu+ngXnjLIrv1QoumzztuPcEdrOlgJlnriAWMo7YFS0lyK8l0OC8m bkWnyTCQCTdw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,404,1596524400"; d="scan'208";a="359901682" Received: from orsmsx605.amr.corp.intel.com ([10.22.229.18]) by orsmga007.jf.intel.com with ESMTP; 22 Oct 2020 08:33:46 -0700 Received: from orsmsx607.amr.corp.intel.com (10.22.229.20) by ORSMSX605.amr.corp.intel.com (10.22.229.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 22 Oct 2020 08:33:45 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx607.amr.corp.intel.com (10.22.229.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 22 Oct 2020 08:33:45 -0700 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.102) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Thu, 22 Oct 2020 08:33:44 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=K3LPDZkfZ3sLv3F4EpmyEJpb8DBlbIIcCXudewkf66JjE0Jc5DhwV7uOBdXgKkpGiVH2BUE/PGmbPtjJO1rzCUNLcal3EJBHf4frAbbNpm1g8Trnp1wtsklJgWmzN3iRYzvgCsC4SqJtLds3CrJWnd79uDCsxAutEebhb3EbjpdYxtC8xejil0z9KeVhfWMzpGbQP/LikibC4Qvd4v6n6oQXk+DlBIlFTqPq7xJVsp6jdcoZcEjksPSyyznPcbKy3QhNO0phPnsoviMZuEFl36oF8vsAHCewH3tnxULb0Q5OR++TVkmL7UHxjeg5aIYNaAH1bYFqIZoPY1277Kllgg== 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=Sp1pEXH7JmLYeTt+jZLS95S7iVsqry6oZ4y6/40DHIE=; b=ju56L145w+Zjlwe4lcnJhu0Xz6I1b4Vr3Nkuxkf27wWpqGY6Y29vFzubKe1YXxXe8FeUu/CJQ/3PdpY2GVpybxlMIcIrpyEH6uYOgEHjPwd5vPsM5QwZjb+TRVhnQMHfaqSaqAO/AAhi6VT2Awoj7skk1Fx/E4T1fUQtZnOhmEGISTThFIRb3zu5iRSw8o+86FfAQKi5AhFGKzQaiG/2RyM9enJCuTBpxziKganoeqZKvgTJWp/Pn3C5KFjvp7hYI3VW4XLapvW46hRuz5onn/kPDLCTzyiiU8EpmOvq6WwuGAVkrTqjEb0BasclirCGkpn+E6Exou+5uH/Iwjj1tg== 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=Sp1pEXH7JmLYeTt+jZLS95S7iVsqry6oZ4y6/40DHIE=; b=jUZoKXlK9kJxtMT874snlecsQlJJddjS/qwzS+/fSSYg1lOsVrSmjfutiId7BxzPbjrW419izf+ZEZS3J3rNoDBUjwjeJhL/xnA8TO9wWfHZnavu7YeWliF0iOZ4/pjiyqmORLfPvu/2rElY+G1c+mFyaRMkOUFOVpTTi1YyWmY= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BYAPR11MB3669.namprd11.prod.outlook.com (2603:10b6:a03:f7::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3477.23; Thu, 22 Oct 2020 15:33:42 +0000 Received: from BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f5a4:3f6b:ade3:296b]) by BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f5a4:3f6b:ade3:296b%3]) with mapi id 15.20.3499.018; Thu, 22 Oct 2020 15:33:42 +0000 From: "Ananyev, Konstantin" To: "yang_y_yi@163.com" , "dev@dpdk.org" CC: "Hu, Jiayu" , "techboard@dpdk.org" , "thomas@monjalon.net" , "yangyi01@inspur.com" Thread-Topic: [PATCH v2] gso: fix free issue of mbuf gso segments attach to Thread-Index: AQHWqD/tzleaP+GQ60mK7fZdnSpuYqmjlw5wgAApS9A= Date: Thu, 22 Oct 2020 15:33:42 +0000 Message-ID: References: <20201022065151.10108-1-yang_y_yi@163.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: 163.com; dkim=none (message not signed) header.d=none;163.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [46.7.39.127] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: ccf52b9c-305d-451e-7355-08d8769fdb32 x-ms-traffictypediagnostic: BYAPR11MB3669: 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-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: R2ccLYYOLydxMZOltD5OdqOSVAwIHkStLdGY3JS+2qrMUNd3weFfJxSfSDpdyKX8VVlZrDGGAAcuGdQyYxLmPz+6ZYn/bxz+jSu69ByigSJM/9sGRKZq0DyddeA283YEfMO3NRon5CIWwlhMQ276mmdke4pff/upFHC4W8kM9bT21HwSGcxjqbmwoYAUZuNZACd4avZ9Z5vlbO3fwemqM34ijPIZXbPcXd3qGv8COoW9QQS3Cy7jPCq0+VoNJoY1gL4DPzr0Fb4C/k/C5bsqblPAr+y748qftGjiseyy38Laqf5FTviwR5ecZwGtQ+MxfzPnB885MNthAfMbS+Fzxg== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR11MB3301.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(396003)(39860400002)(346002)(366004)(136003)(376002)(52536014)(26005)(9686003)(478600001)(83380400001)(5660300002)(8676002)(4326008)(71200400001)(8936002)(6506007)(54906003)(66446008)(64756008)(66556008)(66476007)(76116006)(110136005)(7696005)(66946007)(55016002)(86362001)(33656002)(316002)(2940100002)(186003)(2906002); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: zZN7FvG1siCQ5UlU191Rq39oAxzFhdOakVAi8eYhsZC5Ha7K14vQQ2JYXFL5vDVSraBCHbSV2DxHLEhQ4QjvmAIauo8xVQ0arXunAovZi8oGP7FZlCv6ZE05Ic7tSlUW/heL+RgU4E/0eq+CDqkZ9cu37i9LAKho3kzUdm+NLtbOwTTxmOnj0ItwB/tu7yES1yemTl222shEiqlzopSCrBtUeACn+V0gLyni/nlOGW+KEc6XtlhFdvKSxAUzJUyBQcgVE/XJsPey+cWb21HnIY4mWI80QhkF+eYlvxQL+ng/2zHO94zrWnDsXTdw6pQ6GnF/ekJmsYfFdMax2WdLqrz9oAmC/5o38LbvtFVAbG6yw4TJJlQaB2yoPxBg30Xl6kOkTGzXy62LXNk4gyVEFndpq0x+E1qOrolj+2USC7yrA9/ZhuZ0045gUlP4sbFn9hKYCgj3W6Nsogb2gInjdvODdMmCkovH5w9jM8djFksyXkZ2xrAWJR3jORBAiTlxtWM8HEGoOgIk/SzjQDef1seIa3KFFM6Tj/KyP8VjaFuqybn7Tu9HvaQE7apW5oXi7xwZYlPWI2bFrLzeRisFJ+9tdTuVwus1AoB83mlEFosIsaa67h8Ovw9o4aGc9L9TS3/2IfthxeuL5az66hVSfw== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3301.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ccf52b9c-305d-451e-7355-08d8769fdb32 X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Oct 2020 15:33:42.1270 (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: WZFblutsip7rcaL43j1X5xFsFaQ0Q/M0tGqtmRM8wycOXTLEndd5o6iZdXkY7hMcyCToIZgWcRJIv1EKEWpfyDDRJYKfRMv2sbQLiXK6oCc= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3669 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH v2] gso: fix free issue of mbuf gso segments attach to 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" > > > > rte_gso_segment decreased refcnt of pkt by one, but > > it is wrong if pkt is external mbuf, pkt won't be > > freed because of incorrect refcnt, the result is > > application can't allocate mbuf from mempool because > > mbufs in mempool are run out of. > > > > One correct way is application should call > > rte_pktmbuf_free after calling rte_gso_segment to free > > pkt explicitly. rte_gso_segment mustn't handle it, this > > should be responsibility of application. >=20 > Probably needs to be stated clearly: > It is a change in functional behaviour. > Without deprecation note in advance. > TB members: please provide your opinion on that patch. >=20 > > > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO") > > Signed-off-by: Yi Yang > > --- > > Changelog: > > > > v1->v2: > > - update description of rte_gso_segment(). > > - change code which calls rte_gso_segment() to > > fix free issue. > > > > --- > > app/test-pmd/csumonly.c | 3 ++- > > doc/guides/prog_guide/generic_segmentation_offload_lib.rst | 7 +++++-- >=20 > I think release notes also have to be updated. >=20 > > lib/librte_gso/rte_gso.c | 9 +------= -- > > lib/librte_gso/rte_gso.h | 7 +++++-- > > 4 files changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c > > index 3d7d244..829e07f 100644 > > --- a/app/test-pmd/csumonly.c > > +++ b/app/test-pmd/csumonly.c > > @@ -1080,11 +1080,12 @@ struct simple_gre_hdr { > > ret =3D rte_gso_segment(pkts_burst[i], gso_ctx, > > &gso_segments[nb_segments], > > GSO_MAX_PKT_BURST - nb_segments); > > + /* pkts_burst[i] can be freed safely here. */ > > + rte_pktmbuf_free(pkts_burst[i]); >=20 > It doesn't look correct to me. > I think it should be: > If (ret > 1) rte_pktmbuf_free(pkts_burst[i]); >=20 > > if (ret >=3D 0) > > nb_segments +=3D ret; > > else { > > TESTPMD_LOG(DEBUG, "Unable to segment packet"); > > - rte_pktmbuf_free(pkts_burst[i]); > > } > > } >=20 >=20 > About drivers/net/tap/rte_eth_tap.c: > I think it has to be modified too, as here: >=20 > /* free original mbuf */ > rte_pktmbuf_free(mbuf_in); > /* free tso mbufs */ > if (num_tso_mbufs > 0) > rte_pktmbuf_free_bulk(mbuf, num_tso_mbufs); >=20 > if mbuf[0] =3D=3D mbuf_in > Will have a double free() for the same mbuf. >=20 > > > > diff --git a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > index 205cb8a..8577572 100644 > > --- a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > +++ b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > @@ -25,8 +25,9 @@ Bearing that in mind, the GSO library enables DPDK ap= plications to segment > > packets in software. Note however, that GSO is implemented as a standa= lone > > library, and not via a 'fallback' mechanism (i.e. for when TSO is unsu= pported > > in the underlying hardware); that is, applications must explicitly inv= oke the > > -GSO library to segment packets. The size of GSO segments ``(segsz)`` i= s > > -configurable by the application. > > +GSO library to segment packets, they also must call ``rte_pktmbuf_free= ()`` to > > +free mbuf GSO segments attach to after calling ``rte_gso_segment()``. >=20 > Probably worth to mention that if return code =3D=3D 1, then > output mbuf will point to input mbuf and extra care with free() is requir= ed. Another possibility - change gso_segment() behaviour even further, and don't put input_pkt into pkt_out[] when no segmentation happened. Might be even a bit cleaner and less error prone.=20 >=20 > > The size > > +of GSO segments ``(segsz)`` is configurable by the application. > > > > Limitations > > ----------- > > @@ -233,6 +234,8 @@ To segment an outgoing packet, an application must: > > > > #. Invoke the GSO segmentation API, ``rte_gso_segment()``. > > > > +#. Call ``rte_pktmbuf_free()`` to free mbuf ``rte_gso_segment()`` segm= ents. > > + > > #. If required, update the L3 and L4 checksums of the newly-created se= gments. > > For tunneled packets, the outer IPv4 headers' checksums should also= be > > updated. Alternatively, the application may offload checksum calcul= ation > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c > > index 751b5b6..0d6cae5 100644 > > --- a/lib/librte_gso/rte_gso.c > > +++ b/lib/librte_gso/rte_gso.c > > @@ -30,7 +30,6 @@ > > uint16_t nb_pkts_out) > > { > > struct rte_mempool *direct_pool, *indirect_pool; > > - struct rte_mbuf *pkt_seg; > > uint64_t ol_flags; > > uint16_t gso_size; > > uint8_t ipid_delta; > > @@ -80,13 +79,7 @@ > > return 1; > > } > > > > - if (ret > 1) { > > - pkt_seg =3D pkt; > > - while (pkt_seg) { > > - rte_mbuf_refcnt_update(pkt_seg, -1); > > - pkt_seg =3D pkt_seg->next; > > - } > > - } else if (ret < 0) { > > + if (ret < 0) { > > /* Revert the ol_flags in the event of failure. */ > > pkt->ol_flags =3D ol_flags; > > } > > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h > > index 3aab297..f6694f9 100644 > > --- a/lib/librte_gso/rte_gso.h > > +++ b/lib/librte_gso/rte_gso.h > > @@ -89,8 +89,11 @@ struct rte_gso_ctx { > > * the GSO segments are sent to should support transmission of multi-s= egment > > * packets. > > * > > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefo= re, > > - * when all GSO segments are freed, the input packet is freed automati= cally. > > + * If the input packet is GSO'd, all the indirect segments are attache= d to the > > + * input packet. > > + * > > + * rte_gso_segment() will not free the input packet no matter whether = it is > > + * GSO'd or not, the application should free it after call rte_gso_seg= ment(). > > * > > * If the memory space in pkts_out or MBUF pools is insufficient, this > > * function fails, and it returns (-1) * errno. Otherwise, GSO succeed= s, > > -- > > 1.8.3.1