Re: [dpdk-dev] [PATCH 01/32] net/ngbe: add packet type

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On September 16, 2021 12:48 AM, Ferruh Yigit wrote:
> On 9/8/2021 9:37 AM, Jiawen Wu wrote:
> > Add packet type marco definition and convert ptype to ptid.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >  doc/guides/nics/features/ngbe.ini |   1 +
> >  doc/guides/nics/ngbe.rst          |   1 +
> >  drivers/net/ngbe/meson.build      |   1 +
> >  drivers/net/ngbe/ngbe_ethdev.c    |   9 +
> >  drivers/net/ngbe/ngbe_ethdev.h    |   4 +
> >  drivers/net/ngbe/ngbe_ptypes.c    | 300
> ++++++++++++++++++++++++++++++
> >  drivers/net/ngbe/ngbe_ptypes.h    | 240 ++++++++++++++++++++++++
> >  drivers/net/ngbe/ngbe_rxtx.c      |  16 ++
> >  drivers/net/ngbe/ngbe_rxtx.h      |   2 +
> >  9 files changed, 574 insertions(+)
> >  create mode 100644 drivers/net/ngbe/ngbe_ptypes.c  create mode
> 100644
> > drivers/net/ngbe/ngbe_ptypes.h
> >
> > diff --git a/doc/guides/nics/features/ngbe.ini
> > b/doc/guides/nics/features/ngbe.ini
> > index 08d5f1b0dc..8b7588184a 100644
> > --- a/doc/guides/nics/features/ngbe.ini
> > +++ b/doc/guides/nics/features/ngbe.ini
> > @@ -8,6 +8,7 @@ Speed capabilities   = Y
> >  Link status          = Y
> >  Link status event    = Y
> >  Queue start/stop     = Y
> > +Packet type parsing  = Y
> 
> "Packet type parsing" also requires to support
> 'rte_eth_dev_get_supported_ptypes()' & 'rte_eth_dev_set_ptypes()' APIs.
> 
> Current implementation seems parses the packet type and updates mbuf field
> for it but doesn't support above APIs, can you please add them too? There is
> already 'ngbe_dev_supported_ptypes_get()' function but dev_ops seems not
> set.
> 

Oops.., I forgot it.

