From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <konstantin.ananyev@intel.com>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
by dpdk.org (Postfix) with ESMTP id ACB9568CD
for <dev@dpdk.org>; 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" <konstantin.ananyev@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>, Thomas Monjalon
<thomas.monjalon@6wind.com>
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" <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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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