<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
