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 13BA7A04DC; Mon, 19 Oct 2020 10:48:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D491EBE4D; Mon, 19 Oct 2020 10:48:14 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 88EACBE48 for ; Mon, 19 Oct 2020 10:48:11 +0200 (CEST) IronPort-SDR: SESXMuci0u8UtvzowIzLftB/s5GrXS1qI9eX7pvOmpmm/yKk5ntmh2igxh2LlWjovlThf+VnPC I2SamY505D8Q== X-IronPort-AV: E=McAfee;i="6000,8403,9778"; a="166213652" X-IronPort-AV: E=Sophos;i="5.77,394,1596524400"; d="scan'208,217";a="166213652" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2020 01:48:09 -0700 IronPort-SDR: zDw7DUNOLVelWoFnaYMFPg4XMpjpJ+RSGK3sRqXlnVKtQKp39zYVtcv0LO9FegWbERTggUTNQW VqhfRrsHEM2w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,394,1596524400"; d="scan'208,217";a="347362955" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmsmga004.fm.intel.com with ESMTP; 19 Oct 2020 01:48:09 -0700 Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 19 Oct 2020 01:48:08 -0700 Received: from orsmsx601.amr.corp.intel.com (10.22.229.14) by ORSMSX612.amr.corp.intel.com (10.22.229.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 19 Oct 2020 01:48:08 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 19 Oct 2020 01:48:08 -0700 Received: from NAM02-SN1-obe.outbound.protection.outlook.com (104.47.36.58) 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; Mon, 19 Oct 2020 01:48:07 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YpuqYhwkSBTyJeStycc2XQVQrXrgJNbJGvJ4ojIcgLi4wtZYCSIfVc8D7kT6DWmsKgBtvBwzpTGwpeFOWtFcgg7WjVboqbhKBsqeZXQSaPjpIEI7gpRy33yPaHj1TYxhJCg2R2YVbY1bhJTKHjtDQrjURwadY/q2d3uTPjr8U3BaEYmW/6SAdNYSOLJUrGImVin/bBhKCEyiOCY9pK4lhSYUbRap2mxp5QUJ8bfB9TD9oJAZn0mw3Mtq9AudbzLlaInVrb6EiSFCWZXKKk0/wjJ1O3K0xa9JnJd0r8YKVYulijU8M7clKkswhgEZjvMgixWYIQTDsJybckWQMWjL0g== 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=AHOvtHEa/smURfnsqczRYdlzfWkc15IopHDtOSPrXic=; b=IbjGpLSZhsht0wtgpeiilHKmIsgELfgv+rNVoxvMYRP6J0OsXm1mwP8E1h5pW7dbTk0D0go2KCXjxfWxD8HYo1o9zO7x1RnU6sG2Tuon5rc6ueR+HmBbwBf9MpEBxYCR6i86yYX9ccd9MuiZpR6zBrGyPLIWDOMdQ/DTH18FopXz+rTL8RT1CRRsHaWOLLxsoNA5srfmQOj5Pzs8bYB9Wm+RccErgZOlWyrW1m8Zbk2J+JCbBPKZj4TOpuKgK0gbJFR1bYp0PG3ekGSdh15de7OXWOr/g5RKC0GN5HFfYk7CNKA5quOX2pJZXw9f44n6Q6spuja47sNxNItoez7m+g== 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=AHOvtHEa/smURfnsqczRYdlzfWkc15IopHDtOSPrXic=; b=LJubJvzv3PjAYMysLlOTWZeDM9sSrGmp6qHC1/uYxfxJ1r2pQm7RULHRCQa6Jwt0Z+BSqdWySM89gpZaK6JG3zWTpUoysslzeYc+mHHz69oeNF1VhLUP3+UAQSYqqaQ89pzyVaxIag2lQivmaj9fcUn8eJ4rW/Yx63ZHQoPdWT8= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BYAPR11MB3448.namprd11.prod.outlook.com (2603:10b6:a03:76::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3477.28; Mon, 19 Oct 2020 08:47:56 +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.3477.028; Mon, 19 Oct 2020 08:47:55 +0000 From: "Ananyev, Konstantin" To: yang_y_yi , "Hu, Jiayu" CC: "dev@dpdk.org" , "olivier.matz@6wind.com" , "thomas@monjalon.net" , "yangyi01@inspur.com" Thread-Topic: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to Thread-Index: AQHWnrL85KBHJVyvm02xWITKaS0ypKmVJ4OAgACCzBCAAKMIAIAAIGYAgACLbLCAAS2VgIAAt6cAgACRmACAAHz1MIAEYn8AgAA514CAACInsA== Date: Mon, 19 Oct 2020 08:47:55 +0000 Message-ID: References: <20201010031020.349516-1-yang_y_yi@163.com> <43f71e6c9d2f4d5ba3ab56a921c5912d@intel.com> <6167423037704e3382f85275be79de30@intel.com> <9239b2e.2116.17525095531.Coremail.yang_y_yi@163.com> <29f24642c9fe498db60c8c3fa6ec0f1d@intel.com> <54283e4e.3fce.1753f9a7772.Coremail.yang_y_yi@163.com> In-Reply-To: <54283e4e.3fce.1753f9a7772.Coremail.yang_y_yi@163.com> 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: 8a51829d-37ed-4bc2-b036-08d8740bac76 x-ms-traffictypediagnostic: BYAPR11MB3448: 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:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: sj0A8K9Plq9iTnEnGMDx1/Ta0IRPbVXwpCDF8z2C8N8LWH7rGBy7YqMHfWhQZXqphSViW7FBmGG8PJVZbNN3bUNAwh76xC9saAuhJSB3pG8W+uMe2XuRW4y0pTG04rovdxZQHR2SFJLOn1w03yN7NIQ2HTWHvKsUhcCX11KQPpMmoEpsrMssH57LWNsCFIclq09086wy3BxiaURw/OYhRLscJzmcKvL9oZvgtyLDsmriaD8STboa47bMR8omqduFX3chcXYsZExPOpKKVh1AHmZve/o3RS/XdgjR1NL9EKTgYYy7DAHd0pn1YFx2meQLOEf1ZepeYHFS71LK354xMA== 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)(39860400002)(396003)(366004)(136003)(346002)(376002)(4326008)(7696005)(26005)(2906002)(186003)(6506007)(53546011)(9686003)(6636002)(86362001)(478600001)(8676002)(8936002)(55016002)(316002)(54906003)(110136005)(76116006)(4001150100001)(52536014)(5660300002)(66446008)(33656002)(66556008)(66476007)(64756008)(66946007)(71200400001)(83380400001); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: a7BqNKSfDjf4sYaHc+BSUoMbcKh5EIvoFUVMg3HGVHH8LOZEW6lCLOpNcd4OZxBLr6OLl3zxnDWjy/rmkUi/QgrKVWwwqCLAK7n9OsJGhMGHN5uY4UvexRyofh0ap9sM/E5TMkx0F+33DbLLHNB+QHAzK86kySUnprzLP9N0mk7TsYa5FTE7LxU+bJKahPIlCz1fUfdM6g//cAixpEql8ynjysHmI8SlOHpH6Ws4Z5bJmuBDVOtnuLMXKZdck8oY2zf6v6k2bgPBrsju5r9DNvnrCV8Vt6nXDa8sn2e419qL2jf+KLoSAvQKPGksIp1qDfgz4aQQCPobNpbRT7X74BYPek85+aIFjdjfj3eALR/uJgTvQq5b0bCdCNi3AlReFqVoLC8DeLG7mKVPI9I6w2JIWiM/UPIDHAVjrb5NeE7Cv5VkYEW21Nh35zTCSPdVmLVNQRncPR5Hirw0DUsd7bcdA4Ql4VY5Ms2emn5X2kEKQnyYqxUzj70AtCXX7sYTws7bqOYHsDav0QZQb8XaYtPfaS5saoip3NPlNbGVozzi8sXgmAa8o5M+xpk18qQyaJ7cekn1aP6jR7C/1t+Lnh8APfYNDwYNdt51jvwlAvvIoYVTaHguef1pQa8JewNYpwaoZVzvCgeAXo/KPxgOOA== 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: 8a51829d-37ed-4bc2-b036-08d8740bac76 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Oct 2020 08:47:55.8687 (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: dmIHcRwdAFOjCEOljbyndDdgVQ/vLEoesIA5x4iGC34c0oVTu48KyL8BTOkYnK5sRbiW6LISViiWhljtCKA2/UwF8XIAuIPlNryGNH/R+kY= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3448 X-OriginatorOrg: intel.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] 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" > -----Original Message----- > From: Ananyev, Konstantin > Sent: Monday, October 19, 2020 9:44 AM > To: Ananyev, Konstantin > Subject: FW: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segmen= ts attach to > > > > From: yang_y_yi > > Sent: Monday, October 19, 2020 7:45 AM > To: Hu, Jiayu > > Cc: Ananyev, Konstantin >; dev@dpdk.org; olivier.matz@6wind.c= om; thomas@monjalon.net; > yangyi01@inspur.com > Subject: Re:RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments a= ttach to > > At 2020-10-19 11:17:48, "Hu, Jiayu" wrote: > > > > > >> -----Original Message----- > >> From: Ananyev, Konstantin > >> Sent: Friday, October 16, 2020 4:31 PM > >> To: Hu, Jiayu ; yang_y_yi > >> Cc: mailto:dev@dpdk.org; mailto:olivier.matz@6wind.com; mailto:thomas@= monjalon.net; > >> mailto:yangyi01@inspur.com > >> Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments a= ttach to > >> > >> > >> > >> > > >> > > > > > > >> > > > > > I think it isn't a good idea to free it in rte_gso_segment, = maybe > >> > > application > >> > > > > will continue to use this pkt for other purpose, rte_gso_segme= nt > >> > > > > > can't make decision for application without any notice, it i= s better to > >> > > return > >> > > > > this decision right backt to application. > >> > > > > > > >> > > > > > >> > > > > I think, if user wants to keep original packet, he can always = call > >> > > > > rte_pktmbuf_refcnt_update(pkt, 1) > >> > > > > just before calling gso function. > >> > > > > > >> > > > > Also, as I remember in some cases it is not safe to do free() = for input > >> > > packet > >> > > > > (as pkt_out[] can contain input pkt itself). Would it also be = user > >> > > responsibility > >> > > > > to determine > >> > > > > such situations? > >> > > > > >> > > > In what case will pkt_out[] contain the input pkt? Can you give = an > >> example? > >> > > > >> > > As I can read the code, whenever gso code decides that > >> > > no segmentation is not really needed, or it is not capable > >> > > of doing it properly. > >> > > Let say: > >> > > > >> > > gso_tcp4_segment(struct rte_mbuf *pkt, > >> > > uint16_t gso_size, > >> > > uint8_t ipid_delta, > >> > > struct rte_mempool *direct_pool, > >> > > struct rte_mempool *indirect_pool, > >> > > struct rte_mbuf **pkts_out, > >> > > uint16_t nb_pkts_out) > >> > > { > >> > > ... > >> > > /* Don't process the fragmented packet */ > >> > > ipv4_hdr =3D (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt,= char *) + > >> > > pkt->l2_len); > >> > > frag_off =3D rte_be_to_cpu_16(ipv4_hdr->fragment_offset); > >> > > if (unlikely(IS_FRAGMENTED(frag_off))) { > >> > > pkts_out[0] =3D pkt; > >> > > return 1; > >> > > } > >> > > > >> > > /* Don't process the packet without data */ > >> > > hdr_offset =3D pkt->l2_len + pkt->l3_len + pkt->l4_len; > >> > > if (unlikely(hdr_offset >=3D pkt->pkt_len)) { > >> > > pkts_out[0] =3D pkt; > >> > > return 1; > >> > > } > >> > > > >> > > That's why in rte_gso_segment() we update refcnt only when ret > 1= . > >> > > >> > But in these cases, the value of ret is 1. So we can free input pkt = only when > >> > ret > 1. Like: > >> > > >> > - 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 > 1) > >> > + rte_pktmbuf_free(pkt); > >> > + else if (ret < 0) { > >> > /* Revert the ol_flags in the event of failure. */ > >> > pkt->ol_flags =3D ol_flags; > >> > } > >> > >> Yes, definitely. I am not arguing about that. > >> My question was to the author of the original patch: > >> He suggests not to free input packet inside gso function and leave it = to the > >> user. > >> So, in his proposition, would it also become user responsibility to de= termine > >> when input packet can be freed (it is not present in pkt_out[]) or not= ? > > > >@Yi, I am OK with the both designs. If you think it's better to free the= input pkt by > >users, you can keep the original design. But one thing to notice is that= you need > >to update definition of rte_gso_segment() in rte_gso.h too. > > > >Thanks, > >Jiayu > > Ok, I prefer to handle it by users, this is incremental patch for rte_gso= _segment description. Is it ok to you? > > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h > index 3aab297..3762eba 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-seg= ment > * packets. > * > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore= , > - * when all GSO segments are freed, the input packet is freed automatica= lly. > + * If the input packet is GSO'd, all the indirect segments are attached = 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_segme= nt(). > * > * If the memory space in pkts_out or MBUF pools is insufficient, this > * function fails, and it returns (-1) * errno. Otherwise, GSO succeeds, I think such change will require much more than that. While formally API is not changed, due to behaviour change, API users will need to change their code, right? If so, I think such change needs to be treated as API breakage. So I think if you'll choose to go ahead with your original approach, you'll need to: 1. Change all places in dpdk.org that uses rte_gso_segment() to make them work correctly with new behaviour. 2. Usually such change has to be declared at least one release in advance v= ia deprecation notice. In 20.11 we do allow some API changes without deprecation notice, but all such changes have to be reviewed by dpdk tech-board. So don't forget to CC your patch to TB for review, and explain clearly changes required in API caller code. Konstantin