[dpdk-dev] [PATCH v12 1/6] ethdev: introduce Rx buffer split

[Ferruh Yigit]

On 10/16/2020 8:22 PM, Ferruh Yigit wrote:
> On 10/16/2020 5:44 PM, Viacheslav Ovsiienko wrote:
>> The DPDK datapath in the transmit direction is very flexible.
>> An application can build the multi-segment packet and manages
>> almost all data aspects - the memory pools where segments
>> are allocated from, the segment lengths, the memory attributes
>> like external buffers, registered for DMA, etc.
>>
>> In the receiving direction, the datapath is much less flexible,
>> an application can only specify the memory pool to configure the
>> receiving queue and nothing more. In order to extend receiving
>> datapath capabilities it is proposed to add the way to provide
>> extended information how to split the packets being received.
>>
>> The new offload flag RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT in device
>> capabilities is introduced to present the way for PMD to report to
>> application about supporting Rx packet split to configurable
>> segments. Prior invoking the rte_eth_rx_queue_setup() routine
>> application should check RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT flag.
>>
>> The following structure is introduced to specify the Rx packet
>> segment for RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT offload:
>>
>> struct rte_eth_rxseg_split {
>>
>>      struct rte_mempool *mp; /* memory pools to allocate segment from */
>>      uint16_t length; /* segment maximal data length,
>>                    configures "split point" */
>>      uint16_t offset; /* data offset from beginning
>>                    of mbuf data buffer */
>>      uint32_t reserved; /* reserved field */
>> };
>>
>> The segment descriptions are added to the rte_eth_rxconf structure:
>>     rx_seg - pointer the array of segment descriptions, each element
>>               describes the memory pool, maximal data length, initial
>>               data offset from the beginning of data buffer in mbuf.
>>          This array allows to specify the different settings for
>>          each segment in individual fashion.
>>     rx_nseg - number of elements in the array
>>
>> If the extended segment descriptions is provided with these new
>> fields the mp parameter of the rte_eth_rx_queue_setup must be
>> specified as NULL to avoid ambiguity.
>>
>> There are two options to specify Rx buffer configuration:
>> - mp is not NULL, rrx_conf.rx_nseg is zero, it is compatible
>>    configuration, follows existing implementation, provides
>>    the single pool and no description for segment sizes
>>    and offsets.
>> - mp is NULL, rx_conf.rx_seg is not NULL, rx_conf.rx_nseg is not
>>    zero, it provides the extended configuration, individually for
>>    each segment.
>>
>> f the Rx queue is configured with new settings the packets being
>> received will be split into multiple segments pushed to the mbufs
>> with specified attributes. The PMD will split the received packets
>> into multiple segments according to the specification in the
>> description array.
>>
>> For example, let's suppose we configured the Rx queue with the
>> following segments:
>>      seg0 - pool0, len0=14B, off0=2
>>      seg1 - pool1, len1=20B, off1=128B
>>      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 + 2 in mbuf from pool0
>>      seg1 - 20B long @ 128 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 + 2 in mbuf from pool0
>>      seg1 - 20B @ 128 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 offload RTE_ETH_RX_OFFLOAD_SCATTER must be present and
>> configured to support new buffer split feature (if rx_nseg
>> is greater than one).
>>
>> The split limitations imposed by underlying PMD is reported
>> in the new introduced rte_eth_dev_info->rx_seg_capa field.
>>
>> The new approach would allow splitting the ingress packets into
>> multiple parts pushed to the memory with different attributes.
>> For example, the packet headers can be pushed to the embedded
>> data buffers within mbufs and the application data into
>> the external buffers attached to mbufs allocated from the
>> different memory pools. The memory attributes for the split
>> parts may differ either - for example the application data
>> may be pushed into the external memory located on the dedicated
>> physical device, say GPU or NVMe. This would improve the DPDK
>> receiving datapath flexibility with preserving compatibility
>> with existing API.
>>
>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo at nvidia.com>
>> Acked-by: Ajit Khaparde <ajit.khaparde at broadcom.com>
>> Acked-by: Jerin Jacob <jerinj at marvell.com>
>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>> Acked-by: Thomas Monjalon <thomas at monjalon.net>
> 
> <...>
> 
>> -    mbp_buf_size = rte_pktmbuf_data_room_size(mp);
>> +    if (mp != NULL) {
>> +        /* Single pool configuration check. */
>> +        if (rx_conf->rx_nseg != 0) {
>> +            RTE_ETHDEV_LOG(ERR,
>> +                       "Ambiguous segment configuration\n");
>> +            return -EINVAL;
>> +        }
> 
> 'rte_eth_rx_queue_setup()' accepts 'rx_conf' to be NULL, there is a NULL check 
> for it somewhere below the function :)
> 
> Above will crash in that case, need to fix this, I am on it, any idea welcome.

adding additional checks as following in the next-net:

  diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
  index 372a63bafa..ee2bc51671 100644
  --- a/lib/librte_ethdev/rte_ethdev.c
  +++ b/lib/librte_ethdev/rte_ethdev.c
  @@ -1947,7 +1947,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t 
rx_queue_id,

          if (mp != NULL) {
                  /* Single pool configuration check. */
  -               if (rx_conf->rx_nseg != 0) {
  +               if (rx_conf != NULL && rx_conf->rx_nseg != 0) {
                          RTE_ETHDEV_LOG(ERR,
                                         "Ambiguous segment configuration\n");
                          return -EINVAL;
  @@ -1983,7 +1983,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t 
rx_queue_id,
                  uint16_t n_seg = rx_conf->rx_nseg;

                  /* Extended multi-segment configuration check. */
  -               if (rx_conf->rx_seg == NULL || rx_conf->rx_nseg == 0) {
  +               if (rx_conf == NULL || rx_conf->rx_seg == NULL || 
rx_conf->rx_nseg == 0) {
                          RTE_ETHDEV_LOG(ERR,
                                         "Memory pool is null and no extended 
configuration provided\n");
                          return -EINVAL;