On Mon, Jun 29, 2020 at 01:56:07PM +0200, David Marchand wrote:
> 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.
>
> [...]
>
> > 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.
>
> [...]
>
> > 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
