Re: [dpdk-dev] [PATCH] test-pmd fix default mbuf size

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

Hi Konstantin,

On 04/29/2015 12:39 PM, Ananyev, Konstantin wrote:
> Hi Olivier,
>
>> -----Original Message-----
>> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
>> Sent: Wednesday, April 29, 2015 10:55 AM
>> To: Ananyev, Konstantin; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] test-pmd fix default mbuf size
>>
>> Hi Konstantin,
>>
>> On 04/28/2015 03:02 PM, Konstantin Ananyev wrote:
>>> Latest mbuf changes (priv_size addition and related fixes)
>>> exposed small problem with testpmd default config:
>>> testpmd default mbuf size is exaclty 2KB, that causes
>>> ixgbe PMD to select scattered RX even for configs with 'normal'
>>> max packet length (max_rx_pkt_len == ETHER_MAX_LEN).
>>>
>>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> ---
>>>    app/test-pmd/testpmd.h | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>>> index 389fc24..037e7ec 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -48,7 +48,8 @@
>>>     * Default size of the mbuf data buffer to receive standard 1518-byte
>>>     * Ethernet frames in a mono-segment memory buffer.
>>>     */
>>> -#define DEFAULT_MBUF_DATA_SIZE 2048 /**< Default size of mbuf data buffer. */
>>> +#define DEFAULT_MBUF_DATA_SIZE (2048 + RTE_PKTMBUF_HEADROOM)
>>> +/**< Default size of mbuf data buffer. */
>>>
>>>    /*
>>>     * The maximum number of segments per packet is used when creating
>>>
>>
>> Indeed, this regression is introduced by one of my recent
>> patch:
>> http://dpdk.org/browse/dpdk/commit/?id=dfb03bbe2b39156f0e42e7f29e09c1e6b6def265
>>
>> Before, m->buf_len was initialized to RTE_PKTMBUF_HEADROOM + 2048.
>> Now it is set to 2048.
>>
>> Maybe a line like this should be added in the commit log:
>> Fixes: dfb03bbe2b ("app/testpmd: use standard functions to initialize
>> mbufs and mbuf pool")
>>
>> Just one question Konstantin: could you just confirm that the
>> reason of the bug? From what I understand:
>> - buf_len is 2048
>> - the rx data size when receiving a packet is 2048 - hdroom = 1920
>> - at init, the ixgbe driver configures the hw to set the max rx
>>     data size, but it has to be a power of 2, so 1024 is chosen
>
> At ixgbe_dev_rx_init(), we need to setup  SRRCTL[rqx_id]. BSIZEPACKET value:
>
> "BSIZEPACKET 4:0 0x2 Receive Buffer Size for Packet Buffer.
> The value is in 1 KB resolution. Value can be from 1 KB to 16 KB. Default buffer size is
> 2 KB. This field should not be set to 0x0. This field should be greater or equal to 0x2
> in queues where RSC is enabled."
>
> As it is it in 1KB units, our 1920 B are rounded down to 1K, and we have to enable scatter RX mode.
>
> As I understand, same story for igb devices.
> I40e seems doesn't have such limitation.
>
>> - then the initialization code check if a packet of ETHER_MAX_LEN
>>     fits in max rx size, and if not, it selects the scatter mode.
>>
>> It makes me wondering 2 things:
>> - should we add a comment in the test-pmd to explain that? (maybe
>>     not, as it is driver-specific, and it is just an optimization)
>
> Might be, or probably somewhere to the docs?
>
>> - should we check the other examples to see if the same problem
>>     exists?
>
> Good point.
> I did a quick check, and yes it seems few other sample apps are also affected:
>
> examples/qos_sched/main.h:#define MBUF_DATA_SIZE (1528 + RTE_PKTMBUF_HEADROOM)
> examples/skeleton/basicfwd.c:#define MBUF_DATA_SIZE (1600 + RTE_PKTMBUF_HEADROOM)
> examples/packet_ordering/main.c:#define MBUF_DATA_SIZE (1600 + RTE_PKTMBUF_HEADROOM)
> examples/rxtx_callbacks/main.c:#define MBUF_DATA_SIZE (1600 + RTE_PKTMBUF_HEADROOM)
>
> I suppose, have to make v2 to include all of the above...
> What probably is more beneficial  - inside rte_mbuf.h:
>
> #define RTE_MBUF_DEFAULT_DATA_SIZE (2048 + RTE_PKTMBUF_HEADROOM)
>
> With some good comments for it, and make all appropriate examples to use it.
> Again, then it would appear in the API reference automatically.
> Konstantin

Yes, good idea, it would solve the doc issue and it factorizes the
default mbuf data size somewhere.

Regards,
Olivier





>
>>
>> If my understanding is correct,
>> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>>
>> Regards,
>> Olivier
>