Re: [nft RFC PATCH] src: add set optimization options

[Pablo Neira Ayuso <pablo@netfilter.org>]

Cc'ing Patrick.

On Wed, Aug 13, 2014 at 10:17:06AM +0200, Arturo Borrero Gonzalez wrote:
> This patch adds options to choose set optimization mechanisms.
> 
> The syntax is one of:
> 
>  nft add set filter set1 { type ipv4_addr size 1024 ; }
>  nft add set filter set1 { type ipv4_addr policy memory ; }
>  nft add set filter set1 { type ipv4_addr policy performance ; }
>  nft add set filter set1 { type ipv4_addr policy memory size 1024 ; }
>  nft add set filter set1 { type ipv4_addr size 1024 policy memory ; }
>  nft add set filter set1 { type ipv4_addr policy performance size 1024 ; }
>  nft add set filter set1 { type ipv4_addr size 1024 policy performance ; }

@Patrick: Does this syntax look reasonable to you?

> Also valid for maps:
> 
>  nft add map filter map1 { type ipv4_addr : verdict policy performace ; }
>  [...]
> 
> 
> This is the output format, which can be imported later with `nft -f':
> 
> table filter {
> 	set set1 {
> 		type ipv4_addr policy memory size 1024
> 	}
> }
> 
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
> 
> This is my proposal for set internal mechanism selection in nft.
> 
> My idea is: given the kernel uses optional arguments to choose the
> set mechanism, in userspace the configuration should be also optional.
> 
> The patch is not fully tested, there are still some issues; It seems that the
> kernel is lacking some details.
> For example, it doesn't dump back to userspace the policy configuration.
> 
> In my opinion, we should be able to inform userspace of the configuration, in a
> way that userspace can differentiate between default values and fixed ones.
> For example, NFT_SET_POL_PERFORMANCE seems to be the default in kernel, but
> we don't want every set to be printed with "policy performance".
> 
> Please, feel free to comment.

Just one minor comment:

     struct {                                                                 
             uint32_t        flags;                                           
             uint32_t        policy;                                          
             struct {                                                         
                     uint32_t        size;                                    
             } desc;                                                          
     } mechanism;                                                             
};

I prefer if you map 1:1 what we have in the kernel:

        struct set {
                ...
                uint32_t policy;
                struct {
                        uint32_t size;
                        uint32_t flags;
                } desc;
        };

Thanks!