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 <[email protected]> >>>> Fixes: dfaf00e ("netdev-dpdk: fix mbuf sizing") >>>> Signed-off-by: Tiago Lam <[email protected]> >>>> --- >>>> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
