Re: [dpdk-dev] [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API

[Bing Zhao <bingz@mellanox.com>]

Hi Olivier,
Many thanks for your comments.

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Friday, July 10, 2020 10:31 PM
> To: Bing Zhao <bingz@mellanox.com>
> Cc: Ori Kam <orika@mellanox.com>; john.mcnamara@intel.com;
> marko.kovacevic@intel.com; Thomas Monjalon
> <thomas@monjalon.net>; ferruh.yigit@intel.com;
> arybchenko@solarflare.com; akhil.goyal@nxp.com; dev@dpdk.org;
> wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com
> Subject: Re: [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API
> 
> Hi Bing,
> 
> On Fri, Jul 10, 2020 at 04:45:22PM +0800, Bing Zhao wrote:
> > Add a new item "rte_flow_item_ecpri" in order to match eCRPI
> header.
> >
> > eCPRI is a packet based protocol used in the fronthaul interface of 5G
> > networks. Header format definition could be found in the
> specification
> > via the link below:
> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> www.
> > gigalight.com%2Fdownloads%2Fstandards%2Fecpri-
> specification.pdf&amp;da
> >
> ta=02%7C01%7Cbingz%40mellanox.com%7C99fe8c970b6648d82f7908
> d824dded33%7
> >
> Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C6372998828794
> 68155&amp;sda
> >
> ta=hv1rAa9I2%2BPpZkoLKzHdEpjrLyvY%2F4IiaEvW3XHnhW4%3D&amp
> ;reserved=0
> >
> > eCPRI message can be over Ethernet layer (.1Q supported also) or
> over
> > UDP layer. Message header formats are the same in these two
> variants.
> >
> > Signed-off-by: Bing Zhao <bingz@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
> > ---
> >  doc/guides/prog_guide/rte_flow.rst     |   8 ++
> >  doc/guides/rel_notes/release_20_08.rst |   5 +
> >  lib/librte_ethdev/rte_flow.c           |   1 +
> >  lib/librte_ethdev/rte_flow.h           |  31 ++++++
> >  lib/librte_net/Makefile                |   1 +
> >  lib/librte_net/meson.build             |   3 +-
> >  lib/librte_net/rte_ecpri.h             | 182
> +++++++++++++++++++++++++++++++++
> >  lib/librte_net/rte_ether.h             |   1 +
> >  8 files changed, 231 insertions(+), 1 deletion(-)  create mode 100644
> > lib/librte_net/rte_ecpri.h
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index d5dd18c..669d519 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1362,6 +1362,14 @@ Matches a PFCP Header.
> >  - ``seid``: session endpoint identifier.
> >  - Default ``mask`` matches s_field and seid.
> >
> > +Item: ``ECPRI``
> > +^^^^^^^^^^^^^
> > +
> > +Matches a eCPRI header.
> > +
> > +- ``hdr``: eCPRI header definition (``rte_ecpri.h``).
> > +- Default ``mask`` matches message type of common header only.
> > +
> >  Actions
> >  ~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > index 988474c..19feb68 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -184,6 +184,11 @@ New Features
> >    which are used to access packet data in a safe manner. Currently
> JIT support
> >    for these instructions is implemented for x86 only.
> >
> > +* **Added eCPRI protocol support in rte_flow.**
> > +
> > +  The ``ECPRI`` item have been added to support eCPRI packet
> > + offloading for  5G network.
> > +
> >  * **Added flow performance test application.**
> >
> >    Added new application to test ``rte_flow`` performance, including:
> > diff --git a/lib/librte_ethdev/rte_flow.c
> > b/lib/librte_ethdev/rte_flow.c index 1685be5..f8fdd68 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -95,6 +95,7 @@ struct rte_flow_desc_data {
> >  	MK_FLOW_ITEM(HIGIG2, sizeof(struct
> rte_flow_item_higig2_hdr)),
> >  	MK_FLOW_ITEM(L2TPV3OIP, sizeof(struct
> rte_flow_item_l2tpv3oip)),
> >  	MK_FLOW_ITEM(PFCP, sizeof(struct rte_flow_item_pfcp)),
> > +	MK_FLOW_ITEM(ECPRI, sizeof(struct rte_flow_item_ecpri)),
> >  };
> >
> >  /** Generate flow_action[] entry. */
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index b0e4199..8a90226 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -28,6 +28,7 @@
> >  #include <rte_byteorder.h>
> >  #include <rte_esp.h>
> >  #include <rte_higig.h>
> > +#include <rte_ecpri.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_mbuf_dyn.h>
> >
> > @@ -527,6 +528,15 @@ enum rte_flow_item_type {
> >  	 */
> >  	RTE_FLOW_ITEM_TYPE_PFCP,
> >
> > +	/**
> > +	 * Matches eCPRI Header.
> > +	 *
> > +	 * Configure flow for eCPRI over ETH or UDP packets.
> > +	 *
> > +	 * See struct rte_flow_item_ecpri.
> > +	 */
> > +	RTE_FLOW_ITEM_TYPE_ECPRI,
> > +
> >  };
> >
> >  /**
> > @@ -1547,6 +1557,27 @@ struct rte_flow_item_pfcp {  #endif
> >
> >  /**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior
> notice
> > + *
> > + * RTE_FLOW_ITEM_TYPE_ECPRI
> > + *
> > + * Match eCPRI Header
> > + */
> > +struct rte_flow_item_ecpri {
> > +	struct rte_ecpri_msg_hdr hdr;
> > +};
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_ECPRI. */ #ifndef
> __cplusplus
> > +static const struct rte_flow_item_ecpri rte_flow_item_ecpri_mask =
> {
> > +	.hdr = {
> > +		.dw0 = 0x0,
> > +	},
> > +};
> > +#endif
> > +
> > +/**
> >   * Matching pattern item definition.
> >   *
> >   * A pattern is formed by stacking items starting from the lowest
> > protocol diff --git a/lib/librte_net/Makefile
> > b/lib/librte_net/Makefile index aa1d6fe..9830e77 100644
> > --- a/lib/librte_net/Makefile
> > +++ b/lib/librte_net/Makefile
> > @@ -20,5 +20,6 @@ SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include
> +=
> > rte_sctp.h rte_icmp.h rte_arp.h
> > SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ether.h
> rte_gre.h
> > rte_net.h  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include +=
> rte_net_crc.h
> > rte_mpls.h rte_higig.h  SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include
> +=
> > rte_gtp.h rte_vxlan.h
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include += rte_ecpri.h
> >
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_net/meson.build b/lib/librte_net/meson.build
> > index f799349..24ed825 100644
> > --- a/lib/librte_net/meson.build
> > +++ b/lib/librte_net/meson.build
> > @@ -15,7 +15,8 @@ headers = files('rte_ip.h',
> >  	'rte_net.h',
> >  	'rte_net_crc.h',
> >  	'rte_mpls.h',
> > -	'rte_higig.h')
> > +	'rte_higig.h',
> > +	'rte_ecpri.h')
> >
> >  sources = files('rte_arp.c', 'rte_ether.c', 'rte_net.c',
> > 'rte_net_crc.c')  deps += ['mbuf'] diff --git
> > a/lib/librte_net/rte_ecpri.h b/lib/librte_net/rte_ecpri.h new file
> > mode 100644 index 0000000..60fb4f7
> > --- /dev/null
> > +++ b/lib/librte_net/rte_ecpri.h
> > @@ -0,0 +1,182 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2020 Mellanox Technologies, Ltd  */
> > +
> > +#ifndef _RTE_ECPRI_H_
> > +#define _RTE_ECPRI_H_
> > +
> > +#include <stdint.h>
> > +#include <rte_byteorder.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/**
> > + * eCPRI Protocol Revision 1.0, 1.1, 1.2, 2.0: 0001b
> > + * Other values are reserved for future  */
> > +#define RTE_ECPRI_REV_UP_TO_20		1
> > +
> > +/**
> > + * eCPRI message types in specifications
> > + * IWF* types will only be supported from rev.2
> > + * 12-63: Reserved for future revision
> > + * 64-255: Vendor Specific
> > + */
> > +#define RTE_ECPRI_MSG_TYPE_IQ_DATA	0
> 
> Here the doxygen comment applies to
> RTE_ECPRI_MSG_TYPE_IQ_DATA.
> 
> I think it should either be a standard comment (no doxygen), or
> something more complex should be done, like grouping:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> stackoverflow.com%2Fquestions%2F30803156%2Fgroup-level-
> documentation-for-preprocessor-defines-in-
> doxygen&amp;data=02%7C01%7Cbingz%40mellanox.com%7C99fe8c9
> 70b6648d82f7908d824dded33%7Ca652971c7d2e4d9ba6a4d149256f46
> 1b%7C0%7C1%7C637299882879478151&amp;sdata=0BGV9rUpTYPpKA
> Thq9BgixZCtnlMVmILVty0f37xJ7I%3D&amp;reserved=0
> (I didn't try)
> 

Thanks, I will have a try to fix this comments.

> 
> > +#define RTE_ECPRI_MSG_TYPE_BIT_SEQ	1
> > +#define RTE_ECPRI_MSG_TYPE_RTC_CTRL	2
> > +#define RTE_ECPRI_MSG_TYPE_GEN_DATA	3
> > +#define RTE_ECPRI_MSG_TYPE_RM_ACC	4
> > +#define RTE_ECPRI_MSG_TYPE_DLY_MSR	5
> > +#define RTE_ECPRI_MSG_TYPE_RMT_RST	6
> > +#define RTE_ECPRI_MSG_TYPE_EVT_IND	7
> > +#define RTE_ECPRI_MSG_TYPE_IWF_UP	8
> > +#define RTE_ECPRI_MSG_TYPE_IWF_OPT	9
> > +#define RTE_ECPRI_MSG_TYPE_IWF_MAP	10
> > +#define RTE_ECPRI_MSG_TYPE_IWF_DCTRL	11
> > +
> > +/**
> > + * Event Type of Message Type #7: Event Indication
> > + * 0x00: Fault(s) Indication
> > + * 0x01: Fault(s) Indication Acknowledge
> > + * 0x02: Notification(s) Indication
> > + * 0x03: Synchronization Request
> > + * 0x04: Synchronization Acknowledge
> > + * 0x05: Synchronization End Indication
> > + * 0x06...0xFF: Reserved
> > + */
> > +#define RTE_ECPRI_EVT_IND_FAULT_IND	0
> > +#define RTE_ECPRI_EVT_IND_FAULT_ACK	1
> > +#define RTE_ECPRI_EVT_IND_NTFY_IND	2
> > +#define RTE_ECPRI_EVT_IND_SYNC_REQ	3
> > +#define RTE_ECPRI_EVT_IND_SYNC_ACK	4
> > +#define RTE_ECPRI_EVT_IND_SYNC_END	5
> > +
> > +/**
> > + * eCPRI Common Header
> > + */
> > +RTE_STD_C11
> > +struct rte_ecpri_common_hdr {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +	uint32_t size:16;		/**< Payload Size */
> > +	uint32_t type:8;		/**< Message Type */
> > +	uint32_t c:1;			/**< Concatenation Indicator
> */
> > +	uint32_t res:3;			/**< Reserved */
> > +	uint32_t revision:4;		/**< Protocol Revision */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +	uint32_t revision:4;		/**< Protocol Revision */
> > +	uint32_t res:3;			/**< Reserved */
> > +	uint32_t c:1;			/**< Concatenation Indicator
> */
> > +	uint32_t type:8;		/**< Message Type */
> > +	uint32_t size:16;		/**< Payload Size */
> > +#endif
> > +} __rte_packed;
> 
> Does it really need to be packed? Why next types do not need it?
> It looks only those which have bitfields are.
> 

Nice catch, thanks. For the common header, there is no need to use
the packed attribute, because it is a u32 and the bitfields will be
aligned.
I checked all the definitions again. Only " Type #4: Remote Memory Access"
needs to use the packed attribute.
For other sub-types, "sub-header" part of the message payload will get
aligned by nature. For example, u16 after u16, u8 after u16, these should
be OK.
But in type #4, the address is 48bits wide, with 16bits MSB and 32bits LSB (no
detailed description in the specification, correct me if anything wrong.) Usually,
the 48bits address will be devided as this in a system. And there is no 48-bits
type at all. So we need to define two parts for it: 32b LSB follows 16b MSB.
u32 after u16 should be with packed attribute. Thanks

> 
> I wonder if the 'dw0' could be in this definition instead of in struct
> rte_ecpri_msg_hdr?
> 
> Something like this:
> 
> struct rte_ecpri_common_hdr {
> 	union {
> 		uint32_t u32;
> 		struct {
> 			...
> 		};
> 	};
> };
> 
> I see 2 advantages:
> 
> - someone that only uses the common_hdr struct can use the .u32
>   in its software
> - when using it in messages, it looks clearer to me:
>     msg.common_hdr.u32 = value;
>   instead of:
>     msg.dw0 = value;
> 
> What do you think?

Thanks for the suggestion, this is much better, I will change it.
Indeed, in my original version, no DW(u32) is defined for the header.
After that, I noticed that it would not be easy for the static casting to a u32
from bitfield(the compiler will complain), and it would not be clear to
swap the endian if the user wants to use this header. I added this DW(u32)
to simplify the usage of this header. But yes, if I do not add it here, it would
be not easy or clear for users who just use this header structure.
I will change it. Is it OK if I use the name "dw0"?

> 
> > +
> > +/**
> > + * eCPRI Message Header of Type #0: IQ Data  */ struct
> > +rte_ecpri_msg_iq_data {
> > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #1: Bit Sequence  */ struct
> > +rte_ecpri_msg_bit_seq {
> > +	rte_be16_t pc_id;		/**< Physical channel ID */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #2: Real-Time Control Data  */
> struct
> > +rte_ecpri_msg_rtc_ctrl {
> > +	rte_be16_t rtc_id;		/**< Real-Time Control Data ID
> */
> > +	rte_be16_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #3: Generic Data Transfer  */
> struct
> > +rte_ecpri_msg_gen_data {
> > +	rte_be32_t pc_id;		/**< Physical channel ID */
> > +	rte_be32_t seq_id;		/**< Sequence ID */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #4: Remote Memory Access  */
> > +RTE_STD_C11
> > +struct rte_ecpri_msg_rm_access {
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +	uint32_t ele_id:16;		/**< Element ID */
> > +	uint32_t rr:4;			/**< Req/Resp */
> > +	uint32_t rw:4;			/**< Read/Write */
> > +	uint32_t rma_id:8;		/**< Remote Memory Access
> ID */
> > +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +	uint32_t rma_id:8;		/**< Remote Memory Access
> ID */
> > +	uint32_t rw:4;			/**< Read/Write */
> > +	uint32_t rr:4;			/**< Req/Resp */
> > +	uint32_t ele_id:16;		/**< Element ID */
> > +#endif
> > +	rte_be16_t addr_m;		/**< 48-bits address (16 MSB)
> */
> > +	rte_be32_t addr_l;		/**< 48-bits address (32 LSB) */
> > +	rte_be16_t length;		/**< number of bytes */
> > +} __rte_packed;
> > +
> > +/**
> > + * eCPRI Message Header of Type #5: One-Way Delay Measurement
> */
> > +struct rte_ecpri_msg_delay_measure {
> > +	uint8_t msr_id;			/**< Measurement ID */
> > +	uint8_t act_type;		/**< Action Type */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #6: Remote Reset  */ struct
> > +rte_ecpri_msg_remote_reset {
> > +	rte_be16_t rst_id;		/**< Reset ID */
> > +	uint8_t rst_op;			/**< Reset Code Op */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header of Type #7: Event Indication  */ struct
> > +rte_ecpri_msg_event_ind {
> > +	uint8_t evt_id;			/**< Event ID */
> > +	uint8_t evt_type;		/**< Event Type */
> > +	uint8_t seq;			/**< Sequence Number */
> > +	uint8_t number;			/**< Number of
> Faults/Notif */
> > +};
> > +
> > +/**
> > + * eCPRI Message Header Format: Common Header + Message
> Types  */
> > +RTE_STD_C11
> > +struct rte_ecpri_msg_hdr {
> > +	union {
> > +		struct rte_ecpri_common_hdr common;
> > +		uint32_t dw0;
> > +	};
> > +	union {
> > +		struct rte_ecpri_msg_iq_data type0;
> > +		struct rte_ecpri_msg_bit_seq type1;
> > +		struct rte_ecpri_msg_rtc_ctrl type2;
> > +		struct rte_ecpri_msg_bit_seq type3;
> > +		struct rte_ecpri_msg_rm_access type4;
> > +		struct rte_ecpri_msg_delay_measure type5;
> > +		struct rte_ecpri_msg_remote_reset type6;
> > +		struct rte_ecpri_msg_event_ind type7;
> > +		uint32_t dummy[3];
> > +	};
> > +};
> 
> What is the point in having this struct?
> 
> From a software point of view, I think it is a bit risky, because its size is
> the size of the largest message. This is probably what you want in your
> case, but when a software will rx or tx such packet, I think they
> shouldn't use this one. My understanding is that you only need this
> structure for the mask in rte_flow.
> 
> Also, I'm not sure to understand the purpose of dummy[3], even after
> reading your answer to Akhil's question.
> 

Basically YES and no. To my understanding, the eCPRI message format is something
like the ICMP packet format. The message (packet) itself will be parsed into
different formats based on the type of the common header. In the message
payload part, there is no distinct definition of the "sub-header". We can divide
them into the sub-header and data parts based on the specification.
E.g. physical channel ID / real-time control ID / Event ID + type are the parts
that datapath forwarding will only care about. The following timestamp or user data
parts are the parts that the higher layer in the application will use.
1. If an application wants to create some offload flow, or even handle it in the SW, the
common header + first several bytes in the payload should be enough. BUT YES, it is
not good or safe to use it in the higher layer of the application.
2. A higher layer of the application should have its own definition of the whole payload
of a specific sub-type, including the parsing of the user data part after the "sub-header".
It is better for them just skip the first 4 bytes of the eCPRI message or a known offset.
We do not need to cover the upper layers. 
I think some comments could be added here, is it OK?
3. Regarding this structure, I add it because I do not want to introduce a lot of new items
in the rte_flow: new items with structures, new enum types. I prefer one single structure
will cover most of the cases (subtypes). What do you think?
4. About the *dummy* u32, I calculated all the "subheaders" and choose the maximal value
of the length. Two purposes (same as the u32 in the common header):
  a. easy to swap the endianness, but not quite useful. Because some parts are u16 and u8,
    and should not be swapped in a u32. (some physical channel ID and address LSB have 32bits width)
    But if some HW parsed the header u32 by u32, then it would be helpful.
  b. easy for checking in flow API, if the user wants to insert a flow. Some checking should
      be done to confirm if it is wildcard flow (all eCPRI messages or eCPRI message in some specific type),
      or some precise flow (to match the pc id or rtc id, for example). With these fields, 3 DW
      of the masks only need to be check before continuing. Or else, the code needs to check the type
      and a lot of switch-case conditions and go through all different members of each header.

> 
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_ECPRI_H_ */
> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index 0ae4e75..184a3f9 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -304,6 +304,7 @@ struct rte_vlan_hdr {  #define
> RTE_ETHER_TYPE_LLDP
> > 0x88CC /**< LLDP Protocol. */  #define RTE_ETHER_TYPE_MPLS
> 0x8847 /**<
> > MPLS ethertype. */  #define RTE_ETHER_TYPE_MPLSM 0x8848 /**<
> MPLS
> > multicast ethertype. */
> > +#define RTE_ETHER_TYPE_ECPRI 0xAEFE /**< eCPRI ethertype (.1Q
> > +supported). */
> >
> >  /**
> >   * Extract VLAN tag information into mbuf
> > --
> > 1.8.3.1
> >