RE: [PATCH v3] app/testpmd: fix protocol header display for Rx buffer split

["Wang, YuanX" <yuanx.wang@intel.com>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, November 7, 2022 7:31 PM
> To: Wang, YuanX <yuanx.wang@intel.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Cc: Ding, Xuan <xuan.ding@intel.com>; Tang, Yaqi <yaqi.tang@intel.com>;
> dev@dpdk.org
> Subject: Re: [PATCH v3] app/testpmd: fix protocol header display for Rx
> buffer split
> 
> On 11/7/22 11:45, Yuan Wang wrote:
> > The "show config rxhdrs" cmd displays the configured protocol headers
> > that are used for protocol-based buffer split.
> > However, it shows inner-ipv6 as inner-ipv4.
> >
> > This patch fixes that by adjusting the order of condition judgments.
> > This patch also uses RTE_PTYPE_*_MASK as masks.
> >
> > Fixes: 52e2e7edcf48 ("app/testpmd: add protocol-based buffer split")
> >
> > Signed-off-by: Yuan Wang <yuanx.wang@intel.com>
> >
> > ---
> > v3:
> > - use RTE_PTYPE_*_MASK as masks.
> > - refactor to use switch statement.
> > v2:
> > - add fixline.
> >
> > ---
> >   app/test-pmd/config.c | 89 +++++++++++++++++++++----------------------
> >   1 file changed, 44 insertions(+), 45 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > e8a1b77c2a..8638dfed11 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -5070,73 +5070,72 @@ show_rx_pkt_segments(void)
> >
> >   static const char *get_ptype_str(uint32_t ptype)
> >   {
> > -	if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_TCP)) ==
> > -		(RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_TCP))
> > +	switch (ptype & (RTE_PTYPE_L3_MASK | RTE_PTYPE_L4_MASK)) {
> > +	case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP:
> >   		return "ipv4-tcp";
> 
> If I map "ipv4-tcp" to packets types, I get:
> RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_TCP but vice versa it is sufficient to have just
> RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_TCP I think such
> asymmetry in mapping is bad.

The reason for the asymmetric is that the lib requires that each segment cannot be repeated with the previous one. For example "set rxhdrs eth,ipv4-udp", seg[0]=RTE_PTYPE_L2_ETHER,seg[1]=RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP. 
In order not to duplicate, seg[1] is truncated to RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP. 
In this way we can use the truncated seg[1] to map to "ipv4-udp". The same goes for tunneled packets.

> 
> > -	else if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_UDP)) ==
> > -		(RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_UDP))
> > +	case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_UDP:
> >   		return "ipv4-udp";
> > -	else if ((ptype & (RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_SCTP)) ==
> > -		(RTE_PTYPE_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_L4_SCTP))
> > +	case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP:
> >   		return "ipv4-sctp";
> > -	else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_TCP)) ==
> > -		(RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_TCP))
> > +	case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_TCP:
> >   		return "ipv6-tcp";
> > -	else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_UDP)) ==
> > -		(RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_UDP))
> > +	case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_UDP:
> >   		return "ipv6-udp";
> > -	else if ((ptype & (RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_SCTP)) ==
> > -		(RTE_PTYPE_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_L4_SCTP))
> > +	case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_L4_SCTP:
> >   		return "ipv6-sctp";
> > -	else if ((ptype & RTE_PTYPE_L4_TCP) == RTE_PTYPE_L4_TCP)
> > +	case RTE_PTYPE_L4_TCP:
> >   		return "tcp";
> > -	else if ((ptype & RTE_PTYPE_L4_UDP) == RTE_PTYPE_L4_UDP)
> > +	case RTE_PTYPE_L4_UDP:
> >   		return "udp";
> > -	else if ((ptype & RTE_PTYPE_L4_SCTP) == RTE_PTYPE_L4_SCTP)
> > +	case RTE_PTYPE_L4_SCTP:
> >   		return "sctp";
> > -	else if ((ptype & RTE_PTYPE_L3_IPV4_EXT_UNKNOWN) ==
> RTE_PTYPE_L3_IPV4_EXT_UNKNOWN)
> > +	case RTE_PTYPE_L3_IPV4_EXT_UNKNOWN:
> >   		return "ipv4";
> > -	else if ((ptype & RTE_PTYPE_L3_IPV6_EXT_UNKNOWN) ==
> RTE_PTYPE_L3_IPV6_EXT_UNKNOWN)
> > +	case RTE_PTYPE_L3_IPV6_EXT_UNKNOWN:
> >   		return "ipv6";
> > -	else if ((ptype & RTE_PTYPE_L2_ETHER) == RTE_PTYPE_L2_ETHER)
> > +	}
> > +
> > +	switch (ptype & RTE_PTYPE_L2_MASK) {
> 
> Having many switches here looks confusing. Who defines priorities? IMHO it
> should be single switch here and values should be in exactly the same order
> as get_ptype().
> Ideally both function should be close to each other.

The order of switch statement (and if statement as well) is to correctly map ptype to string. For example set rxhdrs ipv4-udp corresponds to RTE_PTYPE_L2_ETHER | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN | RTE_PTYPE_L3_IPV4_EXT_UNKNOWN UNKNOWN | RTE_PTYPE_L4_UDP; we need to prioritize RTE_PTYPE_L3_IPV4_EXT_UNKNOWN|RTE_PTYPE_L4_UDP over RTE_PTYPE_L2_ETHER.

I have an idea to solve the multiple switches and asymmetry issue by adding a variable to hold the original ptypes. Please see v4.

> 
> > +	case RTE_PTYPE_L2_ETHER:
> >   		return "eth";
> > +	}
> >
> > -	else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_TCP)) ==
> > -		(RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_TCP))
> > -		return "inner-ipv4-tcp";
> > -	else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_UDP)) ==
> > -		(RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_UDP))
> > -		return "inner-ipv4-udp";
> > -	else if ((ptype & (RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_SCTP)) ==
> > -		(RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_SCTP))
> > -		return "inner-ipv4-sctp";
> > -	else if ((ptype & (RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_TCP)) ==
> > -		(RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_TCP))
> > +	switch (ptype & (RTE_PTYPE_INNER_L3_MASK |
> RTE_PTYPE_INNER_L4_MASK)) {
> > +	case RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN |
> RTE_PTYPE_INNER_L4_TCP:
> >   		return "inner-ipv6-tcp";
> 
> get_ptype():
> inner-ipv6-tcp -> RTE_PTYPE_TUNNEL_GRENAT |
> RTE_PTYPE_INNER_L2_ETHER |
> RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN | RTE_PTYPE_INNER_L4_TCP
> 
> So, mapping is asymmetric again.
> 
> Out of topic for the patch:
> Also I'm wondering why inner-ipv6-tcp is a grenat. Why not VxLAN, not
> GENEVE?

The reason inner-ipv6-tcp is grenat is to simplify the expression.
If add VxLAN/GENEVE, then the setting command needs to be changed to grenat-ipv6-tcp,VxLAN-ipv6-tcp,GENEVE-ipv6-tcp,
and so on grenat-ipv6,VxLANX-ipv6... This increases the complexity of the command.
So, for simplicity we choose grenat as the default for tunneled packets.
Any thoughts?

Thanks,
Yuan