Hello,

On 23/10/2020 07:13, Ruifeng Wang wrote:

-----Original Message-----
From: David Marchand <david.march...@redhat.com>
Sent: Thursday, October 22, 2020 11:14 PM
To: Ruifeng Wang <ruifeng.w...@arm.com>
Cc: Bruce Richardson <bruce.richard...@intel.com>; Vladimir Medvedkin
<vladimir.medved...@intel.com>; dev <dev@dpdk.org>; Honnappa
Nagarahalli <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>; Kevin
Traynor <ktray...@redhat.com>; tho...@monjalon.net
Subject: Re: [PATCH v2 2/2] lpm: hide internal data

On Wed, Oct 21, 2020 at 5:02 AM Ruifeng Wang <ruifeng.w...@arm.com>
wrote:
diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c index
51a0ae578..88d31df6d 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -42,9 +42,17 @@ enum valid_flag {

  /** @internal LPM structure. */
  struct __rte_lpm {
-       /* LPM metadata. */
+       /* Exposed LPM data. */
         struct rte_lpm 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. */
+       /**< Rule info table. */
+       struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH];
+       struct rte_lpm_rule *rules_tbl; /**< LPM rules. */

- We hide the rules, is there a reason to keep struct rte_lpm_rule_info and
struct rte_lpm_rule exposed?

These structs can be moved into rte_lpm.c and made internal too.

Agree, these structs are control plane related and could be moved from public struct.


- Rather than have translations lpm -> i_lpm, in many places of this library, we
should translate only in the functions exposed to the user.
Besides, it is a bit hard to read between internal_lpm and i_lpm, I would
adopt a single i_lpm convention for the whole file.
I went and tried to do it (big search and replace + build tests, no runtime
check though).
This results in:
https://github.com/david-
marchand/dpdk/commit/4e61f0ce7cf2ac472565d3c6aa5bb78ffb8f70c9

What do you think?

I'm fine with the change. It looks good.
I checked unit test is passing.


Also checked with testfib app with "-c -a" args, FIB and LPM lookup returns same values.

Thanks.
/Ruifeng

--
David Marchand


--
Regards,
Vladimir

Reply via email to