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 7531BA0350; Mon, 29 Jun 2020 14:55:24 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 529AE1BEB5; Mon, 29 Jun 2020 14:55:23 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 804451BE90 for ; Mon, 29 Jun 2020 14:55:21 +0200 (CEST) IronPort-SDR: MIc+Z/UXbnDxUqX2d2v+LTydNzd4GZB6RJbDyMHulBa+jEmVJTfcerXzBjf19AjdQBUEK2ip+m i0T/tB7Pkobg== X-IronPort-AV: E=McAfee;i="6000,8403,9666"; a="134273710" X-IronPort-AV: E=Sophos;i="5.75,294,1589266800"; d="scan'208";a="134273710" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jun 2020 05:55:20 -0700 IronPort-SDR: BWWQNdNfW4HOk72K8+kpi6SvnS0EriItQ3psSYVkEPVJtFAnkViaux5A08gu3VXIGNYOboqKol aZUaJSrH1Vbg== X-IronPort-AV: E=Sophos;i="5.75,294,1589266800"; d="scan'208";a="454181705" Received: from bricha3-mobl.ger.corp.intel.com ([10.251.82.47]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 29 Jun 2020 05:55:17 -0700 Date: Mon, 29 Jun 2020 13:55:14 +0100 From: Bruce Richardson To: David Marchand Cc: Ruifeng Wang , Vladimir Medvedkin , John McNamara , Marko Kovacevic , Ray Kinsella , Neil Horman , dev , "Ananyev, Konstantin" , Honnappa Nagarahalli , nd Message-ID: <20200629125514.GC572@bricha3-MOBL.ger.corp.intel.com> References: <20190906094534.36060-1-ruifeng.wang@arm.com> <20200629080301.97515-1-ruifeng.wang@arm.com> <20200629080301.97515-2-ruifeng.wang@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v5 1/3] lib/lpm: integrate RCU QSBR 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 Mon, Jun 29, 2020 at 01:56:07PM +0200, David Marchand wrote: > On Mon, Jun 29, 2020 at 10:03 AM Ruifeng Wang wrote: > > > > Currently, the tbl8 group is freed even though the readers might be > > using the tbl8 group entries. The freed tbl8 group can be reallocated > > quickly. This results in incorrect lookup results. > > > > RCU QSBR process is integrated for safe tbl8 group reclaim. > > Refer to RCU documentation to understand various aspects of > > integrating RCU library into other libraries. > > > > Signed-off-by: Ruifeng Wang > > Reviewed-by: Honnappa Nagarahalli > > --- > > doc/guides/prog_guide/lpm_lib.rst | 32 +++++++ > > lib/librte_lpm/Makefile | 2 +- > > lib/librte_lpm/meson.build | 1 + > > lib/librte_lpm/rte_lpm.c | 129 ++++++++++++++++++++++++++--- > > lib/librte_lpm/rte_lpm.h | 59 +++++++++++++ > > lib/librte_lpm/rte_lpm_version.map | 6 ++ > > 6 files changed, 216 insertions(+), 13 deletions(-) > > > > diff --git a/doc/guides/prog_guide/lpm_lib.rst b/doc/guides/prog_guide/lpm_lib.rst > > index 1609a57d0..7cc99044a 100644 > > --- a/doc/guides/prog_guide/lpm_lib.rst > > +++ b/doc/guides/prog_guide/lpm_lib.rst > > @@ -145,6 +145,38 @@ depending on whether we need to move to the next table or not. > > Prefix expansion is one of the keys of this algorithm, > > since it improves the speed dramatically by adding redundancy. > > > > +Deletion > > +~~~~~~~~ > > + > > +When deleting a rule, a replacement rule is searched for. Replacement rule is an existing rule that has > > +the longest prefix match with the rule to be deleted, but has smaller depth. > > + > > +If a replacement rule is found, target tbl24 and tbl8 entries are updated to have the same depth and next hop > > +value with the replacement rule. > > + > > +If no replacement rule can be found, target tbl24 and tbl8 entries will be cleared. > > + > > +Prefix expansion is performed if the rule's depth is not exactly 24 bits or 32 bits. > > + > > +After deleting a rule, a group of tbl8s that belongs to the same tbl24 entry are freed in following cases: > > + > > +* All tbl8s in the group are empty . > > + > > +* All tbl8s in the group have the same values and with depth no greater than 24. > > + > > +Free of tbl8s have different behaviors: > > + > > +* If RCU is not used, tbl8s are cleared and reclaimed immediately. > > + > > +* If RCU is used, tbl8s are reclaimed when readers are in quiescent state. > > + > > +When the LPM is not using RCU, tbl8 group can be freed immediately even though the readers might be using > > +the tbl8 group entries. This might result in incorrect lookup results. > > + > > +RCU QSBR process is integrated for safe tbl8 group reclaimation. Application has certain responsibilities > > +while using this feature. Please refer to resource reclaimation framework of :ref:`RCU library ` > > +for more details. > > + > > Would the lpm6 library benefit from the same? > Asking as I do not see much code shared between lpm and lpm6. > > [...] > > > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c > > index 38ab512a4..41e9c49b8 100644 > > --- a/lib/librte_lpm/rte_lpm.c > > +++ b/lib/librte_lpm/rte_lpm.c > > @@ -1,5 +1,6 @@ > > /* SPDX-License-Identifier: BSD-3-Clause > > * Copyright(c) 2010-2014 Intel Corporation > > + * Copyright(c) 2020 Arm Limited > > */ > > > > #include > > @@ -245,13 +246,84 @@ rte_lpm_free(struct rte_lpm *lpm) > > TAILQ_REMOVE(lpm_list, te, next); > > > > rte_mcfg_tailq_write_unlock(); > > - > > +#ifdef ALLOW_EXPERIMENTAL_API > > + if (lpm->dq) > > + rte_rcu_qsbr_dq_delete(lpm->dq); > > +#endif > > All DPDK code under lib/ is compiled with the ALLOW_EXPERIMENTAL_API flag set. > There is no need to protect against this flag in rte_lpm.c. > > [...] > > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h > > index b9d49ac87..7889f21b3 100644 > > --- a/lib/librte_lpm/rte_lpm.h > > +++ b/lib/librte_lpm/rte_lpm.h > > > @@ -130,6 +143,28 @@ struct rte_lpm { > > __rte_cache_aligned; /**< LPM tbl24 table. */ > > struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */ > > struct rte_lpm_rule *rules_tbl; /**< LPM rules. */ > > +#ifdef ALLOW_EXPERIMENTAL_API > > + /* RCU config. */ > > + struct rte_rcu_qsbr *v; /* RCU QSBR variable. */ > > + enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */ > > + struct rte_rcu_qsbr_dq *dq; /* RCU QSBR defer queue. */ > > +#endif > > +}; > > This is more a comment/question for the lpm maintainers. > > Afaics, the rte_lpm structure is exported/public because of lookup > which is inlined. > But most of the structure can be hidden and stored in a private > structure that would embed the exposed rte_lpm. > The slowpath functions would only have to translate from publicly > exposed to internal representation (via container_of). > > This patch could do this and be the first step to hide the unneeded > exposure of other fields (later/in 20.11 ?). > > Thoughts? > Hiding as much of the structures as possible is always a good idea, so if that is possible in this patchset I would support such a move. /Bruce