> -----Original Message----- > From: Ananyev, Konstantin > Sent: Friday, December 5, 2014 6:56 AM > To: Olivier MATZ; Thomas Monjalon > Cc: dev at dpdk.org; Liu, Jijiang > Subject: RE: [dpdk-dev] [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce > PKT_TX_VXLAN_CKSUM > > > > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > > Sent: Thursday, December 04, 2014 1:51 PM > > To: Ananyev, Konstantin; Thomas Monjalon > > Cc: dev at 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 has 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_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; 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 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 > > > 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 > > > happily 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 real > advantage. > > > > 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. > > > > > 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)) > > 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) should > never be up. > > > > > > > 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) > > The thing is that it wouldn't be possible with FVL driver - it has to setup > mutually > exclusive fields for these 2 cases: > 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 always 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
Yes. I agree. PKT_TX_IPV4 /* packet is IPv4 */ 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 precisely. Yes, the comments are different, the PKT_TX_IPV4 has different meanings, but 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
