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 408BEA050B; Thu, 7 Apr 2022 12:52:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DE5EE4068B; Thu, 7 Apr 2022 12:52:24 +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 E0E1D40689 for ; Thu, 7 Apr 2022 12:52:22 +0200 (CEST) Received: by mail-io1-f51.google.com with SMTP id z6so6345935iot.0 for ; Thu, 07 Apr 2022 03:52:22 -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; bh=nV/Fl9v+In9yanl4wHkmd00vB4puJ3TGpn1aHRpNB9U=; b=KJOGIplUAhMIKiElqbxyKpr/g21XdEgWH7ISWrBqnxV3yCweXVE6tMiPxrMrskZyM5 cd0VEKHjtBBv4sMbUyJe/gL8FUNVn+ux9UqFRqdFn78nHRW3obh9jDXJXjjzO2CE2jxm ViwCqviKY2TZU2bPeIYTVv6LWn8dBqHgbFi9316yexpbodJS5Z9hJ1EwPsCwyZ2ASdo/ 5BsYMOstUPUkt4QQOi4NSoxiOxJCI3jct7hJCZLRGAP670XUAXZ0o+02o7zHDTQXFrVQ vNqYDTIYzm6S/8kQB+gXWsjOIkGr4pFHkyCNcLzu8KfzMnujcyRACLB9wMo7YxzI/gQe phtg== 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; bh=nV/Fl9v+In9yanl4wHkmd00vB4puJ3TGpn1aHRpNB9U=; b=eXrfMdt/W4RpfjEz7PLSJjaPu8WvNo5sUEJhIZ+Tkkk5bfMoj8Lt0WzQJ1dx9kP2cq XJBj3CIMSEOnFABjfLgtdxoWQCXNZqcSr6rDji5fNHjfaEMdsA9PKU1B6C/IXAb4PFub it2TKVXJRN5uzv8jyHMXrN0z7h0wHVfhCofcBHG3ZpiV6zqwZzOR5WaSJikTg4FFHs1P QOvPr0dv6Td7XmPGogNwa7QBTu4+C/aZ2nPT/EgPgGasbDwVtSWnMsbjJyCTBRIdBh99 DqqC7kokQDXhLW6s0vy2sM/4ToPr4gSMwTqP1kkLb0XkhFx8dFPLMJ0uY2fNgIYl1zJM J7MQ== X-Gm-Message-State: AOAM533nBMKMI2UXRaVd0rKpxpRvaoDirB3Yqvt0J0cIA8RBnQr+zvyk zDYZSMkfp92eSc4NiyxZp5Z/DXnIyJn8Ih6TNJA= X-Google-Smtp-Source: ABdhPJwZbM5UCmEZR52yxexoWqGbwId31DTCKFNWmLjSX3kZAfqR/WFzRcyt8ONolrdWcNguF+MrNbDhnWonQZpd+Ao= X-Received: by 2002:a05:6638:268b:b0:324:2a07:3463 with SMTP id o11-20020a056638268b00b003242a073463mr502778jat.280.1649328742136; Thu, 07 Apr 2022 03:52:22 -0700 (PDT) MIME-Version: 1.0 References: <20220214120246.4181470-1-jerinj@marvell.com> <20220301085824.1041009-1-skori@marvell.com> In-Reply-To: From: Jerin Jacob Date: Thu, 7 Apr 2022 16:21:56 +0530 Message-ID: Subject: Re: [PATCH v3 1/1] ethdev: mtr: support input color selection To: "Dumitrescu, Cristian" Cc: "skori@marvell.com" , Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , Ray Kinsella , "dev@dpdk.org" , Jerin Jacob Content-Type: text/plain; charset="UTF-8" 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 0 0 0 0 0 0 0 0 On Tue, Mar 1, 2022 at 11:18 PM Dumitrescu, Cristian wrote: > > HI Jerin, Hi Cristian, > > Thanks for your patch! I think we are making great progress, here are a few more comments: > > > > > +/** > > + * Input color method > > + */ > > +enum rte_mtr_input_color_method { > > We should clean up the names of these methods a bit: we should not mix header names (VLAN, IP) with header field names (DSCP, PCP), in the sense that to me METHOD_VLAN_DSCP should be replaced with either: > * METHOD_OUTER_VLAN_IP :shorter name, as only the headers are mentioned (my preference, but I am OK with both) OK, We will keep VLAN and IP. By default OUTER is implicit in other DPDK API spec,i.e if not mentioned, it is outer. Hence I removed the outer. I can add outer explicit if you think in that way. See last comment. > * METHOD_OUTER_VLAN_PCP_IP_DSCP: longer name, as both the headers and the header fields are mentioned > > Please put a blank line in between these methods to better readability. > > I see some issues in the list of methods below, I am trying to do my best to catch them all: Thanks. Sorry for the delay in reply. > > > + /** > > + * The input color is always green. > > + * The default_input_color is ignored for this method. > > + * @see struct rte_mtr_params::default_input_color > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_COLOR_BLIND = RTE_BIT64(0), > > OK. > > > + /** > > + * If the input packet has at least one VLAN label, its input color 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. > > + * @see struct rte_mtr_params::default_input_color > > + * @see struct rte_mtr_params::vlan_table > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_VLAN = RTE_BIT64(1), > > OK. > Does your HW use PCP+DEI , or just PCP? PCP + DEI > > > + /** > > + * If the input packet is IPv4 or IPv6, its input color is detected 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 > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_DSCP = RTE_BIT64(2), > > OK. > Please change name to METHOD_IP. > Description: Change the "outermost DSCP" to "the DSCP field of the outermost IP header". OK > I would move this up on the second position (to follow immediately after the color blind method). Please check the summary below. > > > + /** > > + * If the input packet has at least one VLAN label, its input color is > > + * detected by the outermost VLAN DEI(1bit), PCP(3 bits) > > + * indexing into the struct rte_mtr_params::vlan_table. > > + * If the input packet is IPv4 or IPv6, its input color is detected 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::vlan_table > > + * @see struct rte_mtr_params::dscp_table > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_VLAN_DSCP = RTE_BIT64(3), > > OK. > Please change name to METHOD_VLAN_IP. OK > This should follow immediately after the METHOD_VLAN. OK > Description: please use "Otherwise" before "if the input packet is IP"; please replace "outermost DSCP" as above. OK > Is your HW using DEI + PCP or just PCP? OK > > > + /** > > + * If the input packet has at least one VLAN label, its input color 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 > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN = RTE_BIT64(4), > > OK. > Is your HW using DEI + PCP or just PCP? DEI + PCP > > > + /** > > + * If the input packet is IPv4 or IPv6, its input color is detected 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 > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_INNER_DSCP = RTE_BIT64(5), > > This is very confusing to me, I don't get what this one is about: The "inner" word in the name suggests that inner VLAN is attempted first, then IP DSCP (if no VLAN is present), but the description only talks about IP. This case attempts only inner IP DSCP. VLAN does not matter. > > > + /** > > + * If the input packet has at least one VLAN label, its input color is > > + * detected by the innermost VLAN DEI(1bit), PCP(3 bits) > > + * indexing into the struct rte_mtr_params::vlan_table. > > + * If the input packet is IPv4 or IPv6, its input color is detected 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::vlan_table > > + * @see struct rte_mtr_params::dscp_table > > + */ > > + RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_DSCP = > > RTE_BIT64(6), > > OK. > Description fixes: Use "otherwise" before "if IP"; replace innermost DSCP with "DSCP field of the outermost IP header". OK. To summarize we have 4 attributes, Please find below the truth table 1) Outer VLAN 2) Outer IP 3) Inner VLAN 4) Inner IP Inner IP -Inner VLAN- Outer IP-Outer VLAN 0 0 0 0 - Not valid case 0 0 0 1 - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN 0 0 1 0 - RTE_MTR_INPUT_COLOR_METHOD_OUTER_IP 0 0 1 1 - RTE_MTR_INPUT_COLOR_METHOD_OUTER_VLAN_OUTER_IP - If found outer VLAN then vlan else outer IP 0 1 0 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN 0 1 0 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_VLAN - If found inner VLAN else outer VLAN 0 1 1 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP 0 1 1 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_VLAN_OUTER_IP_OUTER_VLAN - If found inner vlan then inner vlan else outer IP else outer VLAN 1 0 0 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP 1 0 0 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_VLAN 1 0 1 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP 1 0 1 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_OUTER_IP_OUTER_VLAN 1 1 0 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN 1 1 0 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_VLAN 1 1 1 0 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP 1 1 1 1 - RTE_MTR_INPUT_COLOR_METHOD_INNER_IP_INNER_VLAN_OUTER_IP_OUTER_VLAN Is this above enumeration fine, If not, Please suggest. In Interms of name, a) We could omit explicit OUTER to reduce the length as suggestion. b) or change IIP, OIP, IVLAN, OVLAN kind of scheme to reduce the name. Let me know the names and enumeration you prefer, I will change accordingly in the next version? > > > > Regards, > Cristian