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 5645BA053A; Sun, 12 Jul 2020 16:43:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5ADDD1D544; Sun, 12 Jul 2020 16:43:56 +0200 (CEST) Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by dpdk.org (Postfix) with ESMTP id C48E81D3F0 for ; Sun, 12 Jul 2020 16:43:54 +0200 (CEST) Received: by mail-wr1-f50.google.com with SMTP id o11so10373486wrv.9 for ; Sun, 12 Jul 2020 07:43:54 -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=Vmvv8ZhG+9P8zFQdUmOE6G6igPWRtS27OljrhsCHDvA=; b=EEUMtQFAH1m5fC1Bkv806OfHyVX1/k6pgcrqQtczxvCe8TE/zJpgg7gv/YRPIj0TKP kNegjQPSkudYcWKGQkGQf9YFt4oVAOnivkZIRq1YVUESZSuUG2Mw7pNuil9cW+sTpQ5s ZFffQUGkb78FQJEWFC3q04Eqj2BBJBstnB/rxnBcopqnLegatLOlEtEmVA9LHs1CygJV 1JjpyrTod1p4RkKHjhICT+0boJPmNQ7WFFkzX3Z2ARlo/nN8s5RBBDhLZSh3s0k0JyGZ nzYnzBGB9gSXjkLLX9ncJISYFhddRKvfRq6GS1SfEFb4HmED6IsgUqVhyBaGt69Crwi3 /V+g== 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=Vmvv8ZhG+9P8zFQdUmOE6G6igPWRtS27OljrhsCHDvA=; b=mNbYaq5kHaX+AC4Q1qhm1AJUvpRQPSGIZ2s3V0+9d6pdq2aKuhRhJUoz+BfdiPF+gU 7ZLRJVsOWDxebJTTQ7FBYGYddbbor4/JNP5TVJUVm75uZzIv3RJFzc2kePPJZBwxa7gT acV1LAgtJq4HXtVKiCjvNRb92A6hRQX1V0yui2cgkDsiO1ZkD1m+Kikf2Ym6OLvZNIDt XbKasF5eHVJi4wLvkWUgufe9xVxIwBRnDnj7vzkasa/HLcNoY9YjJfs3FZu2vnVZqD61 Mud4UaYyuXjx5HYSb9csX4i8AZqZg6GZz8pjCN4PtsbBN1hdKh2gHkaF4xTA2Y9DvHX5 kLHQ== X-Gm-Message-State: AOAM531rgb5jXqYoOm7dleiBqel7NXJo+mZyonFAsEpoJlvovMX8aVmV 3nWTxVy2M8U++dZ5C1DfUbDhbA== X-Google-Smtp-Source: ABdhPJzw6fSUpVnKFza7iCCQiSucSZBcr6ALlnRsyfmq73/gqf03KQFDuJqp2TntY9lr+dAPq675mw== X-Received: by 2002:a5d:6a46:: with SMTP id t6mr78465825wrw.374.1594565034345; Sun, 12 Jul 2020 07:43:54 -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 g195sm18780224wme.38.2020.07.12.07.43.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 12 Jul 2020 07:43:53 -0700 (PDT) Date: Sun, 12 Jul 2020 16:43:53 +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: <20200712144353.GJ5869@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> <20200712131740.GI5869@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" On Sun, Jul 12, 2020 at 02:28:03PM +0000, Bing Zhao wrote: > Hi Olivier, > Thanks > > BR. Bing > > > -----Original Message----- > > From: Olivier Matz > > Sent: Sunday, July 12, 2020 9:18 PM > > 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 > > Subject: Re: [PATCH v5 1/2] rte_flow: add eCPRI key fields to flow API > > > > 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 > > }; > > > > Thanks for your suggestion. > https://stackoverflow.com/questions/10906238/warning-when-using-bitfield-with-unsigned-char > AFAIK (from this page), bitfields support only support bool and int. uint64_t is some type of "long" > and most of the compilers should support it. But I am not sure if it is a standard implementation. The uint8_t[6], as in your v6, is a good idea. > > > > > > > > > > > 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. > > > > I sent patch set v6 with this change, thanks. > > > > > > > > > > > > > + > > > > > +/** > > > > > + * 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. > > > > Yes, in the cases, we do not need the unioned structures at all. > Since the common header is always 4 bytes, an application could use the > sub-types structures started from an offset of 4 of eCPRI layer, as in your example. > This is in the datapath. My original purpose is for some "control path", typically > the flow (not sure if any other new lib implementation) API, then the union > could be used there w/o treating the common header and message payload > header in a separate way and then combine them together. In this case, only > the first several bytes will be accessed and checked, there will be no change > of message itself, and then just passing it to datapath for further handling as > in your example. > > > > 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. > > > > Right now, yes it will be used in rte_flow. But I checked the current implementations > of each item, almost all the headers are defined in their own protocol files. So in v6, > I change the name of it, in order not to confuse the users of this API, would it be OK? > Thanks OK > > > 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? > > > > It might be enlarger but not for now, until a new revision of this specification. For > all the subtypes it has right now, the specification will remain them as same as today. > Only new types will be added then. So after several years, we may consider to change it > in the LTS. Is it OK? OK, I think in this case it may even be another structure > In most cases, the endianness swap could be easy, we will swap it in each DW / u32. Tome > the exception is that some field crosses the u32 boundary, like the address in type#4, we may > treat it separately. And the most useful case is for the mask checking, it could simplify the > checking to at most 3 (u32==0?) without going through each member of different types. > > And v6 already sent, I change the code based on your suggestion. Would you please > help to give some comments also? > > Appreciate for your help and suggestion. > > > > > 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 > > > > > > > >