Re: [PATCH net-next 4/5] ipv4: route: dissect flow in input path if fib rules need it

[Paolo Abeni <pabeni@redhat.com>]

On Sat, 2018-02-24 at 21:44 -0800, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> 
> Dissect flow in fwd path if fib rules require it. Controlled by
> a flag to avoid penatly for the common case. Flag is set when fib
> rules with sport, dport and proto match that require flow dissect
> are installed. Also passes the dissected hash keys to the multipath
> hash function when applicable to avoid dissecting the flow again.
> icmp packets will continue to use inner header for hash
> calculations (Thanks to Nikolay Aleksandrov for some review here).
> 
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  include/net/ip_fib.h     |  2 +-
>  include/net/netns/ipv4.h |  1 +
>  net/ipv4/fib_rules.c     |  6 ++++++
>  net/ipv4/fib_semantics.c |  2 +-
>  net/ipv4/route.c         | 52 +++++++++++++++++++++++++++++++++++-------------
>  5 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index f805243..5ada772 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -371,7 +371,7 @@ int fib_sync_up(struct net_device *dev, unsigned int nh_flags);
>  
>  #ifdef CONFIG_IP_ROUTE_MULTIPATH
>  int fib_multipath_hash(const struct fib_info *fi, const struct flowi4 *fl4,
> -		       const struct sk_buff *skb);
> +		       const struct sk_buff *skb, struct flow_keys *flkeys);
>  #endif
>  void fib_select_multipath(struct fib_result *res, int hash);
>  void fib_select_path(struct net *net, struct fib_result *res,
> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
> index 44668c2..87b8fdc 100644
> --- a/include/net/netns/ipv4.h
> +++ b/include/net/netns/ipv4.h
> @@ -52,6 +52,7 @@ struct netns_ipv4 {
>  #ifdef CONFIG_IP_MULTIPLE_TABLES
>  	struct fib_rules_ops	*rules_ops;
>  	bool			fib_has_custom_rules;
> +	bool			fib_rules_require_fldissect;
>  	struct fib_table __rcu	*fib_main;
>  	struct fib_table __rcu	*fib_default;
>  #endif
> diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
> index 9d55c90..83aa786 100644
> --- a/net/ipv4/fib_rules.c
> +++ b/net/ipv4/fib_rules.c
> @@ -253,6 +253,11 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
>  	}
>  #endif
>  
> +	if (rule->ip_proto ||
> +	    fib_rule_port_range_valid(&rule->sport_range) ||
> +	    fib_rule_port_range_valid(&rule->dport_range))
> +		net->ipv4.fib_rules_require_fldissect = true;
> +
>  	rule4->src_len = frh->src_len;
>  	rule4->srcmask = inet_make_mask(rule4->src_len);
>  	rule4->dst_len = frh->dst_len;

What about using 'fib_rules_require_fldissect' to conditionally avoid
all the tests introduced by patch 2/5 ? Perhaps even using a static key
for that?

It would be great if the kernel would be able to clear this flag when
no more needed. I know it's not the current behaviour for other similar
flags, but I hope we can improve ;)

Both points above also apply to the ipv6 code path.

Thanks,

Paolo