[2/2] lpm: hide internal data
Checks
Commit Message
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(-)
Comments
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.]
> /** LPM RCU QSBR configuration structure. */
> --
> 2.17.1
>
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.
>> /** LPM RCU QSBR configuration structure. */
>> --
>> 2.17.1
>>
> -----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
On 16/09/2020 04:17, Ruifeng Wang wrote:
>
>> -----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.
>
Do you plan to extend? or is this just speculative?
A quick scan and there seems to be several projects using some of these
members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
gatekeeper. I didn't look at the details to see if they are really needed.
Not sure how much notice they'd need or if they update DPDK much, but I
think it's worth having a closer look as to how they use lpm and what
the impact to them is.
> Thanks.
> Ruifeng
>>>> /** LPM RCU QSBR configuration structure. */
>>>> --
>>>> 2.17.1
>>>>
>>
>> --
>> Regards,
>> Vladimir
> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Wednesday, September 30, 2020 4:46 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com>; Bruce Richardson
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>
> On 16/09/2020 04:17, Ruifeng Wang wrote:
> >
> >> -----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.
> >
>
> Do you plan to extend? or is this just speculative?
It is speculative.
>
> A quick scan and there seems to be several projects using some of these
> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
> gatekeeper. I didn't look at the details to see if they are really needed.
>
> Not sure how much notice they'd need or if they update DPDK much, but I
> think it's worth having a closer look as to how they use lpm and what the
> impact to them is.
Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
They will not be impacted by this patch.
But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.
>
> > Thanks.
> > Ruifeng
> >>>> /** LPM RCU QSBR configuration structure. */
> >>>> --
> >>>> 2.17.1
> >>>>
> >>
> >> --
> >> Regards,
> >> Vladimir
Hi Gatekeeper maintainers (I think),
fyi - there is a proposal to remove some members of a struct in DPDK LPM
API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
as it's an LTS I guess it would probably hit Debian in a few months.
The full thread is here:
http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
Maybe you can take a look and tell us if they are needed in Gatekeeper
or you can workaround it?
thanks,
Kevin.
[1]
https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
On 09/10/2020 07:54, Ruifeng Wang wrote:
>
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Wednesday, September 30, 2020 4:46 PM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>> <bruce.richardson@intel.com>
>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>
>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>
>>>> -----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.
>>>
>>
>> Do you plan to extend? or is this just speculative?
> It is speculative.
>
>>
>> A quick scan and there seems to be several projects using some of these
>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>> gatekeeper. I didn't look at the details to see if they are really needed.
>>
>> Not sure how much notice they'd need or if they update DPDK much, but I
>> think it's worth having a closer look as to how they use lpm and what the
>> impact to them is.
> Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
> They will not be impacted by this patch.
> But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.
>
>>
>>> Thanks.
>>> Ruifeng
>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>> --
>>>>>> 2.17.1
>>>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Vladimir
>
Hi Kevin,
We do need fields max_rules and number_tbl8s of struct rte_lpm, so
the removal would force us to have another patch to our local copy of
DPDK. We'd rather avoid this new local patch because we wish to
eventually be in sync with the stock DPDK.
Those fields are needed in Gatekeeper because we found a condition
in an ongoing deployment in which the entries of some LPM tables may
suddenly change a lot to reflect policy changes. To avoid getting into a
state in which the LPM table is inconsistent because it cannot fit all
the new entries, we compute the needed parameters to support the new
entries, and compare with the current parameters. If the current table
doesn't fit everything, we have to replace it with a new LPM table.
If there were a way to obtain the struct rte_lpm_config of a given
LPM table, it would cleanly address our need. We have the same need in
IPv6 and have a local patch to work around it (see
https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
Thus, an IPv4 and IPv6 solution would be best.
PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this
disscussion.
[ ]'s
Michel Machado
On 10/13/20 9:53 AM, Kevin Traynor wrote:
> Hi Gatekeeper maintainers (I think),
>
> fyi - there is a proposal to remove some members of a struct in DPDK LPM
> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
> as it's an LTS I guess it would probably hit Debian in a few months.
>
> The full thread is here:
> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>
> Maybe you can take a look and tell us if they are needed in Gatekeeper
> or you can workaround it?
>
> thanks,
> Kevin.
>
> [1]
> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>
> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>
>>> -----Original Message-----
>>> From: Kevin Traynor <ktraynor@redhat.com>
>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>> <bruce.richardson@intel.com>
>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>
>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>
>>>>> -----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.
>>>>
>>>
>>> Do you plan to extend? or is this just speculative?
>> It is speculative.
>>
>>>
>>> A quick scan and there seems to be several projects using some of these
>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>> gatekeeper. I didn't look at the details to see if they are really needed.
>>>
>>> Not sure how much notice they'd need or if they update DPDK much, but I
>>> think it's worth having a closer look as to how they use lpm and what the
>>> impact to them is.
>> Checked the projects listed above. BESS, NFF-Go and DPVS don't access the members to be hided.
>> They will not be impacted by this patch.
>> But Gatekeeper accesses the rte_lpm internal members that to be hided. Its compilation will be broken with this patch.
>>
>>>
>>>> Thanks.
>>>> Ruifeng
>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>> --
>>>>>>> 2.17.1
>>>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Vladimir
>>
>
Hi Michel,
Could you please describe a condition when LPM gets inconsistent? As I
can see if there is no free tbl8 it will return -ENOSPC.
On 13/10/2020 15:58, Michel Machado wrote:
> Hi Kevin,
>
> We do need fields max_rules and number_tbl8s of struct rte_lpm, so
> the removal would force us to have another patch to our local copy of
> DPDK. We'd rather avoid this new local patch because we wish to
> eventually be in sync with the stock DPDK.
>
> Those fields are needed in Gatekeeper because we found a condition
> in an ongoing deployment in which the entries of some LPM tables may
> suddenly change a lot to reflect policy changes. To avoid getting into a
> state in which the LPM table is inconsistent because it cannot fit all
> the new entries, we compute the needed parameters to support the new
> entries, and compare with the current parameters. If the current table
> doesn't fit everything, we have to replace it with a new LPM table.
>
> If there were a way to obtain the struct rte_lpm_config of a given
> LPM table, it would cleanly address our need. We have the same need in
> IPv6 and have a local patch to work around it (see
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
> Thus, an IPv4 and IPv6 solution would be best.
>
> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this
> disscussion.
>
> [ ]'s
> Michel Machado
>
> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>> Hi Gatekeeper maintainers (I think),
>>
>> fyi - there is a proposal to remove some members of a struct in DPDK LPM
>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>> as it's an LTS I guess it would probably hit Debian in a few months.
>>
>> The full thread is here:
>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>
>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>> or you can workaround it?
>>
>> thanks,
>> Kevin.
>>
>> [1]
>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>>
>>
>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>
>>>> -----Original Message-----
>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>> <bruce.richardson@intel.com>
>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>
>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>
>>>>>> -----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.
>>>>>
>>>>
>>>> Do you plan to extend? or is this just speculative?
>>> It is speculative.
>>>
>>>>
>>>> A quick scan and there seems to be several projects using some of these
>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>> gatekeeper. I didn't look at the details to see if they are really
>>>> needed.
>>>>
>>>> Not sure how much notice they'd need or if they update DPDK much, but I
>>>> think it's worth having a closer look as to how they use lpm and
>>>> what the
>>>> impact to them is.
>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't access
>>> the members to be hided.
>>> They will not be impacted by this patch.
>>> But Gatekeeper accesses the rte_lpm internal members that to be
>>> hided. Its compilation will be broken with this patch.
>>>
>>>>
>>>>> Thanks.
>>>>> Ruifeng
>>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>>> --
>>>>>>>> 2.17.1
>>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Regards,
>>>>>> Vladimir
>>>
>>
On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> Hi Michel,
>
> Could you please describe a condition when LPM gets inconsistent? As I
> can see if there is no free tbl8 it will return -ENOSPC.
Consider this simple example, we need to add the following two
prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If the
LPM table is out of tbl8s, the second prefix is not added and Gatekeeper
will make decisions in violation of the policy. The data structure of
the LPM table is consistent, but its content inconsistent with the policy.
We minimize the need of replacing a LPM table by allocating LPM
tables with the double of what we need (see example here
https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
but the code must be ready for unexpected needs that may arise in
production.
>
> On 13/10/2020 15:58, Michel Machado wrote:
>> Hi Kevin,
>>
>> We do need fields max_rules and number_tbl8s of struct rte_lpm, so
>> the removal would force us to have another patch to our local copy of
>> DPDK. We'd rather avoid this new local patch because we wish to
>> eventually be in sync with the stock DPDK.
>>
>> Those fields are needed in Gatekeeper because we found a condition
>> in an ongoing deployment in which the entries of some LPM tables may
>> suddenly change a lot to reflect policy changes. To avoid getting into
>> a state in which the LPM table is inconsistent because it cannot fit
>> all the new entries, we compute the needed parameters to support the
>> new entries, and compare with the current parameters. If the current
>> table doesn't fit everything, we have to replace it with a new LPM table.
>>
>> If there were a way to obtain the struct rte_lpm_config of a given
>> LPM table, it would cleanly address our need. We have the same need in
>> IPv6 and have a local patch to work around it (see
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
>> Thus, an IPv4 and IPv6 solution would be best.
>>
>> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this
>> disscussion.
>>
>> [ ]'s
>> Michel Machado
>>
>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>> Hi Gatekeeper maintainers (I think),
>>>
>>> fyi - there is a proposal to remove some members of a struct in DPDK LPM
>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>
>>> The full thread is here:
>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>
>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>> or you can workaround it?
>>>
>>> thanks,
>>> Kevin.
>>>
>>> [1]
>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>>>
>>>
>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>> <bruce.richardson@intel.com>
>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>
>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>
>>>>>>> -----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.
>>>>>>
>>>>>
>>>>> Do you plan to extend? or is this just speculative?
>>>> It is speculative.
>>>>
>>>>>
>>>>> A quick scan and there seems to be several projects using some of
>>>>> these
>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>> gatekeeper. I didn't look at the details to see if they are really
>>>>> needed.
>>>>>
>>>>> Not sure how much notice they'd need or if they update DPDK much,
>>>>> but I
>>>>> think it's worth having a closer look as to how they use lpm and
>>>>> what the
>>>>> impact to them is.
>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
>>>> access the members to be hided.
>>>> They will not be impacted by this patch.
>>>> But Gatekeeper accesses the rte_lpm internal members that to be
>>>> hided. Its compilation will be broken with this patch.
>>>>
>>>>>
>>>>>> Thanks.
>>>>>> Ruifeng
>>>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>>>> --
>>>>>>>>> 2.17.1
>>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Regards,
>>>>>>> Vladimir
>>>>
>>>
>
On 13/10/2020 18:46, Michel Machado wrote:
> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>> Hi Michel,
>>
>> Could you please describe a condition when LPM gets inconsistent? As I
>> can see if there is no free tbl8 it will return -ENOSPC.
>
> Consider this simple example, we need to add the following two
> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If the
> LPM table is out of tbl8s, the second prefix is not added and Gatekeeper
> will make decisions in violation of the policy. The data structure of
> the LPM table is consistent, but its content inconsistent with the policy.
Aha, thanks. So do I understand correctly that you need to add a set of
routes atomically (either the entire set is installed or nothing)?
If so, then I would suggest having 2 lpm and switching them atomically
after a successful addition. As for now, even if you have enough tbl8's,
routes are installed non atomically, i.e. there will be a time gap
between adding two routes, so in this time interval the table will be
inconsistent with the policy.
Also, if new lpm algorithms are added to the DPDK, they won't have such
a thing as tbl8.
>
> We minimize the need of replacing a LPM table by allocating LPM
> tables with the double of what we need (see example here
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
> but the code must be ready for unexpected needs that may arise in
> production.
>
Usually, the table is initialized with a large enough number of entries,
enough to add a possible number of routes. One tbl8 group takes up 1Kb
of memory which is nothing comparing to the size of tbl24 which is 64Mb.
P.S. consider using rte_fib library, it has a number of advantages over
LPM. You can replace the loop in __lookup_fib_bulk() with a bulk lookup
call and this will probably increase the speed.
>>
>> On 13/10/2020 15:58, Michel Machado wrote:
>>> Hi Kevin,
>>>
>>> We do need fields max_rules and number_tbl8s of struct rte_lpm,
>>> so the removal would force us to have another patch to our local copy
>>> of DPDK. We'd rather avoid this new local patch because we wish to
>>> eventually be in sync with the stock DPDK.
>>>
>>> Those fields are needed in Gatekeeper because we found a
>>> condition in an ongoing deployment in which the entries of some LPM
>>> tables may suddenly change a lot to reflect policy changes. To avoid
>>> getting into a state in which the LPM table is inconsistent because
>>> it cannot fit all the new entries, we compute the needed parameters
>>> to support the new entries, and compare with the current parameters.
>>> If the current table doesn't fit everything, we have to replace it
>>> with a new LPM table.
>>>
>>> If there were a way to obtain the struct rte_lpm_config of a
>>> given LPM table, it would cleanly address our need. We have the same
>>> need in IPv6 and have a local patch to work around it (see
>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
>>> Thus, an IPv4 and IPv6 solution would be best.
>>>
>>> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to this
>>> disscussion.
>>>
>>> [ ]'s
>>> Michel Machado
>>>
>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>> Hi Gatekeeper maintainers (I think),
>>>>
>>>> fyi - there is a proposal to remove some members of a struct in DPDK
>>>> LPM
>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>
>>>> The full thread is here:
>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>>
>>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>>> or you can workaround it?
>>>>
>>>> thanks,
>>>> Kevin.
>>>>
>>>> [1]
>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>>>>
>>>>
>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>> <bruce.richardson@intel.com>
>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>
>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>
>>>>>>>> -----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.
>>>>>>>
>>>>>>
>>>>>> Do you plan to extend? or is this just speculative?
>>>>> It is speculative.
>>>>>
>>>>>>
>>>>>> A quick scan and there seems to be several projects using some of
>>>>>> these
>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>> gatekeeper. I didn't look at the details to see if they are really
>>>>>> needed.
>>>>>>
>>>>>> Not sure how much notice they'd need or if they update DPDK much,
>>>>>> but I
>>>>>> think it's worth having a closer look as to how they use lpm and
>>>>>> what the
>>>>>> impact to them is.
>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
>>>>> access the members to be hided.
>>>>> They will not be impacted by this patch.
>>>>> But Gatekeeper accesses the rte_lpm internal members that to be
>>>>> hided. Its compilation will be broken with this patch.
>>>>>
>>>>>>
>>>>>>> Thanks.
>>>>>>> Ruifeng
>>>>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>>>>> --
>>>>>>>>>> 2.17.1
>>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Regards,
>>>>>>>> Vladimir
>>>>>
>>>>
>>
On 10/13/20 3:06 PM, Medvedkin, Vladimir wrote:
>
>
> On 13/10/2020 18:46, Michel Machado wrote:
>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>> Hi Michel,
>>>
>>> Could you please describe a condition when LPM gets inconsistent? As
>>> I can see if there is no free tbl8 it will return -ENOSPC.
>>
>> Consider this simple example, we need to add the following two
>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
>> the LPM table is out of tbl8s, the second prefix is not added and
>> Gatekeeper will make decisions in violation of the policy. The data
>> structure of the LPM table is consistent, but its content inconsistent
>> with the policy.
>
> Aha, thanks. So do I understand correctly that you need to add a set of
> routes atomically (either the entire set is installed or nothing)?
Yes.
> If so, then I would suggest having 2 lpm and switching them atomically
> after a successful addition. As for now, even if you have enough tbl8's,
> routes are installed non atomically, i.e. there will be a time gap
> between adding two routes, so in this time interval the table will be
> inconsistent with the policy.
> Also, if new lpm algorithms are added to the DPDK, they won't have such
> a thing as tbl8.
Our code already deals with synchronization.
>> We minimize the need of replacing a LPM table by allocating LPM
>> tables with the double of what we need (see example here
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
>> but the code must be ready for unexpected needs that may arise in
>> production.
>>
>
> Usually, the table is initialized with a large enough number of entries,
> enough to add a possible number of routes. One tbl8 group takes up 1Kb
> of memory which is nothing comparing to the size of tbl24 which is 64Mb.
When the prefixes come from BGP, initializing a large enough table
is fine. But when prefixes come from threat intelligence, the number of
prefixes can vary wildly and the number of prefixes above 24 bits are
way more common.
> P.S. consider using rte_fib library, it has a number of advantages over
> LPM. You can replace the loop in __lookup_fib_bulk() with a bulk lookup
> call and this will probably increase the speed.
I'm not aware of the rte_fib library. The only documentation that I
found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it
just says "FIB (Forwarding information base) implementation for IPv4
Longest Prefix Match".
>>>
>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>> Hi Kevin,
>>>>
>>>> We do need fields max_rules and number_tbl8s of struct rte_lpm,
>>>> so the removal would force us to have another patch to our local
>>>> copy of DPDK. We'd rather avoid this new local patch because we wish
>>>> to eventually be in sync with the stock DPDK.
>>>>
>>>> Those fields are needed in Gatekeeper because we found a
>>>> condition in an ongoing deployment in which the entries of some LPM
>>>> tables may suddenly change a lot to reflect policy changes. To avoid
>>>> getting into a state in which the LPM table is inconsistent because
>>>> it cannot fit all the new entries, we compute the needed parameters
>>>> to support the new entries, and compare with the current parameters.
>>>> If the current table doesn't fit everything, we have to replace it
>>>> with a new LPM table.
>>>>
>>>> If there were a way to obtain the struct rte_lpm_config of a
>>>> given LPM table, it would cleanly address our need. We have the same
>>>> need in IPv6 and have a local patch to work around it (see
>>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
>>>> Thus, an IPv4 and IPv6 solution would be best.
>>>>
>>>> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to
>>>> this disscussion.
>>>>
>>>> [ ]'s
>>>> Michel Machado
>>>>
>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>>> Hi Gatekeeper maintainers (I think),
>>>>>
>>>>> fyi - there is a proposal to remove some members of a struct in
>>>>> DPDK LPM
>>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11 but
>>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>>
>>>>> The full thread is here:
>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>>>
>>>>> Maybe you can take a look and tell us if they are needed in Gatekeeper
>>>>> or you can workaround it?
>>>>>
>>>>> thanks,
>>>>> Kevin.
>>>>>
>>>>> [1]
>>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>>>>>
>>>>>
>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>>> <bruce.richardson@intel.com>
>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>>
>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>>
>>>>>>>>> -----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.
>>>>>>>>
>>>>>>>
>>>>>>> Do you plan to extend? or is this just speculative?
>>>>>> It is speculative.
>>>>>>
>>>>>>>
>>>>>>> A quick scan and there seems to be several projects using some of
>>>>>>> these
>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>>> gatekeeper. I didn't look at the details to see if they are
>>>>>>> really needed.
>>>>>>>
>>>>>>> Not sure how much notice they'd need or if they update DPDK much,
>>>>>>> but I
>>>>>>> think it's worth having a closer look as to how they use lpm and
>>>>>>> what the
>>>>>>> impact to them is.
>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
>>>>>> access the members to be hided.
>>>>>> They will not be impacted by this patch.
>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be
>>>>>> hided. Its compilation will be broken with this patch.
>>>>>>
>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Ruifeng
>>>>>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>>>>>> --
>>>>>>>>>>> 2.17.1
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Regards,
>>>>>>>>> Vladimir
>>>>>>
>>>>>
>>>
>
On 13/10/2020 20:48, Michel Machado wrote:
> On 10/13/20 3:06 PM, Medvedkin, Vladimir wrote:
>>
>>
>> On 13/10/2020 18:46, Michel Machado wrote:
>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>> Hi Michel,
>>>>
>>>> Could you please describe a condition when LPM gets inconsistent? As
>>>> I can see if there is no free tbl8 it will return -ENOSPC.
>>>
>>> Consider this simple example, we need to add the following two
>>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
>>> the LPM table is out of tbl8s, the second prefix is not added and
>>> Gatekeeper will make decisions in violation of the policy. The data
>>> structure of the LPM table is consistent, but its content
>>> inconsistent with the policy.
>>
>> Aha, thanks. So do I understand correctly that you need to add a set
>> of routes atomically (either the entire set is installed or nothing)?
>
> Yes.
>
>> If so, then I would suggest having 2 lpm and switching them atomically
>> after a successful addition. As for now, even if you have enough
>> tbl8's, routes are installed non atomically, i.e. there will be a time
>> gap between adding two routes, so in this time interval the table will
>> be inconsistent with the policy.
>> Also, if new lpm algorithms are added to the DPDK, they won't have
>> such a thing as tbl8.
>
> Our code already deals with synchronization.
OK, so my suggestion here would be to add new routes to the shadow copy
of the lpm, and if it returns -ENOSPC, than create a new LPM with double
amount of tbl8's and add all the routes to it. Then switch the
active-shadow LPM pointers. In this case you'll always add a bulk of
routes atomically.
>
>>> We minimize the need of replacing a LPM table by allocating LPM
>>> tables with the double of what we need (see example here
>>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
>>> but the code must be ready for unexpected needs that may arise in
>>> production.
>>>
>>
>> Usually, the table is initialized with a large enough number of
>> entries, enough to add a possible number of routes. One tbl8 group
>> takes up 1Kb of memory which is nothing comparing to the size of tbl24
>> which is 64Mb.
>
> When the prefixes come from BGP, initializing a large enough table
> is fine. But when prefixes come from threat intelligence, the number of
> prefixes can vary wildly and the number of prefixes above 24 bits are
> way more common.
>
>> P.S. consider using rte_fib library, it has a number of advantages
>> over LPM. You can replace the loop in __lookup_fib_bulk() with a bulk
>> lookup call and this will probably increase the speed.
>
> I'm not aware of the rte_fib library. The only documentation that I
> found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it
> just says "FIB (Forwarding information base) implementation for IPv4
> Longest Prefix Match".
That's true, I'm going to add programmer's guide soon.
Although the fib API is very similar to existing LPM.
>
>>>>
>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>> Hi Kevin,
>>>>>
>>>>> We do need fields max_rules and number_tbl8s of struct rte_lpm,
>>>>> so the removal would force us to have another patch to our local
>>>>> copy of DPDK. We'd rather avoid this new local patch because we
>>>>> wish to eventually be in sync with the stock DPDK.
>>>>>
>>>>> Those fields are needed in Gatekeeper because we found a
>>>>> condition in an ongoing deployment in which the entries of some LPM
>>>>> tables may suddenly change a lot to reflect policy changes. To
>>>>> avoid getting into a state in which the LPM table is inconsistent
>>>>> because it cannot fit all the new entries, we compute the needed
>>>>> parameters to support the new entries, and compare with the current
>>>>> parameters. If the current table doesn't fit everything, we have to
>>>>> replace it with a new LPM table.
>>>>>
>>>>> If there were a way to obtain the struct rte_lpm_config of a
>>>>> given LPM table, it would cleanly address our need. We have the
>>>>> same need in IPv6 and have a local patch to work around it (see
>>>>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db26a78115cb8c8f).
>>>>> Thus, an IPv4 and IPv6 solution would be best.
>>>>>
>>>>> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to
>>>>> this disscussion.
>>>>>
>>>>> [ ]'s
>>>>> Michel Machado
>>>>>
>>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
>>>>>> Hi Gatekeeper maintainers (I think),
>>>>>>
>>>>>> fyi - there is a proposal to remove some members of a struct in
>>>>>> DPDK LPM
>>>>>> API that Gatekeeper is using [1]. It would be only from DPDK 20.11
>>>>>> but
>>>>>> as it's an LTS I guess it would probably hit Debian in a few months.
>>>>>>
>>>>>> The full thread is here:
>>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-ruifeng.wang@arm.com/
>>>>>>
>>>>>>
>>>>>> Maybe you can take a look and tell us if they are needed in
>>>>>> Gatekeeper
>>>>>> or you can workaround it?
>>>>>>
>>>>>> thanks,
>>>>>> Kevin.
>>>>>>
>>>>>> [1]
>>>>>> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c#L235-L248
>>>>>>
>>>>>>
>>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
>>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin, Vladimir
>>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
>>>>>>>> <bruce.richardson@intel.com>
>>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
>>>>>>>>
>>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
>>>>>>>>>
>>>>>>>>>> -----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.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Do you plan to extend? or is this just speculative?
>>>>>>> It is speculative.
>>>>>>>
>>>>>>>>
>>>>>>>> A quick scan and there seems to be several projects using some
>>>>>>>> of these
>>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
>>>>>>>> gatekeeper. I didn't look at the details to see if they are
>>>>>>>> really needed.
>>>>>>>>
>>>>>>>> Not sure how much notice they'd need or if they update DPDK
>>>>>>>> much, but I
>>>>>>>> think it's worth having a closer look as to how they use lpm and
>>>>>>>> what the
>>>>>>>> impact to them is.
>>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
>>>>>>> access the members to be hided.
>>>>>>> They will not be impacted by this patch.
>>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be
>>>>>>> hided. Its compilation will be broken with this patch.
>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>> Ruifeng
>>>>>>>>>>>> /** LPM RCU QSBR configuration structure. */
>>>>>>>>>>>> --
>>>>>>>>>>>> 2.17.1
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Regards,
>>>>>>>>>> Vladimir
>>>>>>>
>>>>>>
>>>>
>>
<snip>
> >>
> >>
> >> On 13/10/2020 18:46, Michel Machado wrote:
> >>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>> Hi Michel,
> >>>>
> >>>> Could you please describe a condition when LPM gets inconsistent?
> >>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>
> >>> Consider this simple example, we need to add the following two
> >>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
> >>> the LPM table is out of tbl8s, the second prefix is not added and
> >>> Gatekeeper will make decisions in violation of the policy. The data
> >>> structure of the LPM table is consistent, but its content
> >>> inconsistent with the policy.
max_rules and number_tbl8s in 'struct rte_lpm' contain the config information. These 2 fields do not change based on the routes added and do not indicate the amount of space left. So, you cannot use this information to decide if there is enough space to add more routes.
> >>
> >> Aha, thanks. So do I understand correctly that you need to add a set
> >> of routes atomically (either the entire set is installed or nothing)?
> >
> > Yes.
> >
> >> If so, then I would suggest having 2 lpm and switching them
> >> atomically after a successful addition. As for now, even if you have
> >> enough tbl8's, routes are installed non atomically, i.e. there will
> >> be a time gap between adding two routes, so in this time interval the
> >> table will be inconsistent with the policy.
> >> Also, if new lpm algorithms are added to the DPDK, they won't have
> >> such a thing as tbl8.
> >
> > Our code already deals with synchronization.
If the application code already deals with synchronization, is it possible to revert back (i.e. delete the routes that got added so far) when the addition of the route-set fails?
>
> OK, so my suggestion here would be to add new routes to the shadow copy
> of the lpm, and if it returns -ENOSPC, than create a new LPM with double
> amount of tbl8's and add all the routes to it. Then switch the active-shadow
> LPM pointers. In this case you'll always add a bulk of routes atomically.
>
> >
> >>> We minimize the need of replacing a LPM table by allocating LPM
> >>> tables with the double of what we need (see example here
> >>>
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> >>> bfd06ad606674f1e07/lua/examples/policy.lua#L172-L183),
> >>> but the code must be ready for unexpected needs that may arise in
> >>> production.
> >>>
> >>
> >> Usually, the table is initialized with a large enough number of
> >> entries, enough to add a possible number of routes. One tbl8 group
> >> takes up 1Kb of memory which is nothing comparing to the size of
> >> tbl24 which is 64Mb.
> >
> > When the prefixes come from BGP, initializing a large enough table
> > is fine. But when prefixes come from threat intelligence, the number
> > of prefixes can vary wildly and the number of prefixes above 24 bits
> > are way more common.
> >
> >> P.S. consider using rte_fib library, it has a number of advantages
> >> over LPM. You can replace the loop in __lookup_fib_bulk() with a bulk
> >> lookup call and this will probably increase the speed.
> >
> > I'm not aware of the rte_fib library. The only documentation that
> > I found on Google was https://doc.dpdk.org/api/rte__fib_8h.html and it
> > just says "FIB (Forwarding information base) implementation for IPv4
> > Longest Prefix Match".
>
> That's true, I'm going to add programmer's guide soon.
> Although the fib API is very similar to existing LPM.
>
> >
> >>>>
> >>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>> Hi Kevin,
> >>>>>
> >>>>> We do need fields max_rules and number_tbl8s of struct
> >>>>> rte_lpm, so the removal would force us to have another patch to
> >>>>> our local copy of DPDK. We'd rather avoid this new local patch
> >>>>> because we wish to eventually be in sync with the stock DPDK.
> >>>>>
> >>>>> Those fields are needed in Gatekeeper because we found a
> >>>>> condition in an ongoing deployment in which the entries of some
> >>>>> LPM tables may suddenly change a lot to reflect policy changes. To
> >>>>> avoid getting into a state in which the LPM table is inconsistent
> >>>>> because it cannot fit all the new entries, we compute the needed
> >>>>> parameters to support the new entries, and compare with the
> >>>>> current parameters. If the current table doesn't fit everything,
> >>>>> we have to replace it with a new LPM table.
> >>>>>
> >>>>> If there were a way to obtain the struct rte_lpm_config of a
> >>>>> given LPM table, it would cleanly address our need. We have the
> >>>>> same need in IPv6 and have a local patch to work around it (see
> >>>>>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> 26a78115cb8c8f).
I do not see why such an API is not possible, we could add one API that returns max_rules and number_tbl8s (essentially, the config that was passed to rte_lpm_create API).
But, is there a possibility to store that info in the application as that data was passed to rte_lpm from the application?
> >>>>> Thus, an IPv4 and IPv6 solution would be best.
> >>>>>
> >>>>> PS: I've added Qiaobin Fu, another Gatekeeper maintainer, to
> >>>>> this disscussion.
> >>>>>
> >>>>> [ ]'s
> >>>>> Michel Machado
> >>>>>
> >>>>> On 10/13/20 9:53 AM, Kevin Traynor wrote:
> >>>>>> Hi Gatekeeper maintainers (I think),
> >>>>>>
> >>>>>> fyi - there is a proposal to remove some members of a struct in
> >>>>>> DPDK LPM API that Gatekeeper is using [1]. It would be only from
> >>>>>> DPDK 20.11 but as it's an LTS I guess it would probably hit
> >>>>>> Debian in a few months.
> >>>>>>
> >>>>>> The full thread is here:
> >>>>>> http://inbox.dpdk.org/dev/20200907081518.46350-1-
> ruifeng.wang@arm
> >>>>>> .com/
> >>>>>>
> >>>>>>
> >>>>>> Maybe you can take a look and tell us if they are needed in
> >>>>>> Gatekeeper or you can workaround it?
> >>>>>>
> >>>>>> thanks,
> >>>>>> Kevin.
> >>>>>>
> >>>>>> [1]
> >>>>>>
> https://github.com/AltraMayor/gatekeeper/blob/master/gt/lua_lpm.c
> >>>>>> #L235-L248
> >>>>>>
> >>>>>>
> >>>>>> On 09/10/2020 07:54, Ruifeng Wang wrote:
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
> >>>>>>>> Sent: Wednesday, September 30, 2020 4:46 PM
> >>>>>>>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; Medvedkin,
> Vladimir
> >>>>>>>> <vladimir.medvedkin@intel.com>; Bruce Richardson
> >>>>>>>> <bruce.richardson@intel.com>
> >>>>>>>> Cc: dev@dpdk.org; Honnappa Nagarahalli
> >>>>>>>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >>>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
> >>>>>>>>
> >>>>>>>> On 16/09/2020 04:17, Ruifeng Wang wrote:
> >>>>>>>>>
> >>>>>>>>>> -----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.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Do you plan to extend? or is this just speculative?
> >>>>>>> It is speculative.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> A quick scan and there seems to be several projects using some
> >>>>>>>> of these
> >>>>>>>> members that you are proposing to hide. e.g. BESS, NFF-Go, DPVS,
> >>>>>>>> gatekeeper. I didn't look at the details to see if they are
> >>>>>>>> really needed.
> >>>>>>>>
> >>>>>>>> Not sure how much notice they'd need or if they update DPDK
> >>>>>>>> much, but I
> >>>>>>>> think it's worth having a closer look as to how they use lpm and
> >>>>>>>> what the
> >>>>>>>> impact to them is.
> >>>>>>> Checked the projects listed above. BESS, NFF-Go and DPVS don't
> >>>>>>> access the members to be hided.
> >>>>>>> They will not be impacted by this patch.
> >>>>>>> But Gatekeeper accesses the rte_lpm internal members that to be
> >>>>>>> hided. Its compilation will be broken with this patch.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>> Ruifeng
> >>>>>>>>>>>> /** LPM RCU QSBR configuration structure. */
> >>>>>>>>>>>> --
> >>>>>>>>>>>> 2.17.1
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> Regards,
> >>>>>>>>>> Vladimir
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
> --
> Regards,
> Vladimir
On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>> Hi Michel,
>>>>>>
>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>
>>>>> Consider this simple example, we need to add the following two
>>>>> prefixes with different next hops: 10.99.0.0/16, 18.99.99.128/25. If
>>>>> the LPM table is out of tbl8s, the second prefix is not added and
>>>>> Gatekeeper will make decisions in violation of the policy. The data
>>>>> structure of the LPM table is consistent, but its content
>>>>> inconsistent with the policy.
> max_rules and number_tbl8s in 'struct rte_lpm' contain the config information. These 2 fields do not change based on the routes added and do not indicate the amount of space left. So, you cannot use this information to decide if there is enough space to add more routes.
We are aware that those fields hold the config information not a
status of the LPM table.
Before updating a LPM table that holds network prefixes derived from
threat intelligence, we compute the minimum values for max_rules and
number_tbl8s. Here is an example of how we do it:
https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
Once these minimum values are available, we get the parameters of
the LPM table to be updated and check if we can update it, or have to
recreate it.
>>>> Aha, thanks. So do I understand correctly that you need to add a set
>>>> of routes atomically (either the entire set is installed or nothing)?
>>>
>>> Yes.
>>>
>>>> If so, then I would suggest having 2 lpm and switching them
>>>> atomically after a successful addition. As for now, even if you have
>>>> enough tbl8's, routes are installed non atomically, i.e. there will
>>>> be a time gap between adding two routes, so in this time interval the
>>>> table will be inconsistent with the policy.
>>>> Also, if new lpm algorithms are added to the DPDK, they won't have
>>>> such a thing as tbl8.
>>>
>>> Our code already deals with synchronization.
> If the application code already deals with synchronization, is it possible to revert back (i.e. delete the routes that got added so far) when the addition of the route-set fails?
The way the code is structured, this would require a significant
rewrite because the code assumes that it will succeed since the capacity
of the LPM tables was already checked.
>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>> Hi Kevin,
>>>>>>>
>>>>>>> We do need fields max_rules and number_tbl8s of struct
>>>>>>> rte_lpm, so the removal would force us to have another patch to
>>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
>>>>>>> because we wish to eventually be in sync with the stock DPDK.
>>>>>>>
>>>>>>> Those fields are needed in Gatekeeper because we found a
>>>>>>> condition in an ongoing deployment in which the entries of some
>>>>>>> LPM tables may suddenly change a lot to reflect policy changes. To
>>>>>>> avoid getting into a state in which the LPM table is inconsistent
>>>>>>> because it cannot fit all the new entries, we compute the needed
>>>>>>> parameters to support the new entries, and compare with the
>>>>>>> current parameters. If the current table doesn't fit everything,
>>>>>>> we have to replace it with a new LPM table.
>>>>>>>
>>>>>>> If there were a way to obtain the struct rte_lpm_config of a
>>>>>>> given LPM table, it would cleanly address our need. We have the
>>>>>>> same need in IPv6 and have a local patch to work around it (see
>>>>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>> 26a78115cb8c8f).
> I do not see why such an API is not possible, we could add one API that returns max_rules and number_tbl8s (essentially, the config that was passed to rte_lpm_create API).
> But, is there a possibility to store that info in the application as that data was passed to rte_lpm from the application?
A suggestion for what this API could look like:
void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config
*config);
void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct
rte_lpm6_config *config);
If the final choice is for not supporting a way to retrieve the
config information on the API, we'll look for a place to keep a copy of
the parameters in our code.
<snip>
>
> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
> >>>> On 13/10/2020 18:46, Michel Machado wrote:
> >>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>>>> Hi Michel,
> >>>>>>
> >>>>>> Could you please describe a condition when LPM gets inconsistent?
> >>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>>>
> >>>>> Consider this simple example, we need to add the following
> >>>>> two prefixes with different next hops: 10.99.0.0/16,
> >>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
> >>>>> prefix is not added and Gatekeeper will make decisions in
> >>>>> violation of the policy. The data structure of the LPM table is
> >>>>> consistent, but its content inconsistent with the policy.
> > max_rules and number_tbl8s in 'struct rte_lpm' contain the config
> information. These 2 fields do not change based on the routes added and do
> not indicate the amount of space left. So, you cannot use this information to
> decide if there is enough space to add more routes.
>
> We are aware that those fields hold the config information not a status of
> the LPM table.
>
> Before updating a LPM table that holds network prefixes derived from
> threat intelligence, we compute the minimum values for max_rules and
> number_tbl8s. Here is an example of how we do it:
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
>
> Once these minimum values are available, we get the parameters of the
> LPM table to be updated and check if we can update it, or have to recreate it.
>
> >>>> Aha, thanks. So do I understand correctly that you need to add a
> >>>> set of routes atomically (either the entire set is installed or nothing)?
> >>>
> >>> Yes.
> >>>
> >>>> If so, then I would suggest having 2 lpm and switching them
> >>>> atomically after a successful addition. As for now, even if you
> >>>> have enough tbl8's, routes are installed non atomically, i.e. there
> >>>> will be a time gap between adding two routes, so in this time
> >>>> interval the table will be inconsistent with the policy.
> >>>> Also, if new lpm algorithms are added to the DPDK, they won't have
> >>>> such a thing as tbl8.
> >>>
> >>> Our code already deals with synchronization.
> > If the application code already deals with synchronization, is it possible to
> revert back (i.e. delete the routes that got added so far) when the addition
> of the route-set fails?
>
> The way the code is structured, this would require a significant rewrite
> because the code assumes that it will succeed since the capacity of the LPM
> tables was already checked.
>
> >>>>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>>>> Hi Kevin,
> >>>>>>>
> >>>>>>> We do need fields max_rules and number_tbl8s of struct
> >>>>>>> rte_lpm, so the removal would force us to have another patch to
> >>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
> >>>>>>> because we wish to eventually be in sync with the stock DPDK.
> >>>>>>>
> >>>>>>> Those fields are needed in Gatekeeper because we found a
> >>>>>>> condition in an ongoing deployment in which the entries of some
> >>>>>>> LPM tables may suddenly change a lot to reflect policy changes.
> >>>>>>> To avoid getting into a state in which the LPM table is
> >>>>>>> inconsistent because it cannot fit all the new entries, we
> >>>>>>> compute the needed parameters to support the new entries, and
> >>>>>>> compare with the current parameters. If the current table
> >>>>>>> doesn't fit everything, we have to replace it with a new LPM table.
> >>>>>>>
> >>>>>>> If there were a way to obtain the struct rte_lpm_config of
> >>>>>>> a given LPM table, it would cleanly address our need. We have
> >>>>>>> the same need in IPv6 and have a local patch to work around it
> >>>>>>> (see
> >>>>>>>
> >>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> >> 26a78115cb8c8f).
> > I do not see why such an API is not possible, we could add one API that
> returns max_rules and number_tbl8s (essentially, the config that was passed
> to rte_lpm_create API).
> > But, is there a possibility to store that info in the application as that data
> was passed to rte_lpm from the application?
>
> A suggestion for what this API could look like:
>
> void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config
> *config); void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct
> rte_lpm6_config *config);
>
> If the final choice is for not supporting a way to retrieve the config
> information on the API, we'll look for a place to keep a copy of the
> parameters in our code.
IMO, this is not a performance critical path and it is not a difficult solution to store these values in the application. My suggestion is to skip adding the API and store the values in the application.
Vladimir, what's your opinion?
Hello,
On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
> <snip>
>>
>> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>>>> Hi Michel,
>>>>>>>>
>>>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>>>
>>>>>>> Consider this simple example, we need to add the following
>>>>>>> two prefixes with different next hops: 10.99.0.0/16,
>>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
>>>>>>> prefix is not added and Gatekeeper will make decisions in
>>>>>>> violation of the policy. The data structure of the LPM table is
>>>>>>> consistent, but its content inconsistent with the policy.
>>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
>> information. These 2 fields do not change based on the routes added and do
>> not indicate the amount of space left. So, you cannot use this information to
>> decide if there is enough space to add more routes.
Thanks Honnappa, agree, these two fields are read only after LPM
initialization, I confused them with rte_fib's "rsvd_tbl8s" and
"cur_tbl8s", so there is no need to read them directly from LPM after
initialization. I'd suggest just keeping them as external variables
outside of the LPM library (in kind of a global configuration I suppose?).
>>
>> We are aware that those fields hold the config information not a status of
>> the LPM table.
>>
>> Before updating a LPM table that holds network prefixes derived from
>> threat intelligence, we compute the minimum values for max_rules and
>> number_tbl8s. Here is an example of how we do it:
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
>> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
>>
>> Once these minimum values are available, we get the parameters of the
>> LPM table to be updated and check if we can update it, or have to recreate it.
>>
>>>>>> Aha, thanks. So do I understand correctly that you need to add a
>>>>>> set of routes atomically (either the entire set is installed or nothing)?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> If so, then I would suggest having 2 lpm and switching them
>>>>>> atomically after a successful addition. As for now, even if you
>>>>>> have enough tbl8's, routes are installed non atomically, i.e. there
>>>>>> will be a time gap between adding two routes, so in this time
>>>>>> interval the table will be inconsistent with the policy.
>>>>>> Also, if new lpm algorithms are added to the DPDK, they won't have
>>>>>> such a thing as tbl8.
>>>>>
>>>>> Our code already deals with synchronization.
>>> If the application code already deals with synchronization, is it possible to
>> revert back (i.e. delete the routes that got added so far) when the addition
>> of the route-set fails?
>>
>> The way the code is structured, this would require a significant rewrite
>> because the code assumes that it will succeed since the capacity of the LPM
>> tables was already checked.
>>
>>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>>>> Hi Kevin,
>>>>>>>>>
>>>>>>>>> We do need fields max_rules and number_tbl8s of struct
>>>>>>>>> rte_lpm, so the removal would force us to have another patch to
>>>>>>>>> our local copy of DPDK. We'd rather avoid this new local patch
>>>>>>>>> because we wish to eventually be in sync with the stock DPDK.
>>>>>>>>>
>>>>>>>>> Those fields are needed in Gatekeeper because we found a
>>>>>>>>> condition in an ongoing deployment in which the entries of some
>>>>>>>>> LPM tables may suddenly change a lot to reflect policy changes.
>>>>>>>>> To avoid getting into a state in which the LPM table is
>>>>>>>>> inconsistent because it cannot fit all the new entries, we
>>>>>>>>> compute the needed parameters to support the new entries, and
>>>>>>>>> compare with the current parameters. If the current table
>>>>>>>>> doesn't fit everything, we have to replace it with a new LPM table.
>>>>>>>>>
>>>>>>>>> If there were a way to obtain the struct rte_lpm_config of
>>>>>>>>> a given LPM table, it would cleanly address our need. We have
>>>>>>>>> the same need in IPv6 and have a local patch to work around it
>>>>>>>>> (see
>>>>>>>>>
>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>>>> 26a78115cb8c8f).
>>> I do not see why such an API is not possible, we could add one API that
>> returns max_rules and number_tbl8s (essentially, the config that was passed
>> to rte_lpm_create API).
>>> But, is there a possibility to store that info in the application as that data
>> was passed to rte_lpm from the application?
>>
>> A suggestion for what this API could look like:
>>
>> void rte_lpm_get_config(const struct rte_lpm *lpm, struct rte_lpm_config
>> *config); void rte_lpm6_get_config(const struct rte_lpm6 *lpm, struct
>> rte_lpm6_config *config);
>>
>> If the final choice is for not supporting a way to retrieve the config
>> information on the API, we'll look for a place to keep a copy of the
>> parameters in our code.
> IMO, this is not a performance critical path and it is not a difficult solution to store these values in the application. My suggestion is to skip adding the API and store the values in the application.
> Vladimir, what's your opinion?
Agree. Global vars or part of a global configuration could be used here.
>
<snip>
>
> Hello,
>
> On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
> > <snip>
> >>
> >> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
> >>>>>> On 13/10/2020 18:46, Michel Machado wrote:
> >>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
> >>>>>>>> Hi Michel,
> >>>>>>>>
> >>>>>>>> Could you please describe a condition when LPM gets inconsistent?
> >>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
> >>>>>>>
> >>>>>>> Consider this simple example, we need to add the following
> >>>>>>> two prefixes with different next hops: 10.99.0.0/16,
> >>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
> >>>>>>> prefix is not added and Gatekeeper will make decisions in
> >>>>>>> violation of the policy. The data structure of the LPM table is
> >>>>>>> consistent, but its content inconsistent with the policy.
> >>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
> >> information. These 2 fields do not change based on the routes added
> >> and do not indicate the amount of space left. So, you cannot use this
> >> information to decide if there is enough space to add more routes.
>
> Thanks Honnappa, agree, these two fields are read only after LPM
> initialization, I confused them with rte_fib's "rsvd_tbl8s" and "cur_tbl8s", so
> there is no need to read them directly from LPM after initialization. I'd
> suggest just keeping them as external variables outside of the LPM library (in
> kind of a global configuration I suppose?).
>
> >>
> >> We are aware that those fields hold the config information not a
> >> status of the LPM table.
> >>
> >> Before updating a LPM table that holds network prefixes derived
> >> from threat intelligence, we compute the minimum values for max_rules
> >> and number_tbl8s. Here is an example of how we do it:
> >>
> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
> >> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
> >>
> >> Once these minimum values are available, we get the parameters
> >> of the LPM table to be updated and check if we can update it, or have to
> recreate it.
> >>
> >>>>>> Aha, thanks. So do I understand correctly that you need to add a
> >>>>>> set of routes atomically (either the entire set is installed or nothing)?
> >>>>>
> >>>>> Yes.
> >>>>>
> >>>>>> If so, then I would suggest having 2 lpm and switching them
> >>>>>> atomically after a successful addition. As for now, even if you
> >>>>>> have enough tbl8's, routes are installed non atomically, i.e.
> >>>>>> there will be a time gap between adding two routes, so in this
> >>>>>> time interval the table will be inconsistent with the policy.
> >>>>>> Also, if new lpm algorithms are added to the DPDK, they won't
> >>>>>> have such a thing as tbl8.
> >>>>>
> >>>>> Our code already deals with synchronization.
> >>> If the application code already deals with synchronization, is it
> >>> possible to
> >> revert back (i.e. delete the routes that got added so far) when the
> >> addition of the route-set fails?
> >>
> >> The way the code is structured, this would require a significant
> >> rewrite because the code assumes that it will succeed since the
> >> capacity of the LPM tables was already checked.
> >>
> >>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
> >>>>>>>>> Hi Kevin,
> >>>>>>>>>
> >>>>>>>>> We do need fields max_rules and number_tbl8s of struct
> >>>>>>>>> rte_lpm, so the removal would force us to have another patch
> >>>>>>>>> to our local copy of DPDK. We'd rather avoid this new local
> >>>>>>>>> patch because we wish to eventually be in sync with the stock
> DPDK.
> >>>>>>>>>
> >>>>>>>>> Those fields are needed in Gatekeeper because we found a
> >>>>>>>>> condition in an ongoing deployment in which the entries of
> >>>>>>>>> some LPM tables may suddenly change a lot to reflect policy
> changes.
> >>>>>>>>> To avoid getting into a state in which the LPM table is
> >>>>>>>>> inconsistent because it cannot fit all the new entries, we
> >>>>>>>>> compute the needed parameters to support the new entries,
> and
> >>>>>>>>> compare with the current parameters. If the current table
> >>>>>>>>> doesn't fit everything, we have to replace it with a new LPM
> table.
> >>>>>>>>>
> >>>>>>>>> If there were a way to obtain the struct rte_lpm_config
> >>>>>>>>> of a given LPM table, it would cleanly address our need. We
> >>>>>>>>> have the same need in IPv6 and have a local patch to work
> >>>>>>>>> around it (see
> >>>>>>>>>
> >>>>
> >>
> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
> >>>> 26a78115cb8c8f).
> >>> I do not see why such an API is not possible, we could add one API
> >>> that
> >> returns max_rules and number_tbl8s (essentially, the config that was
> >> passed to rte_lpm_create API).
> >>> But, is there a possibility to store that info in the application as
> >>> that data
> >> was passed to rte_lpm from the application?
> >>
> >> A suggestion for what this API could look like:
> >>
> >> void rte_lpm_get_config(const struct rte_lpm *lpm, struct
> >> rte_lpm_config *config); void rte_lpm6_get_config(const struct
> >> rte_lpm6 *lpm, struct rte_lpm6_config *config);
> >>
> >> If the final choice is for not supporting a way to retrieve the
> >> config information on the API, we'll look for a place to keep a copy
> >> of the parameters in our code.
> > IMO, this is not a performance critical path and it is not a difficult solution to
> store these values in the application. My suggestion is to skip adding the API
> and store the values in the application.
> > Vladimir, what's your opinion?
>
> Agree. Global vars or part of a global configuration could be used here.
Thank you. I think we are fine to go ahead with merging this patch.
>
> >
>
> --
> Regards,
> Vladimir
On 15/10/2020 23:54, Honnappa Nagarahalli wrote:
> <snip>
>>
>> Hello,
>>
>> On 15/10/2020 18:38, Honnappa Nagarahalli wrote:
>>> <snip>
>>>>
>>>> On 10/14/20 7:57 PM, Honnappa Nagarahalli wrote:
>>>>>>>> On 13/10/2020 18:46, Michel Machado wrote:
>>>>>>>>> On 10/13/20 11:41 AM, Medvedkin, Vladimir wrote:
>>>>>>>>>> Hi Michel,
>>>>>>>>>>
>>>>>>>>>> Could you please describe a condition when LPM gets inconsistent?
>>>>>>>>>> As I can see if there is no free tbl8 it will return -ENOSPC.
>>>>>>>>>
>>>>>>>>> Consider this simple example, we need to add the following
>>>>>>>>> two prefixes with different next hops: 10.99.0.0/16,
>>>>>>>>> 18.99.99.128/25. If the LPM table is out of tbl8s, the second
>>>>>>>>> prefix is not added and Gatekeeper will make decisions in
>>>>>>>>> violation of the policy. The data structure of the LPM table is
>>>>>>>>> consistent, but its content inconsistent with the policy.
>>>>> max_rules and number_tbl8s in 'struct rte_lpm' contain the config
>>>> information. These 2 fields do not change based on the routes added
>>>> and do not indicate the amount of space left. So, you cannot use this
>>>> information to decide if there is enough space to add more routes.
>>
>> Thanks Honnappa, agree, these two fields are read only after LPM
>> initialization, I confused them with rte_fib's "rsvd_tbl8s" and "cur_tbl8s", so
>> there is no need to read them directly from LPM after initialization. I'd
>> suggest just keeping them as external variables outside of the LPM library (in
>> kind of a global configuration I suppose?).
>>
>>>>
>>>> We are aware that those fields hold the config information not a
>>>> status of the LPM table.
>>>>
>>>> Before updating a LPM table that holds network prefixes derived
>>>> from threat intelligence, we compute the minimum values for max_rules
>>>> and number_tbl8s. Here is an example of how we do it:
>>>>
>> https://github.com/AltraMayor/gatekeeper/blob/95d1d6e8201861a0d0c698
>>>> bfd06ad606674f1e07/lua/examples/policy.lua#L135-L166
>>>>
>>>> Once these minimum values are available, we get the parameters
>>>> of the LPM table to be updated and check if we can update it, or have to
>> recreate it.
>>>>
>>>>>>>> Aha, thanks. So do I understand correctly that you need to add a
>>>>>>>> set of routes atomically (either the entire set is installed or nothing)?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> If so, then I would suggest having 2 lpm and switching them
>>>>>>>> atomically after a successful addition. As for now, even if you
>>>>>>>> have enough tbl8's, routes are installed non atomically, i.e.
>>>>>>>> there will be a time gap between adding two routes, so in this
>>>>>>>> time interval the table will be inconsistent with the policy.
>>>>>>>> Also, if new lpm algorithms are added to the DPDK, they won't
>>>>>>>> have such a thing as tbl8.
>>>>>>>
>>>>>>> Our code already deals with synchronization.
>>>>> If the application code already deals with synchronization, is it
>>>>> possible to
>>>> revert back (i.e. delete the routes that got added so far) when the
>>>> addition of the route-set fails?
>>>>
>>>> The way the code is structured, this would require a significant
>>>> rewrite because the code assumes that it will succeed since the
>>>> capacity of the LPM tables was already checked.
>>>>
>>>>>>>>>> On 13/10/2020 15:58, Michel Machado wrote:
>>>>>>>>>>> Hi Kevin,
>>>>>>>>>>>
>>>>>>>>>>> We do need fields max_rules and number_tbl8s of struct
>>>>>>>>>>> rte_lpm, so the removal would force us to have another patch
>>>>>>>>>>> to our local copy of DPDK. We'd rather avoid this new local
>>>>>>>>>>> patch because we wish to eventually be in sync with the stock
>> DPDK.
>>>>>>>>>>>
>>>>>>>>>>> Those fields are needed in Gatekeeper because we found a
>>>>>>>>>>> condition in an ongoing deployment in which the entries of
>>>>>>>>>>> some LPM tables may suddenly change a lot to reflect policy
>> changes.
>>>>>>>>>>> To avoid getting into a state in which the LPM table is
>>>>>>>>>>> inconsistent because it cannot fit all the new entries, we
>>>>>>>>>>> compute the needed parameters to support the new entries,
>> and
>>>>>>>>>>> compare with the current parameters. If the current table
>>>>>>>>>>> doesn't fit everything, we have to replace it with a new LPM
>> table.
>>>>>>>>>>>
>>>>>>>>>>> If there were a way to obtain the struct rte_lpm_config
>>>>>>>>>>> of a given LPM table, it would cleanly address our need. We
>>>>>>>>>>> have the same need in IPv6 and have a local patch to work
>>>>>>>>>>> around it (see
>>>>>>>>>>>
>>>>>>
>>>>
>> https://github.com/cjdoucette/dpdk/commit/3eaf124a781349b8ec8cd880db
>>>>>> 26a78115cb8c8f).
>>>>> I do not see why such an API is not possible, we could add one API
>>>>> that
>>>> returns max_rules and number_tbl8s (essentially, the config that was
>>>> passed to rte_lpm_create API).
>>>>> But, is there a possibility to store that info in the application as
>>>>> that data
>>>> was passed to rte_lpm from the application?
>>>>
>>>> A suggestion for what this API could look like:
>>>>
>>>> void rte_lpm_get_config(const struct rte_lpm *lpm, struct
>>>> rte_lpm_config *config); void rte_lpm6_get_config(const struct
>>>> rte_lpm6 *lpm, struct rte_lpm6_config *config);
>>>>
>>>> If the final choice is for not supporting a way to retrieve the
>>>> config information on the API, we'll look for a place to keep a copy
>>>> of the parameters in our code.
>>> IMO, this is not a performance critical path and it is not a difficult solution to
>> store these values in the application. My suggestion is to skip adding the API
>> and store the values in the application.
>>> Vladimir, what's your opinion?
>>
>> Agree. Global vars or part of a global configuration could be used here.
> Thank you. I think we are fine to go ahead with merging this patch.
>
Michel, I know it's a bit of churn so not zero effort, but is that
solution workable for you? The change in DPDK would not be backported,
so gatekeeper code would only need an update on updating to use DPDK 20.11.
Kevin.
P.S. As you are using DPDK 19.08, we should talk about DPDK LTS but
let's leave that for another thread :-)
>>
>>>
>>
>> --
>> Regards,
>> Vladimir
>>>>> If the final choice is for not supporting a way to retrieve the
>>>>> config information on the API, we'll look for a place to keep a copy
>>>>> of the parameters in our code.
>>>> IMO, this is not a performance critical path and it is not a difficult solution to
>>> store these values in the application. My suggestion is to skip adding the API
>>> and store the values in the application.
>>>> Vladimir, what's your opinion?
>>>
>>> Agree. Global vars or part of a global configuration could be used here.
>> Thank you. I think we are fine to go ahead with merging this patch.
>>
>
> Michel, I know it's a bit of churn so not zero effort, but is that
> solution workable for you? The change in DPDK would not be backported,
> so gatekeeper code would only need an update on updating to use DPDK 20.11.
It's workable.
Thank you for organizing this discussion, Kevin.
> P.S. As you are using DPDK 19.08, we should talk about DPDK LTS but
> let's leave that for another thread :-)
Sure.
On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
> > > IMO, this is not a performance critical path and it is not a difficult solution to
> > store these values in the application. My suggestion is to skip adding the API
> > and store the values in the application.
> > > Vladimir, what's your opinion?
> >
> > Agree. Global vars or part of a global configuration could be used here.
> Thank you. I think we are fine to go ahead with merging this patch.
I saw Honnappa and Kevin acks, are other techboard members aware and
okay with this change?
Thanks.
<snip>
> > >>
> > >> 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>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
<snip>
19/10/2020 16:53, David Marchand:
> On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com> wrote:
> > > > IMO, this is not a performance critical path and it is not a difficult solution to
> > > store these values in the application. My suggestion is to skip adding the API
> > > and store the values in the application.
> > > > Vladimir, what's your opinion?
> > >
> > > Agree. Global vars or part of a global configuration could be used here.
> > Thank you. I think we are fine to go ahead with merging this patch.
>
> I saw Honnappa and Kevin acks, are other techboard members aware and
> okay with this change?
This is an API change for better maintenance.
I'm not sure the benefit is big enough,
but the cost for the applications is said to be workable.
Better to do it now than later, so
Acked-by: Thomas Monjalon <thomas@monjalon.net>
(at the condition it is added in release notes)
What others think?
Hi,
On 20/10/2020 15:22, Thomas Monjalon wrote:
> 19/10/2020 16:53, David Marchand:
>> On Fri, Oct 16, 2020 at 12:54 AM Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com> wrote:
>>>>> IMO, this is not a performance critical path and it is not a difficult solution to
>>>> store these values in the application. My suggestion is to skip adding the API
>>>> and store the values in the application.
>>>>> Vladimir, what's your opinion?
>>>>
>>>> Agree. Global vars or part of a global configuration could be used here.
>>> Thank you. I think we are fine to go ahead with merging this patch.
>>
>> I saw Honnappa and Kevin acks, are other techboard members aware and
>> okay with this change?
>
> This is an API change for better maintenance.
> I'm not sure the benefit is big enough,
> but the cost for the applications is said to be workable.
> Better to do it now than later, so
>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> (at the condition it is added in release notes)
>
> What others think?
>
This changes LGTM (agree with Thomas about release notes).
Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
>
@@ -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. */
+
/* RCU config. */
struct rte_rcu_qsbr *v; /* RCU QSBR variable. */
enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */
@@ -104,7 +112,7 @@ depth_to_range(uint8_t depth)
struct rte_lpm *
rte_lpm_find_existing(const char *name)
{
- struct rte_lpm *l = NULL;
+ struct __rte_lpm *l = NULL;
struct rte_tailq_entry *te;
struct rte_lpm_list *lpm_list;
@@ -123,7 +131,7 @@ rte_lpm_find_existing(const char *name)
return NULL;
}
- return l;
+ return &l->lpm;
}
/*
@@ -157,8 +165,8 @@ rte_lpm_create(const char *name, int socket_id,
/* guarantee there's no existing */
TAILQ_FOREACH(te, lpm_list, next) {
- lpm = te->data;
- if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
+ internal_lpm = te->data;
+ if (strncmp(name, internal_lpm->name, RTE_LPM_NAMESIZE) == 0)
break;
}
@@ -193,10 +201,10 @@ rte_lpm_create(const char *name, int socket_id,
}
lpm = &internal_lpm->lpm;
- lpm->rules_tbl = rte_zmalloc_socket(NULL,
+ internal_lpm->rules_tbl = rte_zmalloc_socket(NULL,
(size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
- if (lpm->rules_tbl == NULL) {
+ if (internal_lpm->rules_tbl == NULL) {
RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
rte_free(internal_lpm);
internal_lpm = NULL;
@@ -211,7 +219,7 @@ rte_lpm_create(const char *name, int socket_id,
if (lpm->tbl8 == NULL) {
RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
- rte_free(lpm->rules_tbl);
+ rte_free(internal_lpm->rules_tbl);
rte_free(internal_lpm);
internal_lpm = NULL;
lpm = NULL;
@@ -221,11 +229,11 @@ rte_lpm_create(const char *name, int socket_id,
}
/* Save user arguments. */
- lpm->max_rules = config->max_rules;
- lpm->number_tbl8s = config->number_tbl8s;
- strlcpy(lpm->name, name, sizeof(lpm->name));
+ internal_lpm->max_rules = config->max_rules;
+ internal_lpm->number_tbl8s = config->number_tbl8s;
+ strlcpy(internal_lpm->name, name, sizeof(internal_lpm->name));
- te->data = lpm;
+ te->data = internal_lpm;
TAILQ_INSERT_TAIL(lpm_list, te, next);
@@ -241,7 +249,7 @@ rte_lpm_create(const char *name, int socket_id,
void
rte_lpm_free(struct rte_lpm *lpm)
{
- struct __rte_lpm *internal_lpm;
+ struct __rte_lpm *internal_lpm = NULL;
struct rte_lpm_list *lpm_list;
struct rte_tailq_entry *te;
@@ -255,7 +263,8 @@ rte_lpm_free(struct rte_lpm *lpm)
/* find our tailq entry */
TAILQ_FOREACH(te, lpm_list, next) {
- if (te->data == (void *) lpm)
+ internal_lpm = te->data;
+ if (&internal_lpm->lpm == lpm)
break;
}
if (te != NULL)
@@ -263,11 +272,10 @@ rte_lpm_free(struct rte_lpm *lpm)
rte_mcfg_tailq_write_unlock();
- internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
if (internal_lpm->dq != NULL)
rte_rcu_qsbr_dq_delete(internal_lpm->dq);
rte_free(lpm->tbl8);
- rte_free(lpm->rules_tbl);
+ rte_free(internal_lpm->rules_tbl);
rte_free(internal_lpm);
rte_free(te);
}
@@ -310,11 +318,11 @@ rte_lpm_rcu_qsbr_add(struct rte_lpm *lpm, struct rte_lpm_rcu_config *cfg)
} else if (cfg->mode == RTE_LPM_QSBR_MODE_DQ) {
/* Init QSBR defer queue. */
snprintf(rcu_dq_name, sizeof(rcu_dq_name),
- "LPM_RCU_%s", lpm->name);
+ "LPM_RCU_%s", internal_lpm->name);
params.name = rcu_dq_name;
params.size = cfg->dq_size;
if (params.size == 0)
- params.size = lpm->number_tbl8s;
+ params.size = internal_lpm->number_tbl8s;
params.trigger_reclaim_limit = cfg->reclaim_thd;
params.max_reclaim_size = cfg->reclaim_max;
if (params.max_reclaim_size == 0)
@@ -352,74 +360,79 @@ static int32_t
rule_add(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth,
uint32_t next_hop)
{
- uint32_t rule_gindex, rule_index, last_rule;
+ uint32_t rule_gindex, rule_index, last_rule, first_index;
+ struct __rte_lpm *i_lpm;
int i;
VERIFY_DEPTH(depth);
+ i_lpm = container_of(lpm, struct __rte_lpm, lpm);
/* Scan through rule group to see if rule already exists. */
- if (lpm->rule_info[depth - 1].used_rules > 0) {
+ if (i_lpm->rule_info[depth - 1].used_rules > 0) {
/* rule_gindex stands for rule group index. */
- rule_gindex = lpm->rule_info[depth - 1].first_rule;
+ rule_gindex = i_lpm->rule_info[depth - 1].first_rule;
/* Initialise rule_index to point to start of rule group. */
rule_index = rule_gindex;
/* Last rule = Last used rule in this rule group. */
- last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+ last_rule = rule_gindex
+ + i_lpm->rule_info[depth - 1].used_rules;
for (; rule_index < last_rule; rule_index++) {
/* If rule already exists update next hop and return. */
- if (lpm->rules_tbl[rule_index].ip == ip_masked) {
+ if (i_lpm->rules_tbl[rule_index].ip == ip_masked) {
- if (lpm->rules_tbl[rule_index].next_hop
+ if (i_lpm->rules_tbl[rule_index].next_hop
== next_hop)
return -EEXIST;
- lpm->rules_tbl[rule_index].next_hop = next_hop;
+ i_lpm->rules_tbl[rule_index].next_hop
+ = next_hop;
return rule_index;
}
}
- if (rule_index == lpm->max_rules)
+ if (rule_index == i_lpm->max_rules)
return -ENOSPC;
} else {
/* Calculate the position in which the rule will be stored. */
rule_index = 0;
for (i = depth - 1; i > 0; i--) {
- if (lpm->rule_info[i - 1].used_rules > 0) {
- rule_index = lpm->rule_info[i - 1].first_rule
- + lpm->rule_info[i - 1].used_rules;
+ if (i_lpm->rule_info[i - 1].used_rules > 0) {
+ rule_index = i_lpm->rule_info[i - 1].first_rule
+ + i_lpm->rule_info[i - 1].used_rules;
break;
}
}
- if (rule_index == lpm->max_rules)
+ if (rule_index == i_lpm->max_rules)
return -ENOSPC;
- lpm->rule_info[depth - 1].first_rule = rule_index;
+ i_lpm->rule_info[depth - 1].first_rule = rule_index;
}
/* Make room for the new rule in the array. */
for (i = RTE_LPM_MAX_DEPTH; i > depth; i--) {
- if (lpm->rule_info[i - 1].first_rule
- + lpm->rule_info[i - 1].used_rules == lpm->max_rules)
+ first_index = i_lpm->rule_info[i - 1].first_rule;
+ if (first_index + i_lpm->rule_info[i - 1].used_rules
+ == i_lpm->max_rules)
return -ENOSPC;
- if (lpm->rule_info[i - 1].used_rules > 0) {
- lpm->rules_tbl[lpm->rule_info[i - 1].first_rule
- + lpm->rule_info[i - 1].used_rules]
- = lpm->rules_tbl[lpm->rule_info[i - 1].first_rule];
- lpm->rule_info[i - 1].first_rule++;
+ if (i_lpm->rule_info[i - 1].used_rules > 0) {
+ i_lpm->rules_tbl[first_index
+ + i_lpm->rule_info[i - 1].used_rules]
+ = i_lpm->rules_tbl[first_index];
+ i_lpm->rule_info[i - 1].first_rule++;
}
}
/* Add the new rule. */
- lpm->rules_tbl[rule_index].ip = ip_masked;
- lpm->rules_tbl[rule_index].next_hop = next_hop;
+ i_lpm->rules_tbl[rule_index].ip = ip_masked;
+ i_lpm->rules_tbl[rule_index].next_hop = next_hop;
/* Increment the used rules counter for this rule group. */
- lpm->rule_info[depth - 1].used_rules++;
+ i_lpm->rule_info[depth - 1].used_rules++;
return rule_index;
}
@@ -432,23 +445,25 @@ static void
rule_delete(struct rte_lpm *lpm, int32_t rule_index, uint8_t depth)
{
int i;
+ struct __rte_lpm *i_lpm;
VERIFY_DEPTH(depth);
- lpm->rules_tbl[rule_index] =
- lpm->rules_tbl[lpm->rule_info[depth - 1].first_rule
- + lpm->rule_info[depth - 1].used_rules - 1];
+ i_lpm = container_of(lpm, struct __rte_lpm, lpm);
+ i_lpm->rules_tbl[rule_index] =
+ i_lpm->rules_tbl[i_lpm->rule_info[depth - 1].first_rule
+ + i_lpm->rule_info[depth - 1].used_rules - 1];
for (i = depth; i < RTE_LPM_MAX_DEPTH; i++) {
- if (lpm->rule_info[i].used_rules > 0) {
- lpm->rules_tbl[lpm->rule_info[i].first_rule - 1] =
- lpm->rules_tbl[lpm->rule_info[i].first_rule
- + lpm->rule_info[i].used_rules - 1];
- lpm->rule_info[i].first_rule--;
+ if (i_lpm->rule_info[i].used_rules > 0) {
+ i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule - 1] =
+ i_lpm->rules_tbl[i_lpm->rule_info[i].first_rule
+ + i_lpm->rule_info[i].used_rules - 1];
+ i_lpm->rule_info[i].first_rule--;
}
}
- lpm->rule_info[depth - 1].used_rules--;
+ i_lpm->rule_info[depth - 1].used_rules--;
}
/*
@@ -459,16 +474,18 @@ static int32_t
rule_find(struct rte_lpm *lpm, uint32_t ip_masked, uint8_t depth)
{
uint32_t rule_gindex, last_rule, rule_index;
+ struct __rte_lpm *internal_lpm;
VERIFY_DEPTH(depth);
- rule_gindex = lpm->rule_info[depth - 1].first_rule;
- last_rule = rule_gindex + lpm->rule_info[depth - 1].used_rules;
+ internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
+ rule_gindex = internal_lpm->rule_info[depth - 1].first_rule;
+ last_rule = rule_gindex + internal_lpm->rule_info[depth - 1].used_rules;
/* Scan used rules at given depth to find rule. */
for (rule_index = rule_gindex; rule_index < last_rule; rule_index++) {
/* If rule is found return the rule index. */
- if (lpm->rules_tbl[rule_index].ip == ip_masked)
+ if (internal_lpm->rules_tbl[rule_index].ip == ip_masked)
return rule_index;
}
@@ -484,9 +501,11 @@ _tbl8_alloc(struct rte_lpm *lpm)
{
uint32_t group_idx; /* tbl8 group index. */
struct rte_lpm_tbl_entry *tbl8_entry;
+ struct __rte_lpm *i_lpm;
+ i_lpm = container_of(lpm, struct __rte_lpm, lpm);
/* Scan through tbl8 to find a free (i.e. INVALID) tbl8 group. */
- for (group_idx = 0; group_idx < lpm->number_tbl8s; group_idx++) {
+ for (group_idx = 0; group_idx < i_lpm->number_tbl8s; group_idx++) {
tbl8_entry = &lpm->tbl8[group_idx *
RTE_LPM_TBL8_GROUP_NUM_ENTRIES];
/* If a free tbl8 group is found clean it and set as VALID. */
@@ -844,6 +863,7 @@ uint32_t *next_hop)
{
uint32_t ip_masked;
int32_t rule_index;
+ struct __rte_lpm *internal_lpm;
/* Check user arguments. */
if ((lpm == NULL) ||
@@ -855,8 +875,9 @@ uint32_t *next_hop)
ip_masked = ip & depth_to_mask(depth);
rule_index = rule_find(lpm, ip_masked, depth);
+ internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
if (rule_index >= 0) {
- *next_hop = lpm->rules_tbl[rule_index].next_hop;
+ *next_hop = internal_lpm->rules_tbl[rule_index].next_hop;
return 1;
}
@@ -897,7 +918,9 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
tbl24_range = depth_to_range(depth);
tbl24_index = (ip_masked >> 8);
struct rte_lpm_tbl_entry zero_tbl24_entry = {0};
+ struct __rte_lpm *i_lpm;
+ i_lpm = container_of(lpm, struct __rte_lpm, lpm);
/*
* Firstly check the sub_rule_index. A -1 indicates no replacement rule
* and a positive number indicates a sub_rule_index.
@@ -939,7 +962,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
*/
struct rte_lpm_tbl_entry new_tbl24_entry = {
- .next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+ .next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
.valid = VALID,
.valid_group = 0,
.depth = sub_rule_depth,
@@ -949,7 +972,7 @@ delete_depth_small(struct rte_lpm *lpm, uint32_t ip_masked,
.valid = VALID,
.valid_group = VALID,
.depth = sub_rule_depth,
- .next_hop = lpm->rules_tbl
+ .next_hop = i_lpm->rules_tbl
[sub_rule_index].next_hop,
};
@@ -1048,6 +1071,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
uint32_t tbl24_index, tbl8_group_index, tbl8_group_start, tbl8_index,
tbl8_range, i;
int32_t tbl8_recycle_index, status = 0;
+ struct __rte_lpm *i_lpm;
/*
* Calculate the index into tbl24 and range. Note: All depths larger
@@ -1061,6 +1085,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
tbl8_index = tbl8_group_start + (ip_masked & 0xFF);
tbl8_range = depth_to_range(depth);
+ i_lpm = container_of(lpm, struct __rte_lpm, lpm);
if (sub_rule_index < 0) {
/*
* Loop through the range of entries on tbl8 for which the
@@ -1076,7 +1101,7 @@ delete_depth_big(struct rte_lpm *lpm, uint32_t ip_masked,
.valid = VALID,
.depth = sub_rule_depth,
.valid_group = lpm->tbl8[tbl8_group_start].valid_group,
- .next_hop = lpm->rules_tbl[sub_rule_index].next_hop,
+ .next_hop = i_lpm->rules_tbl[sub_rule_index].next_hop,
};
/*
@@ -1188,16 +1213,21 @@ rte_lpm_delete(struct rte_lpm *lpm, uint32_t ip, uint8_t depth)
void
rte_lpm_delete_all(struct rte_lpm *lpm)
{
+ struct __rte_lpm *internal_lpm;
+
+ internal_lpm = container_of(lpm, struct __rte_lpm, lpm);
/* Zero rule information. */
- memset(lpm->rule_info, 0, sizeof(lpm->rule_info));
+ memset(internal_lpm->rule_info, 0, sizeof(internal_lpm->rule_info));
/* Zero tbl24. */
memset(lpm->tbl24, 0, sizeof(lpm->tbl24));
/* Zero tbl8. */
memset(lpm->tbl8, 0, sizeof(lpm->tbl8[0])
- * RTE_LPM_TBL8_GROUP_NUM_ENTRIES * lpm->number_tbl8s);
+ * RTE_LPM_TBL8_GROUP_NUM_ENTRIES
+ * internal_lpm->number_tbl8s);
/* Delete all rules form the rules table. */
- memset(lpm->rules_tbl, 0, sizeof(lpm->rules_tbl[0]) * lpm->max_rules);
+ memset(internal_lpm->rules_tbl, 0,
+ sizeof(internal_lpm->rules_tbl[0]) * internal_lpm->max_rules);
}
@@ -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. */
};
/** LPM RCU QSBR configuration structure. */