Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data

From: Ruifeng Wang <Ruifeng.Wang@arm.com>
To: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>,
	Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	nd <nd@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
Date: Wed, 16 Sep 2020 03:17:15 +0000	[thread overview]
Message-ID: <VI1PR0802MB23515EC923BDE2CC77B289ED9E210@VI1PR0802MB2351.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <81eb8fde-cd4f-1df9-0ebb-05c902b30fe3@intel.com>


> -----Original Message-----
> From: Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Sent: Wednesday, September 16, 2020 12:28 AM
> To: Bruce Richardson <bruce.richardson@intel.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH 2/2] lpm: hide internal data
> 
> Hi Ruifeng,
> 
> On 15/09/2020 17:02, Bruce Richardson wrote:
> > On Mon, Sep 07, 2020 at 04:15:17PM +0800, Ruifeng Wang wrote:
> >> Fields except tbl24 and tbl8 in rte_lpm structure have no need to be
> >> exposed to the user.
> >> Hide the unneeded exposure of structure fields for better ABI
> >> maintainability.
> >>
> >> Suggested-by: David Marchand <david.marchand@redhat.com>
> >> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >> ---
> >>   lib/librte_lpm/rte_lpm.c | 152 +++++++++++++++++++++++---------------
> -
> >>   lib/librte_lpm/rte_lpm.h |   7 --
> >>   2 files changed, 91 insertions(+), 68 deletions(-)
> >>
> > <snip>
> >> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> >> index 03da2d37e..112d96f37 100644
> >> --- a/lib/librte_lpm/rte_lpm.h
> >> +++ b/lib/librte_lpm/rte_lpm.h
> >> @@ -132,17 +132,10 @@ struct rte_lpm_rule_info {
> >>
> >>   /** @internal LPM structure. */
> >>   struct rte_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. */
> >> -	struct rte_lpm_rule_info rule_info[RTE_LPM_MAX_DEPTH]; /**<
> Rule info table. */
> >> -
> >>   	/* LPM Tables. */
> >>   	struct rte_lpm_tbl_entry tbl24[RTE_LPM_TBL24_NUM_ENTRIES]
> >>   			__rte_cache_aligned; /**< LPM tbl24 table. */
> >>   	struct rte_lpm_tbl_entry *tbl8; /**< LPM tbl8 table. */
> >> -	struct rte_lpm_rule *rules_tbl; /**< LPM rules. */
> >>   };
> >>
> >
> > Since this changes the ABI, does it not need advance notice?
> >
> > [Basically the return value point from rte_lpm_create() will be
> > different, and that return value could be used by rte_lpm_lookup()
> > which as a static inline function will be in the binary and using the
> > old structure offsets.]
> >
> 
> Agree with Bruce, this patch breaks ABI, so it can't be accepted without prior
> notice.
> 
So if the change wants to happen in 20.11, a deprecation notice should have been
added in 20.08.
I should have added a deprecation notice. This change will have to wait for next ABI update window.

Thanks.
Ruifeng
> >>   /** LPM RCU QSBR configuration structure. */
> >> --
> >> 2.17.1
> >>
> 
> --
> Regards,
> Vladimir