Re: [PATCH v3 0/3] enhance TX checksum command and csum forwarding engine

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

Hi,

On 01/20/2015 02:12 AM, Ananyev, Konstantin wrote:
>>>> I think a good definition would
>>>>     be:
>>>>
>>>>     Packet is IPv4. This flag must be set when using any offload
>>>>     feature (TSO, L3 or L4 checksum) to tell the NIC that the packet
>>>>     is an IPv4 packet.
>>>>
>>>>     That's why I added PKT_TX_IPV4 in the examples.
>>>
>>> I suppose we discussed it several times: both ways are possible.
>>>  From PMD perspective - treating PKT_TX_IPV4 and PKT_TX_IP_CSUM
>>> As mutually exclusive seems a bit more plausible.
>>>  From the upper layer - my understanding, that it is doesn't really matter.
>>> I thought we had an agreement about it in 1.8, no?
>>
>> Indeed, this was already discussed, but there was a lot of pressure
>> for 1.8.0 to push something, even not perfect. The fog around comments
>> shows that the API was not very clearly defined for 1.8.0. If you read
>> the comments of the API, it is impossible to understand when the
>> PKT_TX_IPV4 or PKT_TX_IP_CSUM flags must be set. I would even say
>> more: the only place where the comments bring a valuable information
>> (L4 checksum and TSO) describe the case where PKT_TX_IPV4 and
>> PKT_TX_IP_CSUM are not exclusive...
>>
>> So I will fix that in my coming patch series. Just for information,
>> I'm pretty sure that having PKT_TX_IPV4 and PKT_TX_IP_CSUM as not
>> exclusive flag would not require any change anywhere in the PMDs (even
>> in i40e).
>
> Right now - no.
> Though as I said from PMD perspective having them exclusive is a bit preferable.
> Again, I don't see any big difference from upper layer code.

Sure, it does not make a big difference in terms of code. But
in terms of API, the naming of the flag is coherent to what it is
used for. And it's easier to find a simple definition, like:

  * Packet is IPv4. This flag must be set when using any offload feature
  * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
  * packet.

>> On the contrary, making them exclusive would require to
>> change the ixgbe TSO code because we check.
>
> Hmm, so you are saying there is a bug somewhere  in ixbe_rxtx.c?
> What particular place you are talking about?

Sorry, I spoke too fast. In TSO code, we check PKT_TX_IP_CKSUM (and not
PKT_TX_IPV4 as I thought), so it would work for both methods without
patching the code.

In this case, it means that both approach would not require to
modify the code.

>>>>     *Problem 3*: without using the word "fortville", it is difficult
>>>>     to understand the goal of the flag PKT_TX_UDP_TUNNEL_PKT. Indeed,
>>>>     once PKT_TX_OUTER_IPV4/6 is set, it looks obvious that it's a
>>>>     tunnel packet. I suggest to remove the PKT_TX_UDP_TUNNEL_PKT
>>>>     flag. In linux, the driver doesn't care about the tunnel type,
>>>>     it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations [6].
>>>
>>> It might be obvious that it is a tunnel packet from PKT_TX_OUTER_* is set,
>>> but it is not obvious what type of tunnelling it would be.
>>> FVL HW supports HW TX offloads for different type of tunnelling and
>>> requires that SW provide information about tunnelling type.
>>>  From i40e datasheet:
>>> L4TUNT L4 Tunneling Type (Teredo / GRE header / VXLAN header) indication:
>>> 00b - No UDP / GRE tunneling (field must be set to zero if EIPT equals to zero)
>>> 01b - UDP tunneling header (any UDP tunneling, VXLAN and Geneve).
>>> 10b - GRE tunneling header
>>> As we do plan to support other than UDP tunnelling types, I suppose we'll need to keep
>>> PKT_TX_UDP_TUNNEL_PKT flag.
>>
>> As I've said: in linux, the driver doesn't care about the tunnel type,
>> it always set I40E_TXD_CTX_UDP_TUNNELING for all encapsulations.
>
> Ok, and why it should be our problem?
> We have a lot of things done in a different manner then linux/freebsd kernel drivers,
> Why now it became a problem?

