From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 637BAA0509; Sun, 1 May 2022 14:57:15 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 415EF4069D; Sun, 1 May 2022 14:57:15 +0200 (CEST) Received: from mail-io1-f51.google.com (mail-io1-f51.google.com [209.85.166.51]) by mails.dpdk.org (Postfix) with ESMTP id 155FC4003F for ; Sun, 1 May 2022 14:57:13 +0200 (CEST) Received: by mail-io1-f51.google.com with SMTP id f2so13640857ioh.7 for ; Sun, 01 May 2022 05:57:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=FnF4J4pjkNNzsQwd9hSInSQ+ZgYx05G1dUrO7rLQIw4=; b=jB1rpeQIm8D9HlHKnghVYSyZnhQDURjlpHt3KDwEXyDAErIXhNwC69m8d8HgOh8aZx CWSEfqvO9V1fCqvOyploVI1Pi9VTjFo0MXOdOYAfx2/+wosL7Y/TOGW9a/Fr0+ixwjGg vCxnWEfmlG2aOrM6448Aa4Urxj10wcQRymSrtolqXjSfPh3Hn7ZYSrNh7Gvlm+l50h0s 2fg9uRFH17gkioE00QvkCSCJt5VhdVij8b8zxRhT3gkcmH2Hpa4LZ7IZExk5Okevo3mS TcU1C2fb+JseY6lOOd/PyA6ggOEK3Ia4xdShlHKs2QY+bOIEcxaoeAHaUqyG1QWawcAD C5iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=FnF4J4pjkNNzsQwd9hSInSQ+ZgYx05G1dUrO7rLQIw4=; b=DUhF0Oc0tx+pQkC3dwNvWIi+GnHwXU9He62CBa9nbdMOoINWn1Wz2ggfPTXaQsSr2l c8NgjXrY8NYPp1Jt4LLApzdSvodzEbJIfDNVuLsWwiIBBP7aLXLWgIamINUad8SmIyyZ 13PZDmu1wDnq8P6ZR80ngrmXu+7RCyRpOUlWa3LA47c72C5IZ336Y5UNlBkuNcbvKqws STI9/XGh+a8hDyOOIIpbwPnDfy4arrsH8LWYr/vZ2gc31hauK1uAegGOAAGh/zSHzovc OkFoVFLOxTS2D7pW7BJwvvNbj8IO8DYJ9ghoDGwFtMTucESopsJgcSZ2DWC0BID/lyl7 OaLQ== X-Gm-Message-State: AOAM531ghLenuAm9vvzHtSKwIXnmvzMt8X5kcO8mQHUEwf6DUybX0Fxs LRaKED202lB8t4qpzyUyodVTzn3xmY798ehDjGg= X-Google-Smtp-Source: ABdhPJz5UQJLnQOA5v+0pRTUYWfeiUyor9koGnzyAlK1OVqoA4Ek291SRCYEcuyZsrG4YbME5n8O1TXqHryY8IZFVA0= X-Received: by 2002:a02:9483:0:b0:32b:1d:5d52 with SMTP id x3-20020a029483000000b0032b001d5d52mr3355482jah.158.1651409831834; Sun, 01 May 2022 05:57:11 -0700 (PDT) MIME-Version: 1.0 References: <20220301085824.1041009-1-skori@marvell.com> <20220421180241.514767-1-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Sun, 1 May 2022 18:26:45 +0530 Message-ID: Subject: Re: [dpdk-dev] [PATCH v4] ethdev: mtr: support protocol based input color selection To: "Dumitrescu, Cristian" Cc: "jerinj@marvell.com" , "dev@dpdk.org" , Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , Ray Kinsella , "ajit.khaparde@broadcom.com" , "aboyer@pensando.io" , "Xing, Beilei" , "Richardson, Bruce" , "chas3@att.com" , "Xia, Chenbo" , "Loftus, Ciara" , "dsinghrawat@marvell.com" , "ed.czeck@atomicrules.com" , "evgenys@amazon.com" , "grive@u256.net" , "g.singh@nxp.com" , "zhouguoyang@huawei.com" , "Wang, Haiyue" , "hkalra@marvell.com" , "heinrich.kuhn@corigine.com" , "hemant.agrawal@nxp.com" , "hyonkim@cisco.com" , "igorch@amazon.com" , "irusskikh@marvell.com" , "jgrajcia@cisco.com" , "Singh, Jasvinder" , "jianwang@trustnetic.com" , "jiawenwu@trustnetic.com" , "Wu, Jingjing" , "Daley, John" , "john.miller@atomicrules.com" , "linville@tuxdriver.com" , "Wiles, Keith" , "kirankumark@marvell.com" , "oulijun@huawei.com" , "lironh@marvell.com" , "longli@microsoft.com" , "mw@semihalf.com" , "spinler@cesnet.cz" , "matan@nvidia.com" , "Peters, Matt" , "maxime.coquelin@redhat.com" , "mk@semihalf.com" , "humin29@huawei.com" , "pnalla@marvell.com" , "ndabilpuram@marvell.com" , "Yang, Qiming" , "Zhang, Qi Z" , "radhac@marvell.com" , "rahul.lakkireddy@chelsio.com" , "rmody@marvell.com" , "Xu, Rosen" , "sachin.saxena@oss.nxp.com" , "skoteshwar@marvell.com" , "shshaikh@marvell.com" , "shaibran@amazon.com" , Shepard Siegel , "asomalap@amd.com" , "somnath.kotur@broadcom.com" , "sthemmin@microsoft.com" , "Webster, Steven" , "skori@marvell.com" , "mtetsuyah@gmail.com" , "vburru@marvell.com" , "viacheslavo@nvidia.com" , "Wang, Xiao W" , "cloud.wangxiaoyun@huawei.com" , "yisen.zhuang@huawei.com" , "Wang, Yong" , "xuanziyang2@huawei.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Apr 26, 2022 at 5:38 PM Dumitrescu, Cristian wrote: > > Hi Jerin, Hi Cristian > > Thank you for implementing according to our agreement, I am happy to see = that we are converging. :-) > > Here are some comments below: > > > > > diff --git a/lib/ethdev/rte_mtr.h b/lib/ethdev/rte_mtr.h > > index 40df0888c8..76ffbcf724 100644 > > --- a/lib/ethdev/rte_mtr.h > > +++ b/lib/ethdev/rte_mtr.h > > @@ -213,6 +213,52 @@ struct rte_mtr_meter_policy_params { > > const struct rte_flow_action *actions[RTE_COLORS]; > > }; > > > > +/** > > + * Input color protocol method > > I suggest adding some more explanations here: > More than one method can be enabled for a given meter. Even if enabled, a= method might not be applicable to each input packet, in case the associate= d protocol header is not present in the packet. The highest priority method= that is both enabled for the meter and also applicable for the current inp= ut packet wins; if none is both enabled and applicable, the default input c= olor is used. @see function rte_mtr_color_in_protocol_priority_set() OK. > > > + */ > > +enum rte_mtr_color_in_protocol { > > + /** > > + * If the input packet has at least one VLAN label, its input col= or is > > + * detected by the outermost VLAN DEI(1bit), PCP(3 bits) > > + * indexing into the struct rte_mtr_params::vlan_table. > > + * Otherwise, the *default_input_color* is applied. > > + * > > The statement "Otherwise, the *default_input_color* is applied" is incorr= ect IMO and should be removed, as multiple methods might be enabled and als= o applicable to a specific input packet, in which case the highest priority= method wins, as opposed to the default input color. > > I suggest a simplification "Enable the detection of the packet input colo= r based on the outermost VLAN header fields DEI (1 bit) and PCP (3 bits). T= hese fields are used as index into the VLAN table" OK > > > + * @see struct rte_mtr_params::default_input_color > > + * @see struct rte_mtr_params::vlan_table > > + */ > > + RTE_MTR_COLOR_IN_PROTO_OUTER_VLAN =3D RTE_BIT64(0), > > + /** > > + * If the input packet has at least one VLAN label, its input col= or is > > + * detected by the innermost VLAN DEI(1bit), PCP(3 bits) > > + * indexing into the struct rte_mtr_params::vlan_table. > > + * Otherwise, the *default_input_color* is applied. > > + * > > + * @see struct rte_mtr_params::default_input_color > > + * @see struct rte_mtr_params::vlan_table > > + */ > > Same simplification suggested here. OK > > > + RTE_MTR_COLOR_IN_PROTO_INNER_VLAN =3D RTE_BIT64(1), > > + /** > > + * If the input packet is IPv4 or IPv6, its input color is detect= ed by > > + * the outermost DSCP field indexing into the > > + * struct rte_mtr_params::dscp_table. > > + * Otherwise, the *default_input_color* is applied. > > + * > > + * @see struct rte_mtr_params::default_input_color > > + * @see struct rte_mtr_params::dscp_table > > + */ > > Same simplification suggested here. OK > > > + RTE_MTR_COLOR_IN_PROTO_OUTER_DSCP =3D RTE_BIT64(2), > > I am OK to keep DSCP for the name of the table instead of renaming the ta= ble, as you suggested, but this method name should reflect the protocol, no= t the field: RTE_MTR_COLOR_IN_PROTO_OUTER_IP. OK > > > + /** > > + * If the input packet is IPv4 or IPv6, its input color is detect= ed by > > + * the innermost DSCP field indexing into the > > + * struct rte_mtr_params::dscp_table. > > + * Otherwise, the *default_input_color* is applied. > > + * > > + * @see struct rte_mtr_params::default_input_color > > + * @see struct rte_mtr_params::dscp_table > > + */ > > Same simplification suggested here. OK > > > + RTE_MTR_COLOR_IN_PROTO_INNER_DSCP =3D RTE_BIT64(3), > > I am OK to keep DSCP for the name of the table instead of renaming the ta= ble, as you suggested, but this method name should reflect the protocol, no= t the field: RTE_MTR_COLOR_IN_PROTO_INNER_IP. OK > > > + > > /** > > * Parameters for each traffic metering & policing object > > * > > @@ -233,20 +279,58 @@ struct rte_mtr_params { > > */ > > int use_prev_mtr_color; > > > > - /** Meter input color. When non-NULL: it points to a pre-allocate= d and > > + /** Meter input color based on IP DSCP protocol field. > > + * > > + * Valid when *input_color_proto_mask* set to any of the followin= g > > + * RTE_MTR_COLOR_IN_PROTO_OUTER_DSCP, > > + * RTE_MTR_COLOR_IN_PROTO_INNER_DSCP > > + * > > + * When non-NULL: it points to a pre-allocated and > > * pre-populated table with exactly 64 elements providing the inp= ut > > * color for each value of the IPv4/IPv6 Differentiated Services = Code > > - * Point (DSCP) input packet field. When NULL: it is equivalent t= o > > - * setting this parameter to an all-green populated table (i.e. t= able > > - * with all the 64 elements set to green color). The color blind = mode > > - * is configured by setting *use_prev_mtr_color* to 0 and *dscp_t= able* > > - * to either NULL or to an all-green populated table. When > > - * *use_prev_mtr_color* is non-zero value or when *dscp_table* > > contains > > - * at least one yellow or red color element, then the color aware= mode > > - * is configured. > > + * Point (DSCP) input packet field. > > + * > > + * When NULL: it is equivalent to setting this parameter to an al= l-green > > + * populated table (i.e. table with all the 64 elements set to gr= een > > + * color). The color blind mode is configured by setting > > + * *use_prev_mtr_color* to 0 and *dscp_table* to either NULL or t= o an > > + * all-green populated table. > > + * > > + * When *use_prev_mtr_color* is non-zero value or when > > *dscp_table* > > + * contains at least one yellow or red color element, then the co= lor > > + * aware mode is configured. > > + * > > + * @see enum > > rte_mtr_color_in_protocol::RTE_MTR_COLOR_IN_PROTO_OUTER_DSCP > > + * @see enum > > rte_mtr_color_in_protocol::RTE_MTR_COLOR_IN_PROTO_INNER_DSCP > > + * @see struct rte_mtr_params::input_color_proto_mask > > */ > > enum rte_color *dscp_table; > > - > > + /** Meter input color based on VLAN DEI(1bit), PCP(3 bits) protoc= ol > > + * fields. > > + * > > + * Valid when *input_color_proto_mask* set to any of the followin= g > > + * RTE_MTR_COLOR_IN_PROTO_OUTER_VLAN, > > + * RTE_MTR_COLOR_IN_PROTO_INNER_VLAN > > + * > > + * When non-NULL: it points to a pre-allocated and pre-populated > > + * table with exactly 16 elements providing the input color for > > + * each value of the DEI(1bit), PCP(3 bits) input packet field. > > + * > > + * When NULL: it is equivalent to setting this parameter to an > > + * all-green populated table (i.e. table with > > + * all the 16 elements set to green color). The color blind mode > > + * is configured by setting *use_prev_mtr_color* to 0 and > > + * *vlan_table* to either NULL or to an all-green populated table= . > > + * > > + * When *use_prev_mtr_color* is non-zero value > > + * or when *vlan_table* contains at least one yellow or > > + * red color element, then the color aware mode is configured. > > + * > > + * @see enum > > rte_mtr_color_in_protocol::RTE_MTR_COLOR_IN_PROTO_OUTER_VLAN > > + * @see enum > > rte_mtr_color_in_protocol::RTE_MTR_COLOR_IN_PROTO_INNER_VLAN > > + * @see struct rte_mtr_params::input_color_proto_mask > > + */ > > + enum rte_color *vlan_table; > > /** Non-zero to enable the meter, zero to disable the meter at th= e > > time > > * of MTR object creation. Ignored when the meter profile indicat= ed by > > * *meter_profile_id* is set to NONE. > > @@ -261,6 +345,25 @@ struct rte_mtr_params { > > > > /** Meter policy ID. @see rte_mtr_meter_policy_add() */ > > uint32_t meter_policy_id; > > + > > + /** Set of input color protocols to be enabled. > > + * > > + * Set value to zero to configure as color blind mode. > > + * > > + * When multiple bits set then rte_mtr_color_in_protocol_priority= _set() > > + * shall be used to set the priority, in the order, in which prot= ocol > > + * to be used to find the inpput color. > > + * > > + * @see enum rte_mtr_color_in_protocol > > + * @see rte_mtr_color_in_protocol_priority_set() > > + */ > > + uint64_t input_color_proto_mask; > > We should not have this as an input parameter at all, please remove this = field. This mask is implicitly created by the user by calling the rte_mtr_c= olor_in_protocol_priority_set() API function. If this function is called fo= r a given method, then that method is enabled with the given priority; when= this function is not called for a given method, then that method is disabl= ed. OK. Since we had rte_mtr_color_in_protocol_priority_set(), I was thinking it is only setting "priority". I will change to rte_mtr_color_in_protocol_set() and add API to _get() as w= ell. > > > + > > + /** Input color to be set for the input packet when none of the > > + * enabled input color methods is applicable to the input packet. > > + * Ignored when this when *input_color_proto_mask* set to zero. > > + */ > > + enum rte_color default_input_color; > > }; > > > > /** > > @@ -417,6 +520,16 @@ struct rte_mtr_capabilities { > > * @see enum rte_mtr_stats_type > > */ > > uint64_t stats_mask; > > + > > + /** Set of supported input color protocol. > > + * @see enum rte_mtr_color_in_protocol > > + */ > > + uint64_t input_color_proto_mask; > > Agree. > > > + > > + /** When non-zero, it indicates that driver supports separate > > + * input color table for given ethdev port. > > + */ > > + int separate_input_color_table_per_port; > > The input color tables are actually configured per meter object, do we al= so need a "separate_input_color_table_per_meter" capability flag? Yes. We have a similar limitation. > > > }; > > > > /** > > @@ -832,6 +945,59 @@ rte_mtr_meter_dscp_table_update(uint16_t port_id, > > enum rte_color *dscp_table, > > struct rte_mtr_error *error); > > > > +/** > > + * MTR object VLAN table update > > + * > > + * @param[in] port_id > > + * The port identifier of the Ethernet device. > > + * @param[in] mtr_id > > + * MTR object ID. Needs to be valid. > > + * @param[in] vlan_table > > + * When non-NULL: it points to a pre-allocated and pre-populated tab= le with > > + * exactly 16 elements providing the input color for each value of t= he > > + * each value of the DEI(1bit), PCP(3 bits) input packet field. > > + * When NULL: it is equivalent to setting this parameter to an "all-= green" > > + * populated table (i.e. table with all the 16 elements set to green= color). > > + * @param[out] error > > + * Error details. Filled in only on error, when not NULL. > > + * @return > > + * 0 on success, non-zero error code otherwise. > > + */ > > +__rte_experimental > > +int > > +rte_mtr_meter_vlan_table_update(uint16_t port_id, uint32_t mtr_id, > > + enum rte_color *vlan_table, > > + struct rte_mtr_error *error); > > +/** > > + * Set the priority for input color protocol > > + * > > + * When multiple bits set in struct rte_mtr_params::input_color_proto_= mask > > + * then this API shall be used to set the priority, in the order, in > > + * which protocol to be used to find the input color. > > As stated above, we should remove struct rte_mtr_params::input_color_prot= o_mask, as it is build implicitly by calling this function. > > I suggest reiterating the explanation from above: > More than one method can be enabled for a given meter. Even if enabled, a= method might not be applicable to each input packet, in case the associate= d protocol header is not present in the packet. The highest priority method= that is both enabled for the meter and also applicable for the current inp= ut packet wins; if none is both enabled and applicable, the default input c= olor is used. @see function rte_mtr_color_in_protocol_priority_set() OK > > > + * > > + * @param[in] port_id > > + * The port identifier of the Ethernet device. > > + * @param[in] mtr_id > > + * MTR object ID. Needs to be valid. > > + * @param[in] proto > > + * Input color protocol to apply priority. > > + * MTR object's *input_color_proto_mask* should be configured > > + * with proto value. > > + * @param[in] priority > > + * Input color protocol priority. Value zero indicates > > + * the highest priority. > > Agree. > > > + * @param[out] error > > + * Error details. Filled in only on error, when not NULL. > > + * @return > > + * 0 on success, non-zero error code otherwise. > > + * > > + * @see rte_mtr_params::input_color_proto_mask > > + */ > > +__rte_experimental > > +int > > +rte_mtr_color_in_protocol_priority_set(uint16_t port_id, uint32_t mtr_= id, > > + enum rte_mtr_color_in_protocol proto, uint32_t priority, > > + struct rte_mtr_error *error); > > /** > > * MTR object enabled statistics counters update > > * > > > > Regards, > Cristian