Re: [dpdk-dev] [RFC] ethdev: introduce Rx buffer split

[Andrew Rybchenko <Andrew.Rybchenko@oktetlabs.ru>]

Hi Slava,

I'm sorry for late reply. See my notes below.

On 10/1/20 11:54 AM, Slava Ovsiienko wrote:
> Hi, Andrew
>
> Thank you for the comments, please see my replies below.
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Thursday, September 17, 2020 19:55
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
>> Cc: Thomas Monjalon <thomasm@mellanox.com>;
>> stephen@networkplumber.org; ferruh.yigit@intel.com; Shahaf Shuler
>> <shahafs@nvidia.com>; olivier.matz@6wind.com; jerinjacobk@gmail.com;
>> maxime.coquelin@redhat.com; david.marchand@redhat.com; Asaf Penso
>> <asafp@nvidia.com>
>> Subject: Re: [dpdk-dev] [RFC] ethdev: introduce Rx buffer split
>>
> [snip]
>>>
>>> For example, let's suppose we configured the Rx queue with the
>>> following segments:
>>> seg0 - pool0, len0=14B, off0=RTE_PKTMBUF_HEADROOM
>>> seg1 - pool1, len1=20B, off1=0B
>>> seg2 - pool2, len2=20B, off2=0B
>>> seg3 - pool3, len3=512B, off3=0B
>>>
>>> The packet 46 bytes long will look like the following:
>>> seg0 - 14B long @ RTE_PKTMBUF_HEADROOM in mbuf from pool0
>>> seg1 - 20B long @ 0 in mbuf from pool1
>>> seg2 - 12B long @ 0 in mbuf from pool2
>>>
>>> The packet 1500 bytes long will look like the following:
>>> seg0 - 14B @ RTE_PKTMBUF_HEADROOM in mbuf from pool0
>>> seg1 - 20B @ 0 in mbuf from pool1
>>> seg2 - 20B @ 0 in mbuf from pool2
>>> seg3 - 512B @ 0 in mbuf from pool3
>>> seg4 - 512B @ 0 in mbuf from pool3
>>> seg5 - 422B @ 0 in mbuf from pool3
>>
>> The behaviour is logical, but what to do if HW can't do it, i.e. use
>> the last
>> segment many times. Should it reject configuration if provided
>> segments are
>> insufficient to fit MTU packet? How to report the limitation?
>> (I'm still trying to convince that SCATTER and BUFFER_SPLIT should be
>> independent).
>
> BUFFER_SPLIT is rather the way to tune SCATTER. Currently scattering
> happens on unconditional mbuf data buffer boundaries (we have reserved
> HEAD space in the first mbuf and fill this one to the buffer end,
> the next mbuf buffers might be filled completely). BUFFER_SPLIT provides
> the way to specify the desired points to split packet, not just blindly
> follow buffer boundaries. There is the check inplemented in common part
> if each split segment fits the mbuf allocated from appropriate pool.
> PMD should do extra check internally whether it supports the requested
> split settings, if not - call will be rejected.
>

@Thomas, @Ferruh: I'd like to hear what other ethdev
maintainers think about it.

> [snip]
>>
>> I dislike the idea to introduce new device operation.
>> rte_eth_rxconf has reserved space and BUFFER_SPLIT offload will mean that
>> PMD looks at the split configuration location there.
>>
> We considered the approach of pushing split setting to the rxconf
> structure.
> [http://patches.dpdk.org/patch/75205/]
> But it seems there are some issues:
>
> - the split configuration description requires the variable length
> array (due
> to variations in number of segments), so rte_eth_rxconf structure would
> have the variable length (not nice, IMO).
>
> We could push pointers to the array of rte_eth_rxseg, but we would lost
> the single structure (and contiguous memory) simplicity, this approach has
> no advantages over the specifying the split configuration as parameters
> of setup_ex().
>

I think it has a huge advantage to avoid extra device
operation.

> - it would introduces the ambiguity, rte_eth_rx_queue_setup()
> specifies the single
> mbuf pool as parameter. What should we do with it? Set to NULL? Treat
> as the first pool? I would prefer to specify all split segments in
> uniform fashion, i.e. as array or rte_eth_rxseg structures (and it can be
> easily updated with some extra segment attributes if needed). So, in my
> opinion, we should remove/replace the pool parameter in rx_queue_setup
> (by introducing new func).
>

I'm trying to resolve the ambiguity as described above
(see BUFFER_SPLIT vs SCATTER). Use the pointer for
tail segments with respect to SCATTER capability.

> - specifying the new extended setup roiutine has an advantage that we
> should
> not update any PMDs code in part of existing implementations of
> rte_eth_rx_queue_setup().

It is not required since it is controlled by the new offload
flags. If the offload is not supported, the new field is
invisible for PMD (it simply ignores).

>
> If PMD supports BUFFER_SPLIT (or other related feature) it just should
> provide
> rte_eth_rx_queue_setup_ex() and check the DEV_RX_OFFLOAD_BUFFER_SPLIT
> (or HEADER_SPLIT, or ever feature) it supports. The common code does
> not check the feature flags - it is on PMDs' own. In order to
> configure PMD
> to perfrom arbitrary desired Rx spliting the application should check
> DEV_RX_OFFLOAD_BUFFER_SPLIT in port capabilites, if found - set
> DEV_RX_OFFLOAD_BUFFER_SPLIT in configuration and call
> rte_eth_rx_queue_setup_ex().
> And this approach can be followed for any other split related feature.
>
> With best regards, Slava
>