> <...>
> 
> > +++ b/drivers/net/ngbe/ngbe_ptypes.c
> > @@ -0,0 +1,300 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018-2021 Beijing WangXun Technology Co., Ltd.
> > + */
> > +
> > +#include <rte_mbuf.h>
> > +#include <rte_memory.h>
> > +
> > +#include "base/ngbe_type.h"
> > +#include "ngbe_ptypes.h"
> > +
> > +/* The ngbe_ptype_lookup is used to convert from the 8-bit ptid in
> > +the
> > + * hardware to a bit-field that can be used by SW to more easily
> > +determine the
> > + * packet type.
> > + *
> > + * Macros are used to shorten the table lines and make this table
> > +human
> > + * readable.
> > + *
> > + * We store the PTYPE in the top byte of the bit field - this is just
> > +so that
> > + * we can check that the table doesn't have a row missing, as the
> > +index into
> > + * the table should be the PTYPE.
> > + */
> > +#define TPTE(ptid, l2, l3, l4, tun, el2, el3, el4) \
> > +	[ptid] = (RTE_PTYPE_L2_##l2 | \
> > +		RTE_PTYPE_L3_##l3 | \
> > +		RTE_PTYPE_L4_##l4 | \
> > +		RTE_PTYPE_TUNNEL_##tun | \
> > +		RTE_PTYPE_INNER_L2_##el2 | \
> > +		RTE_PTYPE_INNER_L3_##el3 | \
> > +		RTE_PTYPE_INNER_L4_##el4)
> > +
> > +#define RTE_PTYPE_L2_NONE               0
> > +#define RTE_PTYPE_L3_NONE               0
> > +#define RTE_PTYPE_L4_NONE               0
> > +#define RTE_PTYPE_TUNNEL_NONE           0
> > +#define RTE_PTYPE_INNER_L2_NONE         0
> > +#define RTE_PTYPE_INNER_L3_NONE         0
> > +#define RTE_PTYPE_INNER_L4_NONE         0
> 
> Why you are defining new PTYPEs? If these are for driver internal you can drop
> the 'RTE_' prefix.
> 

I just want to use short macros, to make the lookup table readable.
So it needs 'RTE_' prefix here, to be compatible with other RTE mbuf packet types.

> <...>
> 
> > +
> > +#ifndef RTE_PTYPE_UNKNOWN
> > +#define RTE_PTYPE_UNKNOWN                   0x00000000
> > +#define RTE_PTYPE_L2_ETHER                  0x00000001
> > +#define RTE_PTYPE_L2_ETHER_TIMESYNC         0x00000002
> > +#define RTE_PTYPE_L2_ETHER_ARP              0x00000003
> > +#define RTE_PTYPE_L2_ETHER_LLDP             0x00000004
> > +#define RTE_PTYPE_L2_ETHER_NSH              0x00000005
> > +#define RTE_PTYPE_L2_ETHER_FCOE             0x00000009
> > +#define RTE_PTYPE_L3_IPV4                   0x00000010
> > +#define RTE_PTYPE_L3_IPV4_EXT               0x00000030
> > +#define RTE_PTYPE_L3_IPV6                   0x00000040
> > +#define RTE_PTYPE_L3_IPV4_EXT_UNKNOWN       0x00000090
> > +#define RTE_PTYPE_L3_IPV6_EXT               0x000000c0
> > +#define RTE_PTYPE_L3_IPV6_EXT_UNKNOWN       0x000000e0
> > +#define RTE_PTYPE_L4_TCP                    0x00000100
> > +#define RTE_PTYPE_L4_UDP                    0x00000200
> > +#define RTE_PTYPE_L4_FRAG                   0x00000300
> > +#define RTE_PTYPE_L4_SCTP                   0x00000400
> > +#define RTE_PTYPE_L4_ICMP                   0x00000500
> > +#define RTE_PTYPE_L4_NONFRAG                0x00000600
> > +#define RTE_PTYPE_TUNNEL_IP                 0x00001000
> > +#define RTE_PTYPE_TUNNEL_GRE                0x00002000
> > +#define RTE_PTYPE_TUNNEL_VXLAN              0x00003000
> > +#define RTE_PTYPE_TUNNEL_NVGRE              0x00004000
> > +#define RTE_PTYPE_TUNNEL_GENEVE             0x00005000
> > +#define RTE_PTYPE_TUNNEL_GRENAT             0x00006000
> > +#define RTE_PTYPE_INNER_L2_ETHER            0x00010000
> > +#define RTE_PTYPE_INNER_L2_ETHER_VLAN       0x00020000
> > +#define RTE_PTYPE_INNER_L3_IPV4             0x00100000
> > +#define RTE_PTYPE_INNER_L3_IPV4_EXT         0x00200000
> > +#define RTE_PTYPE_INNER_L3_IPV6             0x00300000
> > +#define RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN 0x00400000
> > +#define RTE_PTYPE_INNER_L3_IPV6_EXT         0x00500000
> > +#define RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN 0x00600000
> > +#define RTE_PTYPE_INNER_L4_TCP              0x01000000
> > +#define RTE_PTYPE_INNER_L4_UDP              0x02000000
> > +#define RTE_PTYPE_INNER_L4_FRAG             0x03000000
> > +#define RTE_PTYPE_INNER_L4_SCTP             0x04000000
> > +#define RTE_PTYPE_INNER_L4_ICMP             0x05000000
> > +#define RTE_PTYPE_INNER_L4_NONFRAG          0x06000000
> > +#endif /* !RTE_PTYPE_UNKNOWN */
> 
> These are already defined in the mbuf public header, why there are defined
> again?
> 

These can be removed directly. They were written previously for version compatibility.

> <...>
> 
> > @@ -378,6 +389,10 @@ ngbe_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
> >  		rxm->data_len = pkt_len;
> >  		rxm->port = rxq->port_id;
> >
> > +		pkt_info = rte_le_to_cpu_32(rxd.qw0.dw0);
> > +		rxm->packet_type = ngbe_rxd_pkt_info_to_pkt_type(pkt_info,
> > +						       rxq->pkt_type_mask);
> > +
> >  		/*
> >  		 * Store the mbuf address into the next entry of the array
> >  		 * of returned packets.
> > @@ -799,6 +814,7 @@ ngbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
> >  	rxq->port_id = dev->data->port_id;
> >  	rxq->drop_en = rx_conf->rx_drop_en;
> >  	rxq->rx_deferred_start = rx_conf->rx_deferred_start;
> > +	rxq->pkt_type_mask = NGBE_PTID_MASK;
> 
> What is the use of the 'pkt_type_mask', it seems it is a fixed value, why keeping
> it per queue?