From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id E30507DF4 for ; Fri, 5 Dec 2014 05:18:02 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 04 Dec 2014 20:17:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,520,1413270000"; d="scan'208";a="642754110" Received: from kmsmsx153.gar.corp.intel.com ([172.21.73.88]) by fmsmga002.fm.intel.com with ESMTP; 04 Dec 2014 20:17:45 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by KMSMSX153.gar.corp.intel.com (172.21.73.88) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 5 Dec 2014 12:17:39 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.110]) by shsmsx102.ccr.corp.intel.com ([169.254.2.216]) with mapi id 14.03.0195.001; Fri, 5 Dec 2014 12:17:37 +0800 From: "Liu, Jijiang" To: "Ananyev, Konstantin" , 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: AQHQDkIzZMvV7Xzwn0SVLY5kK7hoh5x9OYaAgAAV4YCAABxsgIABPOtwgAANVYCAAAYRAIAABRyAgAAuzICAAJhEAIAA1yFg Date: Fri, 5 Dec 2014 04:17:37 +0000 Message-ID: <1ED644BD7E0A5F4091CF203DAFB8E4CC01DA0472@SHSMSX101.ccr.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> <2601191342CEEE43887BDE71AB977258213BCC7B@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258213BCC7B@IRSMSX105.ger.corp.intel.com> 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 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: Fri, 05 Dec 2014 04:18:04 -0000 > -----Original Message----- > From: Ananyev, Konstantin > Sent: Friday, December 5, 2014 6:56 AM > To: Olivier MATZ; 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 >=20 >=20 > > -----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 > > repalce PKT_TX_VXLAN_CKSUM > > > > Hi, > > > > 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 h= as an > opinion? > > >>>> > > >>>> Let's think about these IPv4/6 flags in terms of checksum and IP > > >>>> version/type, > > >>>> > > >>>> 1. For IPv6 > > >>>> IP checksum is meaningful only for IPv4, so we define 'PKT_TX_IPV= 6 /* > 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 meaningles= s. > Right? > > >>>> > > >>>> PKT_TX_IPV6 /* packet is IPv6 */ ------ IP type: v6; = HW 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 = type: 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 itse= lf 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= things: > > > PKT_TX_IP_CKSUM /* packet is IPv4, and we want hw cksum */ > > > > 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 > > > > > If it would be possible to compress 10 meanings into 1 bit, I would h= appily 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 r= eal > advantage. > > > > in solution 2/, PKT_TX_IP_CKSUM implies PKT_TX_IPV4 so checking > > PKT_TX_IP_CKSUM is still enough in drivers. >=20 > Both 1 and 2 seems backward compatible. >=20 > > > > In the driver, it is also simpler. With solution 1/: > > > > /* check if we need ipcsum */ > > if (flags & PKT_TX_IP_CKSUM) > > > > /* check if packet is ipv4, may be needed to set a hw field */ if > > (flags & (PKT_TX_IP_CKSUM|PKT_TX_IPV4)) >=20 > 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) sh= ould > never be up. >=20 > > > > > > With solution 2/ > > > > /* check if we need ipcsum */ > > if (flags & PKT_TX_IP_CKSUM) > > > > /* check if packet is ipv4, may be needed to set a hw field */ if > > (flags & PKT_TX_IPV4) >=20 > The thing is that it wouldn't be possible with FVL driver - it has to set= up mutually > exclusive fields for these 2 cases: > PKT_TX_IP_CKSUM - ipv4 with HW checksum > PKT_TX_IPV4 - ipv4 without HW checksum >=20 > So with #2, driver has either: > if (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} An= d 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 always overwrite > PKT_TX_IPV4 settings. >=20 > Basically with #2 PKT_TX_IPV4 is not enough to make a decision, even if i= t is set, > we'll have to check for PKT_TX_IP_CKSUM anyway. >=20 > While with 1 we can put them in any order, both: > If (flags & PKT_TX_IP_CKSUM) {...} else if (flags & PKT_TX_IPV4) {...} An= d If (flags > & PKT_TX_IPV4) {...} else if (flags & PKT_TX_IP_CKSUM) {...} Will work. >=20 > Konstantin Yes. I agree. PKT_TX_IPV4 /* packet is IPv4 */=20 This flag don't have too much offload meaning for TX side, because we can'= t use this information to set transmit descriptor ( or set offload register= ) precisely , so it is not a real offload flag. PKT_TX_IPV4 /* packet is IPv4, and we don't want hw cksum */ It is a offload flag, we can use this flag to set transmit descriptor pre= cisely. =20 Yes, the comments are different, the PKT_TX_IPV4 has different meanings, bu= t we have to consider which comment will affect if offload work or not. BTW, we pay too much time on this topic... > > > > > > 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. > > > > Regards, > > Olivier