[PATCH v7 2/4] ethdev: introduce protocol hdr based buffer split

[Wang, YuanX]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Tuesday, October 4, 2022 4:23 PM
> To: Wang, YuanX <yuanx.wang at intel.com>; dev at dpdk.org; Thomas
> Monjalon <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at amd.com>
> Cc: ferruh.yigit at xilinx.com; mdr at ashroe.eu; Li, Xiaoyun
> <xiaoyun.li at intel.com>; Singh, Aman Deep <aman.deep.singh at intel.com>;
> Zhang, Yuying <yuying.zhang at intel.com>; Zhang, Qi Z
> <qi.z.zhang at intel.com>; Yang, Qiming <qiming.yang at intel.com>;
> jerinjacobk at gmail.com; viacheslavo at nvidia.com;
> stephen at networkplumber.org; Ding, Xuan <xuan.ding at intel.com>;
> hpothula at marvell.com; Tang, Yaqi <yaqi.tang at intel.com>
> Subject: Re: [PATCH v7 2/4] ethdev: introduce protocol hdr based buffer split
> 
> On 10/4/22 05:48, Wang, YuanX wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> On 10/2/22 00:05, Yuan Wang wrote:
> >>> +
> >>> +			/* skip the payload */
> >>
> >> Sorry, it is confusing. What do you mean here?
> >
> > Because setting n proto_hdr will generate (n+1) segments. If we want to
> split the packet into n segments, we only need to check the first (n-1)
> proto_hdr.
> > For example, for ETH-IPV4-UDP-PAYLOAD, if we want to split after the UDP
> header, we only need to set and check the UDP header in the first segment.
> >
> > Maybe mask is not a good way, so we will use index to filter out the check
> of proto_hdr inside the last segment.
> 
> I see your point and understand the problem now.
> Thinking a bit more about it I realize that consistency check here should be
> more sophisticated.
> It should not allow:
>   - seg1 - length-based, seg2 - proto-based, seg3 - payload
>   - seg1 - proto-based, seg2 - legnth-based, seg3 - proto-based, seg4 - payload
> I.e. no protocol-based split after length-based.
> But should allow:
>   - seg1 - proto-based, seg2 - legnth-based, seg3 - payload I.e. length based
> split after protocol-based.
> 
> Taking the last point above into account, proto_hdr in the last segment
> should be 0 like in length-based split (not RTE_PTYPE_ALL_MASK).

Just to confirm, do you mean that the payload as last segment should be treated as a length-based split(proto_hdr == 0)?
If so, for this question, 'check that dataroom in the last segment mempool is sufficient> for up to MTU packet if Rx scatter is disabled'
Is it not necessary to compare MTU size and mbuf_size? Because the check in length based split is sufficient. We will send v8 soon with above thought, please help to check.

> 
> It is an interesting question how to request:
>   - seg1 - ETH, seg2 - IPv4, seg3 - UDP, seg4 - payload Should we really repeat
> ETH in seg2->proto_hdr and
> seg3->proto_hdr header and IPv4 in seg3->proto_hdr again?
> I tend to say no since when packet comes to seg2 it already has no ETH
> header.
> 
> If so, how to handle configuration when ETH is repeat in seg2?
> For example,
>    - seg1 ETH+IPv4+UDP
>    - seg2 ETH+IPv6+UDP
>    - seg2 0
> Should we deny it or should we define behaviour like.
> If a packet does not match segX proto_hdr, the segment is skipped and
> segX+1 considered.
> Of course, not all drivers/HW supports it. If so, such configuration should be
> just discarded by the driver itself.

Here a question that needs to be clarified, whether the segments are sequential or independent. I prefer the former because it's more readable. Furthermore, it consists with length based split, which also configures the lengths sequentially. In this case, the following situation does not exist:
- seg1 ETH+IPv4+UDP
- seg2 ETH+IPv6+UDP
- seg3 0

For the case of repeating ETH, such as - seg1 - ETH, seg2 - IPv4, seg3 - UDP, seg4 - payload, as you suggested, we can omit ETH in the following segment. but IPV4-UDP and IPV6-UDP still need  to be distinguished, follow our previous discussion (user wants to split at IPV4-UDP rather than IPV6-UDP although driver supports both). In this case, seg1 - ETH, seg2 - IPv4, seg3 - UDP, seg4 - payload,
we set proto_hdr with:
seg1 proto_hdr1=RTE_PTYPE_L2_ETHER
seg2 proto_hdr2=RTE_PTYPE_L3_IPV4
seg3 proto_hdr3=RTE_PTYPE_L3_IPV4 | RTE_PTYPE_L4_UDP

Thanks,
Yuan