Re: [PATCH v5 2/3] mbuf:add three TX ol_flags and repalce PKT_TX_VXLAN_CKSUM

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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.

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))


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)


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