From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id A6D0FA052A;
Sun, 12 Jul 2020 15:17:45 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 839051D555;
Sun, 12 Jul 2020 15:17:44 +0200 (CEST)
Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com
[209.85.128.68]) by dpdk.org (Postfix) with ESMTP id 17B6F1D52F
for <dev@dpdk.org>; Sun, 12 Jul 2020 15:17:43 +0200 (CEST)
Received: by mail-wm1-f68.google.com with SMTP id o8so10612260wmh.4
for <dev@dpdk.org>; Sun, 12 Jul 2020 06:17:43 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to:user-agent;
bh=y0qdodAYTLBn8KxEIWdldoMYtex1I15new2tbRa6RFE=;
b=Nq41es1eyRL9/Ds8ADJ2ajiylWQ2ocdof5rULVRrvezRgvbREWAZZT1NUy05wFL2NL
OdsPJj5hb6s+RT9Vs3cNlVzNet8KOU75iuDKsMnPq8Ju85gfvdtFpJeWZbDt9422wwyG
DqcPy0Y7h50O9LdreNku+Hxu75Ooez0wYourCIqUtPlqHvJIxlULe4toT4N+8cVl5hIs
BwShIoEPEB/ZpN40tBFYhJwCeHzzXIualISG0cOK3bYjK6re1a43AhjZLnyVmBqFUlxP
8sgI4OiswuR890MJtD+4Sb7FW+KQrwf3Ukb/Mnpe33BMnTAKi+qlV2B0wlPfFxU0WBIi
I0dg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to:user-agent;
bh=y0qdodAYTLBn8KxEIWdldoMYtex1I15new2tbRa6RFE=;
b=JakjicmnDGJLku/F7kEP58J8ItsgRVz45xcUgcCSUBVFXjZwSp2aQuC+QIqDKwq0Sg
syoOty5ZXdUT4YzAQXr+x8lNa11O0cmQAyrMU3JCuF0PQ0EvQ385E3W57JOYW1Q3WOah
fHnpTMlUasUhPrL/8LHZjxKpFueVir5xtfEFkhuqR3RTsoJnGM5cbEk72wQyGj/L/h7h
Wrj6QMNNtqn40OukpbJyorFTNC7ldo2vgkv9/ozjCztuUEQvlmTfGtjXwnsB+oy0F2bG
osn50sbFzfUoqr6+NwifxolF5te+KzRLAGr6utvy56uBEtHWtWERA2OCTiltY2PEkne5
d1Dg==
X-Gm-Message-State: AOAM532zrA3oavzKWgijSTwv8nXuhp9xm1VoD7fM6rCDS+4iedtnTKYZ
FUguoosIlEqP7kZwlkzHvJondg==
X-Google-Smtp-Source: ABdhPJw/HxeC6wqPQiWdxpPzdM52x5WE/wdqSeNgZww0K5pBFJiSM4JEnHLr/4b8dD0lDOFXcalk6g==
X-Received: by 2002:a7b:c763:: with SMTP id x3mr13631380wmk.47.1594559862388;
Sun, 12 Jul 2020 06:17:42 -0700 (PDT)
Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr.
[2a01:cb0c:5:a600:3456:36f7:e65e:d1a0])
by smtp.gmail.com with ESMTPSA id k20sm18201127wmi.27.2020.07.12.06.17.41
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Sun, 12 Jul 2020 06:17:41 -0700 (PDT)
Date: Sun, 12 Jul 2020 15:17:40 +0200
From: Olivier Matz <olivier.matz@6wind.com>
To: Bing Zhao <bingz@mellanox.com>
Cc: Ori Kam <orika@mellanox.com>,
"john.mcnamara@intel.com" <john.mcnamara@intel.com>,
"marko.kovacevic@intel.com" <marko.kovacevic@intel.com>,
Thomas Monjalon <thomas@monjalon.net>,
"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
"akhil.goyal@nxp.com" <akhil.goyal@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>,
"wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>,
"beilei.xing@intel.com" <beilei.xing@intel.com>,
"bernard.iremonger@intel.com" <bernard.iremonger@intel.com>
Message-ID: <20200712131740.GI5869@platinum>
References: <1594136219-133336-1-git-send-email-bingz@mellanox.com>
<1594370723-343354-1-git-send-email-bingz@mellanox.com>
<1594370723-343354-2-git-send-email-bingz@mellanox.com>
<20200710143123.GE5869@platinum>
<VI1PR05MB419258E42CF4981A6D27113FDD620@VI1PR05MB4192.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <VI1PR05MB419258E42CF4981A6D27113FDD620@VI1PR05MB4192.eurprd05.prod.outlook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Subject: Re: [dpdk-dev] [PATCH v5 1/2] rte_flow: add eCPRI key fields to
flow API
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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
> > >
>