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

[Thomas Monjalon <thomas@monjalon.net>]

20/04/2022 16:39, Andrew Rybchenko:
> On 4/12/22 19:40, Ding, Xuan wrote:
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> >> On Sat, Apr 2, 2022 at 4:33 PM <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.
> >>>
> >>> For example, let's suppose we configured the Rx queue with the
> >>> following segments:
> >>>      seg0 - pool0, off0=2B
> >>>      seg1 - pool1, off1=128B
> >>>
> >>> 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
> >>
> >> If we set rte_eth_rxseg_split::proto = RTE_ETH_RX_HEADER_SPLIT_UDP and
> >> rte_eth_rxseg_split.offset = 2, What will be the content for seg0, Will it be,
> >> - offset as Starts atUDP Header
> >> - size of segment as MAX(size of UDP header + 2, 128(as seg 1 start from128).
> >> Right? If not, Please describe
> > 
> > Proto defines the location in packet for split.
> > Offset defines data buffer from beginning of mbuf data buffer, it can be zero.
> > With proto and offset configured, packets received will be split into two segments.
> > 
> > So in this configuration, the seg0 content is UDP header, the seg1 content is the payload.
> > Size of seg0 is size of UDP header, size of seg1 is size of payload.
> > rte_eth_rxseg_split.offset = 2/128 decides the mbuf offset, rather than segment size.
> 
> Above discussion proves that definition of the struct
> rte_eth_rxseg_split is misleading. It is hard to catch
> from naming that length defines a maximum data amount
> to be copied, but office is a an offset in destination
> mbuf. The structure is still experimental and I think
> we should improve naming: offset -> mbuf_offset?

I agree it is confusing.
mbuf_offset could be a better name.
length could be renamed as well. Is data_length better?

But the most important is to have a clear description
in the doxygen comment of the field.
We must specify what is the starting point and the "end" for those fields.

> >> Also, I don't think we need duplate
> >> rte_eth_rx_header_split_protocol_type instead we can reuse existing
> >> RTE_PTYPE_*  flags.
> > 
> > That's a good idea. Yes, I can use the RTE_PTYPE_* here. My only
> > concern is the 32-bits RTE_PTYPE_* will run out of the 32-bits reserved fields.
> > If this proposal is agreed, I will use RTE_PTYPE_* instead of rte_eth_rx_header_split_protocol_type.

Yes I think RTE_PTYPE_* is appropriate.