RE: [v4 1/3] ethdev: introduce protocol type based header split

["Ding, Xuan" <xuan.ding@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, April 21, 2022 6:28 PM
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Wu, WenxuanX
> <wenxuanx.wu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Singh,
> Aman Deep <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> dev@dpdk.org
> Cc: dev@dpdk.org; stephen@networkplumber.org;
> mb@smartsharesystems.com; viacheslavo@nvidia.com; Yu, Ping
> <ping.yu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>;
> david.marchand@redhat.com; Ferruh Yigit <ferruhy@xilinx.com>; Ding, Xuan
> <xuan.ding@intel.com>
> Subject: Re: [v4 1/3] ethdev: introduce protocol type based header split
> 
> 12/04/2022 18:15, Ding, Xuan:
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > > On 4/2/22 13:41, wenxuanx.wu@intel.com wrote:
> > > > From: Xuan Ding <xuan.ding@intel.com>
> > > >
> > > > Header split consists of splitting a received packet into two
> > > > separate regions based on the packet content. The split happens
> > > > after the packet header and before the packet payload. Splitting
> > > > is usually between the packet header that can be posted to a
> > > > dedicated buffer and the packet payload that can be posted to a
> different buffer.
> > > >
> > > > Currently, Rx buffer split supports length and offset based packet split.
> > > > Although header split is a subset of buffer split, configuring
> > > > buffer split based on length is not suitable for NICs that do
> > > > split based on header protocol types. Because tunneling makes the
> > > > conversion from length to protocol type impossible.
> > > >
> > > > This patch extends the current buffer split to support protocol
> > > > type and offset based header split. A new proto field is
> > > > introduced in the rte_eth_rxseg_split structure reserved field to
> > > > specify header protocol type. With Rx offload flag
> > > > RTE_ETH_RX_OFFLOAD_HEADER_SPLIT enabled and protocol type
> > > > configured, PMD will split the ingress packets into two separate
> > > > regions. Currently, both inner and outer
> > > > L2/L3/L4 level header split can be supported.
> > >
> > > RTE_ETH_RX_OFFLOAD_HEADER_SPLIT offload was introduced some
> time ago
> > > to substitute bit-field header_split in struct rte_eth_rxmode. It
> > > allows to enable header split offload with the header size
> > > controlled using split_hdr_size in the same structure.
> > >
> > > Right now I see no single PMD which actually supports
> > > RTE_ETH_RX_OFFLOAD_HEADER_SPLIT with above definition.
> > > Many examples and test apps initialize the field to 0 explicitly.
> > > The most of drivers simply ignore split_hdr_size since the offload
> > > is not advertised, but some double-check that its value is 0.
> > >
> > > I think that it means that the field should be removed on the next
> > > LTS, and I'd say, together with the
> RTE_ETH_RX_OFFLOAD_HEADER_SPLIT offload bit.
> > >
> > > We should not redefine the offload meaning.
> >
> > Yes, you are right. No single PMD supports
> RTE_ETH_RX_OFFLOAD_HEADER_SPLIT now.
> > Previously, I used this flag is to distinguish buffer split and header split.
> > The former supports multi-segments split by length and offset.
> > The later supports two segments split by proto and offset.
> > At this level, header split is a subset of buffer split.
> >
> > Since we shouldn't redefine the meaning of this offload, I will use
> > the RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT flag.
> > The existence of tunnel needs to define a proto field in buffer split,
> > because some PMDs do not support split based on length and offset.
> 
> Before doing anything, the first patch of this series should make the current
> status clearer.
> Example, this line does not explain what it does:
> 	uint16_t split_hdr_size;  /**< hdr buf size (header_split enabled).*/
> And header_split has been removed in ab3ce1e0c193 ("ethdev: remove old
> offload API")
> 
> If RTE_ETH_RX_OFFLOAD_HEADER_SPLIT is not needed, let's add a comment
> to start a deprecation.

Agree, I discussed with Andrew before that RTE_ETH_RX_OFFLOAD_HEADER_SPLIT
is no longer supported by any PMDs.

I can send a separate patch of header split deprecation notice in 22.07,
and start removing the code in 22.11. What do you think?

> 
> > > > For example, let's suppose we configured the Rx queue with the
> > > > following segments:
> > > >      seg0 - pool0, off0=2B
> > > >      seg1 - pool1, off1=128B
> > >
> > > Corresponding feature is named Rx buffer split.
> > > Does it mean that protocol type based header split requires Rx
> > > buffer split feature to be supported?
> >
> > Protocol type based header split does not requires Rx buffer split.
> > In previous design, the header split and buffer split are exclusive.
> > Because we only configure one split offload for one RX queue.
> 
> Things must be made clear and documented.

Thanks for your suggestion, the documents will be improved in v5.

> 
> > > > With header split type configured with
> > > > RTE_ETH_RX_HEADER_SPLIT_UDP, the packet consists of
> MAC_IP_UDP_PAYLOAD will be split like following:
> > > >      seg0 - udp header @ RTE_PKTMBUF_HEADROOM + 2 in mbuf from
> > > pool0
> > > >      seg1 - payload @ 128 in mbuf from pool1
> > >
> > > Is it always outermost UDP? Does it require both UDP over IPv4 and
> > > UDP over
> > > IPv6 to be supported? What will happen if only one is supported? How
> > > application can find out which protocol stack are supported?
> >
> > Both inner and outer UDP are considered.
> > Current design does not distinguish UDP over IPv4 or IPv6.
> > If we want to support granularity like only IPv4 or IPv6 supported,
> > user need add more configurations.
> >
> > If application want to find out which protocol stack is supported, one
> > way I think is to expose the protocol stack supported by the driver through
> dev_info.
> > Any thoughts are welcomed :)
> [...]
> > > > +	uint16_t reserved; /**< Reserved field. */
> > >
> > > As far as I can see the structure is experimental. So, it should not
> > > be the problem to extend it, but it is a really good question raised
> > > by Stephen in RFC
> > > v1 discussion.
> > > Shouldn't we require that all reserved fields are initialized to
> > > zero and ignored on processing? Frankly speaking I always thought
> > > so, but failed to find the place were it is documented.
> >
> > Yes, it can be documented. By default is should be zero, and we can
> > configure it to enable protocol type based buffer split.
> >
> > > @Thomas, @David, @Ferruh?
> 
> Yes that's very important to have a clear state of the reserved fields.
> A value must be set and documented.

Ditto, thanks for your comments. :)

Regards,
Xuan

> 
>