From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id ACB9568CD for ; Thu, 4 Dec 2014 23:56:04 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 04 Dec 2014 14:56:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="425234014" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by FMSMGA003.fm.intel.com with ESMTP; 04 Dec 2014 14:45:41 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.144]) by IRSMSX110.ger.corp.intel.com ([169.254.15.55]) with mapi id 14.03.0195.001; Thu, 4 Dec 2014 22:56:01 +0000 From: "Ananyev, Konstantin" To: Olivier MATZ , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM Thread-Index: AQHQDu5Csn4rdRtSMEuKWiThOnkfhJx9OYaAgAAV4YCAABxsgIABPOtwgACRSjCAAAbgAIAAA4QwgAAwZICAAJFe8A== Date: Thu, 4 Dec 2014 22:56:00 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213BCC7B@IRSMSX105.ger.corp.intel.com> References: <1417532767-1309-1-git-send-email-jijiang.liu@intel.com> <1ED644BD7E0A5F4091CF203DAFB8E4CC01D9FF2B@SHSMSX101.ccr.corp.intel.com> <2601191342CEEE43887BDE71AB977258213BC6F2@IRSMSX105.ger.corp.intel.com> <2324692.x6b6svf072@xps13> <2601191342CEEE43887BDE71AB977258213BC7F9@IRSMSX105.ger.corp.intel.com> <548066C5.4020008@6wind.com> In-Reply-To: <548066C5.4020008@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Dec 2014 22:56:05 -0000 > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Thursday, December 04, 2014 1:51 PM > To: Ananyev, Konstantin; Thomas Monjalon > Cc: dev@dpdk.org; Liu, Jijiang > Subject: Re: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and rep= alce PKT_TX_VXLAN_CKSUM >=20 > Hi, >=20 > On 12/04/2014 12:03 PM, Ananyev, Konstantin wrote: > >>>>> 1/ (Jijiang's patch) > >>>>> PKT_TX_IP_CKSUM /* packet is IPv4, and we want hw cksum */ > >>>>> PKT_TX_IPV6 /* packet is IPv6 */ > >>>>> PKT_TX_IPV4 /* packet is IPv4, and we don't want hw cksum */ > >>>>> > >>>>> with PKT_TX_IP_CKSUM and PKT_TX_IPV4 exclusive > >>>>> > >>>>> and > >>>>> > >>>>> 2/ > >>>>> PKT_TX_IP_CKSUM /* we want hw IP cksum */ > >>>>> PKT_TX_IPV6 /* packet is IPv6 */ > >>>>> PKT_TX_IPV4 /* packet is IPv4 */ > >>>>> > >>>>> with PKT_TX_IP_CKSUM implies PKT_TX_IPV4 > >>>>> > >>>>> > >>>>> Solution 2/ looks better from a user point of view. Anyone else has= an opinion? > >>>> > >>>> Let's think about these IPv4/6 flags in terms of checksum and IP ver= sion/type, > >>>> > >>>> 1. For IPv6 > >>>> IP checksum is meaningful only for IPv4, so we define 'PKT_TX_IPV6 = /* packet is IPv6 */' to tell driver/HW that this is IPV6 > >> packet, > >>>> here we don't talk about the checksum for IPv6 as it is meaningless.= Right? > >>>> > >>>> PKT_TX_IPV6 /* packet is IPv6 */ ------ IP type: v6; H= W checksum: meaningless > >>>> > >>>> 2. For IPv4, > >>>> My patch: > >>>> > >>>> PKT_TX_IP_CKSUM /* packet is IPv4, and we want hw cksum */---------= -----------------IP type: v4; HW checksum: Yes > >>>> PKT_TX_IPV4 /* packet is IPv4, and we don't want hw cksum */ --= --------------------- IP type: v4; HW checksum: No > >>>> > >>>> You want: > >>>> PKT_TX_IP_CKSUM /* we want hw IP cksum */--------------------------= IP type: v4; HW checksum: Yes > >>>> PKT_TX_IPV4 /* packet is IPv4*/ ------------------------ IP ty= pe: v4; HW checksum: yes or no? > >>>> = driver/HW don't know, just know this is= packet with IPv4 header. > >>>> = HW checksum: meaningless?? > >>> > >>> Yep, that's why I also don't like that suggestion: PKT_TX_IPV4 itself= doesn't contain all information. > >>> PMD will have to check PKT_TX_IP_CKSUM anyway. > >> > >> I prefer solution 2 because a flag should bring only 1 information. > > > > Why is that? For example in mbuf we already have a flag that brings 2 t= hings: > > PKT_TX_IP_CKSUM /* packet is IPv4, and we want hw cksum */ >=20 > For the user, it's clearer to have one information in a flag. > If you just look at the name of the flag, the natural meaning is 2/, > else we would need to rename them in: > PKT_TX_IPV4_CKSUM > PKT_TX_IPV4_NO_CKSUM >=20 > > If it would be possible to compress 10 meanings into 1 bit, I would hap= pily do that. > > Unfortunately, it is rarely possible :) > > > >> It's simply saner and could fit to more situations in the future. > > > > Could you give an example of such situation? > > I personally couldn't come up with the case where #2 would have any rea= l advantage. >=20 > in solution 2/, PKT_TX_IP_CKSUM implies PKT_TX_IPV4 so checking > PKT_TX_IP_CKSUM is still enough in drivers. Both 1 and 2 seems backward compatible. >=20 > In the driver, it is also simpler. With solution 1/: >=20 > /* check if we need ipcsum */ > if (flags & PKT_TX_IP_CKSUM) >=20 > /* check if packet is ipv4, may be needed to set a hw field */ > if (flags & (PKT_TX_IP_CKSUM|PKT_TX_IPV4)) Do you really mean 1 here? When all 3 flags are mutually exclusive? If so, it doesn't look right. For 1 both (PKT_TX_IP_CKSUM|PKT_TX_IPV4) shou= ld never be up. =20 >=20 >=20 > With solution 2/ >=20 > /* check if we need ipcsum */ > if (flags & PKT_TX_IP_CKSUM) >=20 > /* check if packet is ipv4, may be needed to set a hw field */ > if (flags & PKT_TX_IPV4) The thing is that it wouldn't be possible with FVL driver - it has to setup= mutually exclusive fields for these 2 cases:=20 PKT_TX_IP_CKSUM - ipv4 with HW checksum PKT_TX_IPV4 - ipv4 without HW checksum So with #2, driver has either: if (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} And always keep condition for PKT_TX_IP_CKSUM first. Or do: if (flags & PKT_TX_IPV4) {...} if (flags & PKT_TX_IP_CKSUM) {...} and in that case always keep condition for PKT_TX_IP_CKSUM last, so it alwa= ys overwrite PKT_TX_IPV4 settings. Basically with #2 PKT_TX_IPV4 is not enough to make a decision, even if it = is set, we'll have to check for PKT_TX_IP_CKSUM anyway. While with 1 we can put them in any order, both: If (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} And If (flags & PKT_TX_IPV4) {...} else if (flags & PKT_TX_IP_CKSUM) {...} Will work. Konstantin >=20 >=20 > I agree it can looks like a detail, but I really think it's important > to have the most logical and straightforward api for mbuf, as it's > the core of DPDK. >=20 > Regards, > Olivier