From: "Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
Michel Machado <michel@digirati.com.br>,
Kevin Traynor <ktraynor@redhat.com>,
Ruifeng Wang <Ruifeng.Wang@arm.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Cody Doucette <doucette@bu.edu>,
Andre Nathan <andre@digirati.com.br>,
Qiaobin Fu <mengxiang0811@gmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
Date: Thu, 15 Oct 2020 20:30:23 +0100 [thread overview]
Message-ID: <505d3e00-717b-25f8-ffea-d6108165a12a@intel.com> (raw)
In-Reply-To: <DBAPR08MB5814D3F01B00EC9DF0B85C7698020@DBAPR08MB5814.eurprd08.prod.outlook.com>
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.
>
--
Regards,
Vladimir
next prev parent reply other threads:[~2020-10-15 19:30 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-07 8:15 [dpdk-dev] [PATCH 0/2] LPM changes Ruifeng Wang
2020-09-07 8:15 ` [dpdk-dev] [PATCH 1/2] lpm: fix free of data structure Ruifeng Wang
2020-09-15 15:55 ` Bruce Richardson
2020-09-15 16:25 ` Medvedkin, Vladimir
2020-09-07 8:15 ` [dpdk-dev] [PATCH 2/2] lpm: hide internal data Ruifeng Wang
2020-09-15 16:02 ` Bruce Richardson
2020-09-15 16:28 ` Medvedkin, Vladimir
2020-09-16 3:17 ` Ruifeng Wang
2020-09-30 8:45 ` Kevin Traynor
2020-10-09 6:54 ` Ruifeng Wang
2020-10-13 13:53 ` Kevin Traynor
2020-10-13 14:58 ` Michel Machado
2020-10-13 15:41 ` Medvedkin, Vladimir
2020-10-13 17:46 ` Michel Machado
2020-10-13 19:06 ` Medvedkin, Vladimir
2020-10-13 19:48 ` Michel Machado
2020-10-14 13:10 ` Medvedkin, Vladimir
2020-10-14 23:57 ` Honnappa Nagarahalli
2020-10-15 13:39 ` Michel Machado
2020-10-15 17:38 ` Honnappa Nagarahalli
2020-10-15 19:30 ` Medvedkin, Vladimir [this message]
2020-10-15 22:54 ` Honnappa Nagarahalli
2020-10-16 11:39 ` Kevin Traynor
2020-10-16 13:55 ` Michel Machado
2020-10-19 14:53 ` David Marchand
2020-10-20 14:22 ` Thomas Monjalon
2020-10-20 14:32 ` Medvedkin, Vladimir
2020-10-19 17:53 ` Honnappa Nagarahalli
2020-09-15 14:41 ` [dpdk-dev] [PATCH 0/2] LPM changes David Marchand
2020-10-19 13:37 ` Kevin Traynor
2020-10-21 3:02 ` [dpdk-dev] [PATCH v2 " Ruifeng Wang
2020-10-21 3:02 ` [dpdk-dev] [PATCH v2 1/2] lpm: fix free of data structure Ruifeng Wang
2020-10-21 3:02 ` [dpdk-dev] [PATCH v2 2/2] lpm: hide internal data Ruifeng Wang
2020-10-21 7:58 ` Thomas Monjalon
2020-10-21 8:15 ` Ruifeng Wang
2020-10-22 15:14 ` David Marchand
2020-10-23 6:13 ` Ruifeng Wang
2020-10-23 16:08 ` Medvedkin, Vladimir
2020-10-23 9:38 ` [dpdk-dev] [PATCH v3 0/2] LPM changes David Marchand
2020-10-23 9:38 ` [dpdk-dev] [PATCH v3 1/2] lpm: fix free of data structure David Marchand
2020-10-23 9:38 ` [dpdk-dev] [PATCH v3 2/2] lpm: hide internal data David Marchand
2020-10-26 8:26 ` [dpdk-dev] [PATCH v3 0/2] LPM changes David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=505d3e00-717b-25f8-ffea-d6108165a12a@intel.com \
--to=vladimir.medvedkin@intel.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=andre@digirati.com.br \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=doucette@bu.edu \
--cc=ktraynor@redhat.com \
--cc=mengxiang0811@gmail.com \
--cc=michel@digirati.com.br \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).