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?
--
David Marchand
