Re: RFC: Designing per chain rule cache support in libnftnl
Hi, On Wed, Nov 28, 2018 at 02:51:54PM +0100, Pablo Neira Ayuso wrote: > On Wed, Nov 28, 2018 at 02:21:01PM +0100, Phil Sutter wrote: > > Hi Pablo, > > > > On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote: > > > On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote: > > > > Phil Sutter wrote: > > > > > > If user doesn't want it cleared at nftnl_chain_free() time they can > > > > > > always allocate a new nftnl_rule_list and splice to that list. > > > > > > > > > > Good point. What do you think about the simple approach of > > > > > introducing: > > > > > > > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct > > > > > nftnl_chain *); > > > > > > > > Looks fine to me. > > > > > > > > > This would allow to reuse nftnl_rule_list routines from > > > > > libnftnl/rule.h. > > > > > One potential problem I see is that users may try to call > > > > > nftnl_rule_list_free(). Can we prevent that somehow? > > > > > > > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() > > > > :-) > > > > > > > > I don't think its an issue. > > > > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to > > > > true for nftnl_rule_list structures that are allocated indirectly on > > > > behalf of nftnl_chain struct, but I think thats taking things too far. > > > > > > Can we have an interface similar to nftnl_rule_add_expr() to add rules > > > to chains? > > > > > > So we add list field to nftnl_chain, and this new interface to > > > add/delete rules. > > > > I didn't look at struct nftnl_rule yet. OK, that seems rather different > > from what I had in mind. So I guess your idea would be to add a field of > > type struct list_head instead of struct nftnl_rule_list and implement > > struct nftnl_rule_iter and relevant functions? > > Yes. We would make explicit the relation between the objects, which > makes sense to me. So far only nftnl_rule and nftnl_expr are basically > "linked" in some way. > > Would this approach for you? Yes, that's fine with me. My idea was to reuse the nftnl_rule_list API, but creating chains' rule lists in a consistent manner with respect to rules' expression lists is probably more important long-term. Thanks, Phil
Re: RFC: Designing per chain rule cache support in libnftnl
On Wed, Nov 28, 2018 at 02:21:01PM +0100, Phil Sutter wrote: > Hi Pablo, > > On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote: > > On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote: > > > Phil Sutter wrote: > > > > > If user doesn't want it cleared at nftnl_chain_free() time they can > > > > > always allocate a new nftnl_rule_list and splice to that list. > > > > > > > > Good point. What do you think about the simple approach of introducing: > > > > > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct > > > > nftnl_chain *); > > > > > > Looks fine to me. > > > > > > > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. > > > > One potential problem I see is that users may try to call > > > > nftnl_rule_list_free(). Can we prevent that somehow? > > > > > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() > > > :-) > > > > > > I don't think its an issue. > > > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to > > > true for nftnl_rule_list structures that are allocated indirectly on > > > behalf of nftnl_chain struct, but I think thats taking things too far. > > > > Can we have an interface similar to nftnl_rule_add_expr() to add rules > > to chains? > > > > So we add list field to nftnl_chain, and this new interface to > > add/delete rules. > > I didn't look at struct nftnl_rule yet. OK, that seems rather different > from what I had in mind. So I guess your idea would be to add a field of > type struct list_head instead of struct nftnl_rule_list and implement > struct nftnl_rule_iter and relevant functions? Yes. We would make explicit the relation between the objects, which makes sense to me. So far only nftnl_rule and nftnl_expr are basically "linked" in some way. Would this approach for you? Thanks!
Re: RFC: Designing per chain rule cache support in libnftnl
Hi Pablo, On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote: > On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote: > > Phil Sutter wrote: > > > > If user doesn't want it cleared at nftnl_chain_free() time they can > > > > always allocate a new nftnl_rule_list and splice to that list. > > > > > > Good point. What do you think about the simple approach of introducing: > > > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct > > > nftnl_chain *); > > > > Looks fine to me. > > > > > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. > > > One potential problem I see is that users may try to call > > > nftnl_rule_list_free(). Can we prevent that somehow? > > > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() :-) > > > > I don't think its an issue. > > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to > > true for nftnl_rule_list structures that are allocated indirectly on > > behalf of nftnl_chain struct, but I think thats taking things too far. > > Can we have an interface similar to nftnl_rule_add_expr() to add rules > to chains? > > So we add list field to nftnl_chain, and this new interface to > add/delete rules. I didn't look at struct nftnl_rule yet. OK, that seems rather different from what I had in mind. So I guess your idea would be to add a field of type struct list_head instead of struct nftnl_rule_list and implement struct nftnl_rule_iter and relevant functions? > We can probably deprecate the existing list interface if we follow > that procedure after a bit of time in favour of this one. OK, cool. Thanks, Phil
Re: RFC: Designing per chain rule cache support in libnftnl
On Fri, Nov 23, 2018 at 01:35:17PM +0100, Pablo Neira Ayuso wrote: > On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote: > > Phil Sutter wrote: > > > > If user doesn't want it cleared at nftnl_chain_free() time they can > > > > always allocate a new nftnl_rule_list and splice to that list. > > > > > > Good point. What do you think about the simple approach of introducing: > > > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct > > > nftnl_chain *); > > > > Looks fine to me. > > > > > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. > > > One potential problem I see is that users may try to call > > > nftnl_rule_list_free(). Can we prevent that somehow? > > > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() :-) > > > > I don't think its an issue. > > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to > > true for nftnl_rule_list structures that are allocated indirectly on > > behalf of nftnl_chain struct, but I think thats taking things too far. > > Can we have an interface similar to nftnl_rule_add_expr() to add rules > to chains? > > So we add list field to nftnl_chain, and this new interface to > add/delete rules. We can add an internal hashtable, that allows lookup by handle. Also add iterators à la nftnl_expr_foreach() too.
Re: RFC: Designing per chain rule cache support in libnftnl
On Fri, Nov 23, 2018 at 12:25:45PM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > > If user doesn't want it cleared at nftnl_chain_free() time they can > > > always allocate a new nftnl_rule_list and splice to that list. > > > > Good point. What do you think about the simple approach of introducing: > > > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct > > nftnl_chain *); > > Looks fine to me. > > > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. > > One potential problem I see is that users may try to call > > nftnl_rule_list_free(). Can we prevent that somehow? > > Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() :-) > > I don't think its an issue. > We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to > true for nftnl_rule_list structures that are allocated indirectly on > behalf of nftnl_chain struct, but I think thats taking things too far. Can we have an interface similar to nftnl_rule_add_expr() to add rules to chains? So we add list field to nftnl_chain, and this new interface to add/delete rules. We can probably deprecate the existing list interface if we follow that procedure after a bit of time in favour of this one.
Re: RFC: Designing per chain rule cache support in libnftnl
Phil Sutter wrote: > > If user doesn't want it cleared at nftnl_chain_free() time they can > > always allocate a new nftnl_rule_list and splice to that list. > > Good point. What do you think about the simple approach of introducing: > > | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct nftnl_chain > *); Looks fine to me. > This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. > One potential problem I see is that users may try to call > nftnl_rule_list_free(). Can we prevent that somehow? Document that nftnl_rule_list_free() pairs with nftnl_rule_list_alloc() :-) I don't think its an issue. We could add a 'bool make_free_no_op' to nftnl_rule_list and set that to true for nftnl_rule_list structures that are allocated indirectly on behalf of nftnl_chain struct, but I think thats taking things too far. > A more fool-proof (but somewhat tedious) solution would be to duplicate > nftnl_rule_list API for use on an nftnl_chain. But I don't quite like > that. I don't like it either, API bloat is problem.
Re: RFC: Designing per chain rule cache support in libnftnl
On Fri, Nov 23, 2018 at 07:49:49AM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > In order to improve performance in 'nft -f' as well as xtables-restore > > with very large rulesets, we need to store rules by chain they belong > > to. In order to avoid pointless code duplication, this should be > > supported by libnftnl. > > Unfortunately we still need to change lookup algorithm as well > (hash, tree?), linear list scan is too expensive. > > We might even need multiple internal ways to keep track of the chains, > e.g. to accelerate insert/delete-by-index :-/ That's right. I would "hide" these details within struct nftnl_rule_list though and provide appropriate lookup routines. For now, I'm focussing on the API, if we get it right the data structure behind it is replaceable/extensible at will. > > Looking into the topic, it seems like extending struct nftnl_chain is > > the most straightforward way to go. My idea is to embed an > > nftnl_rule_list in there, though I'm unsure how to best do that in > > practice: > > > > We could either add a field of type struct nftnl_rule_list which would > > have to be initialized/cleared in nftnl_chain_alloc() and > > nftnl_chain_free(). This would be accompanied by a function to retrieve > > the pointer to that field so the existing rule_list routines may be used > > with it. > > > > Another option would be to add a pointer to a struct nftnl_rule_list. > > Having a function to retrieve a pointer to that pointer, the rule_list > > could be initialized/cleared by users on demand. > > > > What do you consider more practical? Is there a third option I didn't > > think of yet? > > I'd vote for the former (embed nftnl_rule_list). OK, thanks. > If user doesn't want it cleared at nftnl_chain_free() time they can > always allocate a new nftnl_rule_list and splice to that list. Good point. What do you think about the simple approach of introducing: | struct nftnl_rule_list *nftnl_chain_get_rule_list(const struct nftnl_chain *); This would allow to reuse nftnl_rule_list routines from libnftnl/rule.h. One potential problem I see is that users may try to call nftnl_rule_list_free(). Can we prevent that somehow? A more fool-proof (but somewhat tedious) solution would be to duplicate nftnl_rule_list API for use on an nftnl_chain. But I don't quite like that. Cheers, Phil
Re: RFC: Designing per chain rule cache support in libnftnl
Phil Sutter wrote: > In order to improve performance in 'nft -f' as well as xtables-restore > with very large rulesets, we need to store rules by chain they belong > to. In order to avoid pointless code duplication, this should be > supported by libnftnl. Unfortunately we still need to change lookup algorithm as well (hash, tree?), linear list scan is too expensive. We might even need multiple internal ways to keep track of the chains, e.g. to accelerate insert/delete-by-index :-/ > Looking into the topic, it seems like extending struct nftnl_chain is > the most straightforward way to go. My idea is to embed an > nftnl_rule_list in there, though I'm unsure how to best do that in > practice: > > We could either add a field of type struct nftnl_rule_list which would > have to be initialized/cleared in nftnl_chain_alloc() and > nftnl_chain_free(). This would be accompanied by a function to retrieve > the pointer to that field so the existing rule_list routines may be used > with it. > > Another option would be to add a pointer to a struct nftnl_rule_list. > Having a function to retrieve a pointer to that pointer, the rule_list > could be initialized/cleared by users on demand. > > What do you consider more practical? Is there a third option I didn't > think of yet? I'd vote for the former (embed nftnl_rule_list). If user doesn't want it cleared at nftnl_chain_free() time they can always allocate a new nftnl_rule_list and splice to that list.
RFC: Designing per chain rule cache support in libnftnl
Hi, In order to improve performance in 'nft -f' as well as xtables-restore with very large rulesets, we need to store rules by chain they belong to. In order to avoid pointless code duplication, this should be supported by libnftnl. Looking into the topic, it seems like extending struct nftnl_chain is the most straightforward way to go. My idea is to embed an nftnl_rule_list in there, though I'm unsure how to best do that in practice: We could either add a field of type struct nftnl_rule_list which would have to be initialized/cleared in nftnl_chain_alloc() and nftnl_chain_free(). This would be accompanied by a function to retrieve the pointer to that field so the existing rule_list routines may be used with it. Another option would be to add a pointer to a struct nftnl_rule_list. Having a function to retrieve a pointer to that pointer, the rule_list could be initialized/cleared by users on demand. What do you consider more practical? Is there a third option I didn't think of yet? Thanks, Phil