If linux doesn't need an equivalent flag for doing the same thing,
it probably means we don't need it either.

In a performance-oriented software like dpdk, having a flag that we
don't know what the hardware does with, that is not needed in other
drivers of the same harware, that makes the API harder to understand
could be a problem.

Another argument: if we can remove this flag, it would make the
testpmd commands reworkd proposed by Jijiang much more easy to
understand: only a new "csum parse-tunnel on|off" would be required,
and it can be explained in a few words.

I'll try to do some tests on a fortville NIC if I can find one. I'm
curious to see if we can transmit any encapsulation packet (ip in ip,
ip in gre, eth in gre, eth in vxlan, or even a proprietary tunnel).

We should avoid the need to specify the tunnel type in the OUTER
checksum API if we can, else it would limit us to specific
supported protocols.

>>>> I think the following cases should be *forbidden by the API*:
>>>>
>>>> case 9) calculate checksum of in_ip and in_tcp  (was case B.1 in [1])
>>>>
>>>>     mb->outer_l2_len = len(out_eth)
>>>>     mb->outer_l3_len = len(out_ip)
>>>>     mb->l2_len = len(out_udp + vxlan + in_eth)
>>>>     mb->l3_len = len(out_ip)
>>>>     mb->ol_flags |= PKT_TX_IPV4 | PKT_TX_UDP_TUNNEL_PKT | \
>>>>       PKT_TX_IP_CSUM | PKT_TX_UDP_CKSUM;
>>>>     set out_ip checksum to 0 in the packet
>>>>     set out_udp checksum to pseudo header using rte_ipv4_phdr_cksum()
>>>>
>>>>     If we remove the flag PKT_TX_UDP_TUNNEL_PKT, this cannot be
>>>>     supported, but there is no reason to support it as there is
>>>>     already one way to do the same.
>>>>
>>>>     I think the driver should not even look at mb->outer_l2_len
>>>>     and mb->outer_l3_len if no flag PKT_TX_OUTER_* is set.
>>>
>>> Why it should be forbidden?
>>> I admit it might be a bit slower than case 4),
>>> but I think absolutely legal way to setup HW offloads for inner L3/L4.
>>> As I said we need a PKT_TX_UDP_TUNNEL_PKT anyway, so I suppose
>>> PKT_TX_*_TUNNEL_PKT should be an indication is it a tunnel packet or not.
>>> PKT_TX_OUTER_* flags indicate does outer cksum offload is required or not.
>>
>> I don't understand. The result in terms of hardware is exactly the
>> same than case 4). Why should we have 2 different ways for doing the
>> same thing?
>
> If HW supports that capability, why should we forbid it?
> Let user to choose himself what way to use.
> FVL spec lists it as a valid approach.

It is not a hardware feature. Case 4) and case 9) would fill the
hardware registers exactly the same. To me, it's just an API question.

> As one of possible use-cases:  HW VLAN tags insertion for both inner and outer packets.
> FVL can do that, though as I know our PMD doesn't implement it yet.
> For that, we'll need to specify at least:
> outer_l2_len, outer_l3_len, l2_len.
> While PKT_TX_OUTER_* might stay cleared.

If a VLAN flag has to be inserted in outer header, a new flag
PKT_TX_OUTER_INSERT_VLAN would be added. So my specification
would still be correct:

   The driver should look at mb->outer_lX_len only if a
   PKT_TX_OUTER_* flag is present.

>> This is really confusing for an API. Moreover, you said
>> it: it is slower that case 4).
>
> I don't know would be slower then 4) or not for sure.
> That's my guess, based on the fact that for 9) we need to fill 2 descriptors, while fro 4) - only 1.
> Though I didn't measure the difference.
> That's actually one more reason why to allow and support it -
> so people can make sure that on FVL both ways work as expected and measure the difference.
>
> Konstantin


Regards,
Olivier