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

[Olivier Matz <olivier.matz@6wind.com>]

Hi Bing,

On Sat, Jul 11, 2020 at 04:25:49AM +0000, Bing Zhao wrote:
> Hi Olivier,
> Many thanks for your comments.

[...]

> > > +/**
> > > + * 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

What about using a bitfield into a uint64_t ? I mean:

	struct rte_ecpri_msg_rm_access {
if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
		...
		uint64_t length:16;		/**< number of bytes */
		uint64_t addr:48;		/**< address */
#else
		...
		uint64_t addr:48;		/**< address */
		uint64_t length:16;		/**< number of bytes */
#endif
	};


> 
> > 
> > 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"?

In my opinion, u32 is more usual than 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. 

Let me explain what is my vision of how an application would use the
structures (these are completly dummy examples, as I don't know ecpri
protocol at all).

Rx:

	int ecpri_input(struct rte_mbuf *m)
	{
		struct rte_ecpri_common_hdr hdr_copy, *hdr;
		struct rte_ecpri_msg_event_ind event_copy, *event;
		struct app_specific app_copy, *app;

		hdr = rte_pktmbuf_read(m, 0, sizeof(*hdr), &hdr_copy);
		if (unlikely(hdr == NULL))
			return -1;
		switch (hdr->type) {
		...
		case RTE_ECPRI_EVT_IND_NTFY_IND:
			event = rte_pktmbuf_read(m, sizeof(*hdr), sizeof(*event),
				&event_copy);
			if (unlikely(event == NULL))
				return -1;
			...
			app = rte_pktmbuf_read(m, sizeof(*app),
				sizeof(*hdr) + sizeof(*event),
				&app_copy);
			...

Tx:

	int ecpri_output(void)
	{
		struct rte_ecpri_common_hdr *hdr;
		struct rte_ecpri_msg_event_ind *event;
		struct app_specific *app;

		m = rte_pktmbuf_alloc(mp);
		if (unlikely(m == NULL))
			return -1;

		app = rte_pktmbuf_append(m, sizeof(*data));
		if (app == NULL)
			...
		app->... = ...;
		...
		event = rte_pktmbuf_prepend(m, sizeof(*event));
		if (event == NULL)
			...
		event->... = ...;
		...
		hdr = rte_pktmbuf_prepend(m, sizeof(*hdr));
		if (hdr == NULL)
			...
		hdr->... = ...;

		return packet_send(m);
	}

In these 2 examples, we never need the unioned structure (struct
rte_ecpri_msg_hdr).

Using it does not look possible to me, because its size is corresponds
to the largest message, not to the one we are parsing/building.

> 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.

Thanks for the clarification.

I'll tend to say that if the rte_ecpri_msg_hdr structure is only
useful for rte_flow, it should be defined inside rte_flow.

However, I have some fears about the dummy[3]. You said it could be
enlarged later: I think it is dangerous to change the size of a
structure that may be used to parse data (and this would be an ABI
change). Also, it seems dummy[3] cannot be used easily to swap
endianness, so is it really useful?


Thanks,
Olivier


> > > +
> > > +#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
> > >
>