From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Sun, 12 Jul 2020 15:17:43 +0200 (CEST) Received: by mail-wm1-f68.google.com with SMTP id o8so10612260wmh.4 for ; 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 To: Bing Zhao Cc: Ori Kam , "john.mcnamara@intel.com" , "marko.kovacevic@intel.com" , Thomas Monjalon , "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" 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > > >