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 E38C9A04B7; Tue, 13 Oct 2020 17:42:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1C6581D941; Tue, 13 Oct 2020 17:42:09 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 678A21D56F for ; Tue, 13 Oct 2020 17:42:07 +0200 (CEST) IronPort-SDR: kkU8rkbISE5h1YczBtDx8mUmumGsVlfShkNWiWSEPPoJto3wq5GdRsKoiA2DIQ+cN5tLACMtVu 01BmpO52U6iQ== X-IronPort-AV: E=McAfee;i="6000,8403,9773"; a="163293942" X-IronPort-AV: E=Sophos;i="5.77,371,1596524400"; d="scan'208";a="163293942" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2020 08:42:03 -0700 IronPort-SDR: 75a5op0BqXlIEbVMFZCF1aYJq5jfEombP7ebfhwXY71PdRvmpfOulXshFaBs1TXdTVlr4QRFD3 /4a+bSP8il0w== X-IronPort-AV: E=Sophos;i="5.77,371,1596524400"; d="scan'208";a="463534318" Received: from vmedvedk-mobl.ger.corp.intel.com (HELO [10.213.214.142]) ([10.213.214.142]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2020 08:42:00 -0700 To: Michel Machado , Kevin Traynor , Ruifeng Wang , Bruce Richardson , Cody Doucette , Andre Nathan , Qiaobin Fu Cc: "dev@dpdk.org" , Honnappa Nagarahalli , nd References: <20200907081518.46350-1-ruifeng.wang@arm.com> <20200907081518.46350-3-ruifeng.wang@arm.com> <20200915160224.GA825@bricha3-MOBL.ger.corp.intel.com> <81eb8fde-cd4f-1df9-0ebb-05c902b30fe3@intel.com> <48834549-00e9-b762-4915-9a2dd0e5fe1d@redhat.com> <6497770e-9d1c-97c3-3834-84bd96186836@digirati.com.br> From: "Medvedkin, Vladimir" Message-ID: Date: Tue, 13 Oct 2020 16:41:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <6497770e-9d1c-97c3-3834-84bd96186836@digirati.com.br> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data 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 Michel, Could you please describe a condition when LPM gets inconsistent? As I can see if there is no free tbl8 it will return -ENOSPC. On 13/10/2020 15:58, Michel Machado wrote: > Hi Kevin, > >    We do need fields max_rules and number_tbl8s of struct rte_lpm, so > the removal would force us to have another patch to our local copy of > DPDK. We'd rather avoid this new local patch because we wish to > eventually be in sync with the stock DPDK. > >    Those fields are needed in Gatekeeper because we found a condition > in an ongoing deployment in which the entries of some LPM tables may > suddenly change a lot to reflect policy changes. To avoid getting into a > state in which the LPM table is inconsistent because it cannot fit all > the new entries, we compute the needed parameters to support the new > entries, and compare with the current parameters. If the current table > doesn't fit everything, we have to replace it with a new LPM table. > >    If there were a way to obtain the struct rte_lpm_config of a given > LPM table, it would cleanly address our need. We have the same need in > IPv6 and have a local patch to work around it (see > https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f). > Thus, an IPv4 and IPv6 solution would be best. > >    PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this > disscussion. > > [ ]'s > Michel Machado > > On 10/13/20 9:53 AM, Kevin Traynor wrote: >> Hi Gatekeeper maintainers (I think), >> >> fyi - there is a proposal to remove some members of a struct in DPDK LPM >> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but >> as it's an LTS I guess it would probably hit Debian in a few months. >> >> The full thread is here: >> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/ >> >> Maybe you can take a look and tell us if they are needed in Gatekeeper >> or you can workaround it? >> >> thanks, >> Kevin. >> >> [1] >> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248 >> >> >> On 09/10/2020 07:54, Ruifeng Wang wrote: >>> >>>> -----Original Message----- >>>> From: Kevin Traynor >>>> Sent: Wednesday, September 30, 2020 4:46 PM >>>> To: Ruifeng Wang ; Medvedkin, Vladimir >>>> ; Bruce Richardson >>>> >>>> Cc: dev@dpdk.org; Honnappa Nagarahalli >>>> ; nd >>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data >>>> >>>> On 16/09/2020 04:17, Ruifeng Wang wrote: >>>>> >>>>>> -----Original Message----- >>>>>> From: Medvedkin, Vladimir >>>>>> Sent: Wednesday, September 16, 2020 12:28 AM >>>>>> To: Bruce Richardson ; Ruifeng Wang >>>>>> >>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli >>>>>> ; nd >>>>>> Subject: Re: [PATCH 2/2] lpm: hide internal data >>>>>> >>>>>> Hi Ruifeng, >>>>>> >>>>>> On 15/09/2020 17:02, Bruce Richardson wrote: >>>>>>> On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote: >>>>>>>> Fields except tbl24 and tbl8 in rte_lpm structure have no need to >>>>>>>> be exposed to the user. >>>>>>>> Hide the unneeded exposure of structure fields for better ABI >>>>>>>> maintainability. >>>>>>>> >>>>>>>> Suggested-by: David Marchand >>>>>>>> Signed-off-by: Ruifeng Wang >>>>>>>> Reviewed-by: Phil Yang >>>>>>>> --- >>>>>>>>    lib/librte_lpm/rte_lpm.c | 152 >>>>>>>> +++++++++++++++++++++++--------------- >>>>>> - >>>>>>>>    lib/librte_lpm/rte_lpm.h |   7 -- >>>>>>>>    2 files changed, 91 insertions(+), 68 deletions(-) >>>>>>>> >>>>>>> >>>>>>>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h >>>>>>>> index 03da2d37e..112d96f37 100644 >>>>>>>> --- a/lib/librte_lpm/rte_lpm.h >>>>>>>> +++ b/lib/librte_lpm/rte_lpm.h >>>>>>>> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info { >>>>>>>> >>>>>>>>    /** @internal LPM structure. */ >>>>>>>>    struct rte_lpm { >>>>>>>> -    /* LPM metadata. */ >>>>>>>> -    char name[RTE_LPM_NAMESIZE];        /**< Name of the lpm. */ >>>>>>>> -    uint32_t max_rules; /**< Max. balanced rules per lpm. */ >>>>>>>> -    uint32_t number_tbl8s; /**< Number of tbl8s. */ >>>>>>>> -    struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**< >>>>>> Rule info table. */ >>>>>>>> - >>>>>>>>        /* LPM Tables. */ >>>>>>>>        struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES] >>>>>>>>                __rte_cache_aligned; /**< LPM tbl24 table. */ >>>>>>>>        struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */ >>>>>>>> -    struct rte_lpm_rule *rules_tbl; /**< LPM rules. */ >>>>>>>>    }; >>>>>>>> >>>>>>> >>>>>>> Since this changes the ABI, does it not need advance notice? >>>>>>> >>>>>>> [Basically the return value point from rte_lpm_create() will be >>>>>>> different, and that return value could be used by rte_lpm_lookup() >>>>>>> which as a static inline function will be in the binary and using >>>>>>> the old structure offsets.] >>>>>>> >>>>>> >>>>>> Agree with Bruce, this patch breaks ABI, so it can't be accepted >>>>>> without prior notice. >>>>>> >>>>> So if the change wants to happen in 20.11, a deprecation notice should >>>>> have been added in 20.08. >>>>> I should have added a deprecation notice. This change will have to >>>>> wait for >>>> next ABI update window. >>>>> >>>> >>>> Do you plan to extend? or is this just speculative? >>> It is speculative. >>> >>>> >>>> A quick scan and there seems to be several projects using some of these >>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS, >>>> gatekeeper. I didn't look at the details to see if they are really >>>> needed. >>>> >>>> Not sure how much notice they'd need or if they update DPDK much, but I >>>> think it's worth having a closer look as to how they use lpm and >>>> what the >>>> impact to them is. >>> Checked the projects listed above. BESS, NFF-Go and DPVS don't access >>> the members to be hided. >>> They will not be impacted by this patch. >>> But Gatekeeper accesses the rte_lpm internal members that to be >>> hided. Its compilation will be broken with this patch. >>> >>>> >>>>> Thanks. >>>>> Ruifeng >>>>>>>>    /** LPM RCU QSBR configuration structure. */ >>>>>>>> -- >>>>>>>> 2.17.1 >>>>>>>> >>>>>> >>>>>> -- >>>>>> Regards, >>>>>> Vladimir >>> >> -- Regards, Vladimir