Re: [dpdk-dev] [PATCH v2 00/17] add TSO support

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Oliver,

 >> I don't see any big changes in the v2 of that patch.
 >>
 >> At least both things that I have concerns about, stay unchanged in
 >> the v2:
 >>
 >> 1) merge physaddr and buf_len in a bitfield - I still think we better
 >> keep physaddr as 64bit field (PATCH 5).

>As nobody reacted to our discussion thread [1] with other arguments, I
>stayed on my initial position:

>- 2 bytes more in the mbuf structure is always good to have
>- no performance impact detected (see my test reports)
>- the 48 bits physical address limit will be reached in several years,
>   and at this time, maybe the cache lines will be 128 bytes? (solving
>   our initial rte_mbuf issue). So let's just deal with current or near
>   future hardware.

As, I understand right now, you don't need these 2 bytes to enable TSO support.
>From other side, if we'll occupy them by some fields now, then in future
we wouldn't be able to expand our phys_addr without re-working mbuf structure again
and breaking backward compatibility.
No doubt, that there are a lot of extra things that needed(wanted) to be put into the mbuf.
And as new HW with extra offload capabilities will come out - that demand will only increase.
That's why I think we wouldn't be able to squeeze all future fields in 64B anyway.
So it seems to me that we would have to expand mbuf to 128B sooner or later and like we it or not.
So I still think we better keep phys_addr 64bits long.
At least for now. 

>> 2) fix_tcp_phdr_cksum() is inside ixgbe TX function, while I think it
>> should be moved out of it to the upper layer (PATCH 11).

>Thomas's answer on this question [2] was to do that way, so I did not
>modify the v1 patches.

> Thomas Monjalon wrote:
>We know at least 2 checksum methods:
>- the standard one
>- the special one for ixgbe TSO
>In Linux ixgbe, checksum is redone in the driver for TSO case.

I don't think we should use linux ixgbe driver as an etalon here.

>We want to compute checksum in the application/stack in order to prevent 
>driver from modifying packet data, that could cause cache miss.
>But the application cannot always know which checksum method to use because it 
>doesn't have to know which driver will process the packet.
>So we have to choose which checksum method can be done in the application 
>without driver processing. It's not an easy choice.

>It seems simpler and reasonable to choose the standard pseudo-header checksum 
>method as it is done in Linux.
>Having a stable and generic API is something important we must target.

As I said: with the current DPDK implementation the upper code would still be different for packets with TCP checksum (without segmentation) and TCP segmentation:
you need to setup different flags in mbuf, with TSO you need to setup l4_len and mss fields inside mbuf, with just checksum - you don't.
So right now upper layer code has to know is it doing TSO or just checksum anyway.
If that's the case why it also can't compute pseudo-checksum in a 2 different ways too?
Also, the patch introduces support TSO only for ixgbe.
I suppose that TSO support for igb can be added in the same way. 
Let's talk about generic API, when we'll have to expand it into different devices.

Konstantin