[ovs-dev] [PATCH] netdev-dpdk: Add mbuf HEADROOM after alignment.

[Lam, Tiago]

On 27/11/2018 16:10, Stokes, Ian wrote:
>> On 27/11/2018 13:57, Stokes, Ian wrote:
>>>> Commit dfaf00e started using the result of dpdk_buf_size() to
>>>> calculate the available size on each mbuf, as opposed to using the
>>>> previous MBUF_SIZE macro. However, this was calculating the mbuf size
>>>> by adding up the MTU with RTE_PKTMBUF_HEADROOM and only then aligning
>>>> to NETDEV_DPDK_MBUF_ALIGN. Instead, the accounting for the
>>>> RTE_PKTMBUF_HEADROOM should only happen after alignment, as per below.
>>>>
>>>> Before alignment:
>>>> ROUNDUP(MTU(1500) + RTE_PKTMBUF_HEADROOM(128), 1024) = 2048
>>>>
>>>> After aligment:
>>>> ROUNDUP(MTU(1500), 1024) + 128 = 2176
>>>>
>>>> This might seem insignificant, however, it might have performance
>>>> implications in DPDK, where each mbuf is expected to have 2k +
>>>> RTE_PKTMBUF_HEADROOM of available space. This is because not only
>>>> some NICs have course grained alignments of 1k, they will also take
>>>> RTE_PKTMBUF_HEADROOM bytes from the overall available space in an
>>>> mbuf when setting up their Rx requirements. Thus, only the "After
>> alignment"
>>>> case above would guarantee a 2k of available room, as the "Before
>>>> alignment" would report only 1920B.
>>>>
>>>> Some extra information can be found at:
>>>> https://mails.dpdk.org/archives/dev/2018-November/119219.html
>>>>
>>>> Note: This has been found by Ian Stokes while going through some
>>>> af_packet checks.
>>>>
>>>
>>> Hi Tiago,, thanks for this, just a query below.
>>>
>>>> Reported-by: Ian Stokes <ian.stokes at intel.com>
>>>> Fixes: dfaf00e ("netdev-dpdk: fix mbuf sizing")
>>>> Signed-off-by: Tiago Lam <tiago.lam at intel.com>
>>>> ---
>>>>  Documentation/topics/dpdk/memory.rst | 20 ++++++++++----------
>>>>  lib/netdev-dpdk.c                    |  6 ++++--
>>>>  2 files changed, 14 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/Documentation/topics/dpdk/memory.rst
>>>> b/Documentation/topics/dpdk/memory.rst
>>>> index c9b739f..3c4ee17 100644
>>>> --- a/Documentation/topics/dpdk/memory.rst
>>>> +++ b/Documentation/topics/dpdk/memory.rst
>>>> @@ -107,8 +107,8 @@ Example 1
>>>>
>>>>   MTU = 1500 Bytes
>>>>   Number of mbufs = 262144
>>>> - Mbuf size = 2752 Bytes
>>>> - Memory required = 262144 * 2752 = 721 MB
>>>> + Mbuf size = 2880 Bytes
>>>> + Memory required = 262144 * 2880 = 755 MB
>>>
>>> These measurements don't deem to take the header and trailer into
>> account for total size when calculating the memory requirements? Is there
>> any particular reason?
>>>
>>> total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>>>
>>> I would expect above to be 262144 * 3008 = 788 MB.
>>
>> Thanks for looking into it, Ian.
>>
>> I hadn't considered that.
>>
>> But I guess you're right, technically, since that will end up being the
>> total size of each mempool element. However, the size of each mbuf is
>> still the 2880B above.
>>
>> Looking into commit 31154f95 (before the first change to this file was
>> applied), I can see that the size of each mbuf would be 2944B (for a 1500B
>> MTU), where each mempool element would have a size of 3008B (the extra 64B
>> is the mp->header_size above). And we are using the 3008B instead of the
>> 2944B as the mbuf size. Was this on purpose to reflect the total memory
>> required?
> 
> Yes, this is on purpose to account for the total memory requirements for a given deployment as memory for the header would also have to be provisioned for.
> 
>>
>> Just wondering though, I can add the same logic anyway, which would change
>> the values to 2880B + 64B = 2944B.
> 
> I think above should be 3008 again, looking at the v1 of this patch it would break down as follows
> 
> total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
> 3008 = 64 + 2880 + 64
> 
> I would think the trailer should be accounted for as well as the header.
> 
> Ian

Fair enough, I'll send v3.