DPDK patches and discussions
 help / color / mirror / Atom feed
From: Kevin Traynor <ktraynor@redhat.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	"Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>,
	Michel Machado <michel@digirati.com.br>,
	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>,
	"techboard@dpdk.org" <techboard@dpdk.org>,
	David Marchand <david.marchand@redhat.com>,
	"thomas@monjalon.net" <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] lpm: hide internal data
Date: Fri, 16 Oct 2020 12:39:23 +0100	[thread overview]
Message-ID: <3d74eba7-1b27-2b78-7ed3-de1307bdfddc@redhat.com> (raw)
In-Reply-To: <DBAPR08MB581430BDB3D55AF6F611C0B498020@DBAPR08MB5814.eurprd08.prod.outlook.com>

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


  reply	other threads:[~2020-10-16 11:39 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
2020-10-15 22:54                                   ` Honnappa Nagarahalli
2020-10-16 11:39                                     ` Kevin Traynor [this message]
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=3d74eba7-1b27-2b78-7ed3-de1307bdfddc@redhat.com \
    --to=ktraynor@redhat.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Ruifeng.Wang@arm.com \
    --cc=andre@digirati.com.br \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=doucette@bu.edu \
    --cc=mengxiang0811@gmail.com \
    --cc=michel@digirati.com.br \
    --cc=nd@arm.com \
    --cc=techboard@dpdk.org \
    --cc=thomas@monjalon.net \
    --cc=vladimir.medvedkin@intel.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).