Hi David,
Sorry, I missed tracking of this thread.
> -----Original Message-----
> From: David Marchand <david.march...@redhat.com>
> Sent: Monday, June 29, 2020 7:56 PM
> To: Ruifeng Wang <ruifeng.w...@arm.com>; Vladimir Medvedkin
> <vladimir.medved...@intel.com>; Bruce Richardson
> <bruce.richard...@intel.com>
> Cc: John McNamara <john.mcnam...@intel.com>; Marko Kovacevic
> <marko.kovace...@intel.com>; Ray Kinsella <m...@ashroe.eu>; Neil Horman
> <nhor...@tuxdriver.com>; dev <dev@dpdk.org>; Ananyev, Konstantin
> <konstantin.anan...@intel.com>; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v5 1/3] lib/lpm: integrate RCU QSBR
>
> On Mon, Jun 29, 2020 at 10:03 AM Ruifeng Wang <ruifeng.w...@arm.com>
> 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 <ruifeng.w...@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > ---
> > 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
> <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.
>
Didn't look into lpm6. It may need separate integration with RCU since no
shared code between lpm and lpm6 as you mentioned.
> [...]
>
> > 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 <string.h>
> > @@ -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.
>
OK, I see. So DPDK libraries will always be compiled with the
ALLOW_EXPERIMENTAL_API. It is application's
choice to use experimental APIs.
Will update in next version to remove the ALLOW_EXPERIMENTAL_API flag from
rte_lpm.c and only keep the one in rte_lpm.h.
> [...]
>
> > 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 ?).
>
To hide most of the structure is reasonable.
Since it will break ABI, I can do that in 20.11.
> Thoughts?
>
>
> --
> David Marchand
