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

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Venky,

>> my testpmd iofwd test with the txqflags option disabling many mbuf
>> features is not representative of a real world application.

> [Venky] I did see your test reports. I also made the point that the
> tests we have are insufficient for testing the impact. If you look at
> data_ofs, it actually has an impact on two sides - the driver and the
> upper layer. We do not at this time have a test for the upper
> layer/accessor. Secondly, there is a whole class of apps (fast path
> route for example) that aren't in your purview that do not need
> txqflags. Calling it not representative of a real world application is
> incorrect.

I was talking about "iofwd with txqflags", not "txqflags". The iofwd
mode means that the mbuf data is not accessed by the application,
so I think we should not rely on it.

I agree that "txqflags" could be useful in some situations, when the
user does not need multi-segment. Anyway, the tests I've posted on the
list do not show a real impact on this use case.

Finally, as stated in the initial TSO cover letter [1], I've tested
the upper layer impact that you are talking about with the 6WINDGate
stack and there is no visible regression.

> Secondly, your testpmd run baseline performance should be higher. At
> this point, it is about 40% off from the numbers we see on the baseline
> on the same CPU. If the baseline is incorrect, I cannot judge anything
> more on the performance. We need to get the baseline performance the
> same, and then compare impact.

I tested with the default testpmd configuration, I though it was
close enough to the best performance.

> [Venky] I exclude VLAN because it is something explicitly set by the Rx
> side of the driver. Having Rx access a second cache line will generate a
> performance impact (can be mitigated by a prefetch, but it will cost
> more instructions, and cannot be deterministically controlled). The rest
> of the structure is on the transmit side - which is going to be cache
> hot - at least in LLC anyway. There are cases where this will not be in
> LLC - and we have a few of those. Those however, we can mitigate.

If we add another cache line to the mbuf, if the code accesses it
(whatever rx or tx side), it will at least increase the required memory
bandwidth, and in the worst case will result in an additional cache
miss. This is difficult to predict and depends on the use case. I
think each solution would have its drawbacks in specific cases.

I think the tests I've provided are representative enough to assume
that there is no need to search for an alternative, knowing the
fact that it also clean the mbuf structure and give more room for
later use.

> [Venky] I don't think reworking core data structures (especially
> regressing core data structures) is a good thing. We have kept this
> relatively stable over 5 releases, sometimes at the impact of
> performance, and churning data structures is not a good thing.

Sorry, but I don't think this is a good argument: we cannot say
"it was stable during 5 releases so we cannot modify it". I would say
that the code modifications should be guided by technical reasons.

Keeping the API stable could be a good technical reason, but as far
as I understood, the DPDK is not at this stage today. The mbuf
rework brings 9 additional bytes in the structure, which help for
TSO today, but may also help for next features. For instance, having
another checksum flag as proposed by Stephen and me [2].

Moreover, the patches make the rte_mbuf structure clearer: the current
separation rte_mbuf / rte_pktmbuf is a bit strange, for instance the
ol_flags field which is clearly related to a pktmuf is located in
the rte_mbuf.


Now, I'm ready to make some concessions and discuss about an alternative
solution.
Thomas, you are the maintainer ;) what are your plans?

Regards,
Olivier


[1] http://dpdk.org/ml/archives/dev/2014-May/002322.html
[2] http://dpdk.org/ml/archives/dev/2014-May/002339.html