Date:   Wed, 28 Feb 2018 00:38:26 +0200
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Yuval Mintz <yuvalm@...lanox.com>, netdev@...r.kernel.org
Cc:     mlxsw@...lanox.com, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net-next 07/11] ipmr, ip6mr: Unite logic for searching in
 MFC cache

On 27/02/18 20:58, Yuval Mintz wrote:
> ipmr and ip6mr utilize the exact same methods for searching the
> hashed resolved connections, difference being only in the construction
> of the hash comparison key.
> 
> In order to unite the flow, introduce an mr_table operation set that
> would contain the protocol specific information required for common
> flows, in this case - the hash parameters and a comparison key
> representing a (*,*) route.
> 
> Signed-off-by: Yuval Mintz <yuvalm@...lanox.com>
> ---
>  include/linux/mroute_base.h | 52 +++++++++++++++++++++++++++++--
>  net/ipv4/ipmr.c             | 71 ++++++++++---------------------------------
>  net/ipv4/ipmr_base.c        | 54 +++++++++++++++++++++++++++++++--
>  net/ipv6/ip6mr.c            | 74 +++++++++++----------------------------------
>  4 files changed, 134 insertions(+), 117 deletions(-)
> 
> diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
> index 2769e2f..18a1d75 100644
> --- a/include/linux/mroute_base.h
> +++ b/include/linux/mroute_base.h
> @@ -89,10 +89,23 @@ struct mr_mfc {
>  	struct rcu_head	rcu;
>  };
>  
> +struct mr_table;
> +
> +/**
> + * struct mr_table_ops - callbacks and info for protocol-specific ops
> + * rht_params: parameters for accessing the MFC hash
> + * cmparg_any: a hash key to be used for matching on (*,*) routes

kernel doc style uses @variable

> + */
> +struct mr_table_ops {
> +	const struct rhashtable_params *rht_params;
> +	void *cmparg_any;
> +};
> +
>  /**
>   * struct mr_table - a multicast routing table
>   * @list: entry within a list of multicast routing tables
>   * @net: net where this table belongs
> + * @op: protocol specific operations

nit: s/op/ops/

>   * @id: identifier of the table
>   * @mroute_sk: socket associated with the table
>   * @ipmr_expire_timer: timer for handling unresolved routes
> @@ -109,6 +122,7 @@ struct mr_mfc {
>  struct mr_table {
>  	struct list_head	list;
>  	possible_net_t		net;
> +	struct mr_table_ops	ops;
>  	u32			id;
>  	struct sock __rcu	*mroute_sk;
>  	struct timer_list	ipmr_expire_timer;
> @@ -133,10 +147,19 @@ void vif_device_init(struct vif_device *v,
>  
>  struct mr_table *
>  mr_table_alloc(struct net *net, u32 id,
> -	       const struct rhashtable_params *rht_params,
> +	       struct mr_table_ops *ops,
>  	       void (*expire_func)(struct timer_list *t),
>  	       void (*table_set)(struct mr_table *mrt,
>  				 struct net *net));
> +
> +/* These actually return 'struct mr_mfc *', but to avoid need for explicit
> + * castings they simply return void.
> + */
> +void *mr_mfc_find_parent(struct mr_table *mrt,
> +			 void *hasharg, int parent);
> +void *mr_mfc_find_any_parent(struct mr_table *mrt, int vifi);
> +void *mr_mfc_find_any(struct mr_table *mrt, int vifi, void *hasharg);
> +
>  #else
>  static inline void vif_device_init(struct vif_device *v,
>  				   struct net_device *dev,
> @@ -147,14 +170,37 @@ static inline void vif_device_init(struct vif_device *v,
>  {
>  }
>  
> -static inline struct mr_table *
> +static inline void *
>  mr_table_alloc(struct net *net, u32 id,
> -	       const struct rhashtable_params *rht_params,
> +	       struct mr_table_ops *ops,
>  	       void (*expire_func)(struct timer_list *t),
>  	       void (*table_set)(struct mr_table *mrt,
>  				 struct net *net))
>  {
>  	return NULL;
>  }
> +
> +static inline void *mr_mfc_find_parent(struct mr_table *mrt,
> +				       void *hasharg, int parent)
> +{
> +	return NULL;
> +}
> +
> +static inline void *mr_mfc_find_any_parent(struct mr_table *mrt,
> +					   int vifi)
> +{
> +	return NULL;
> +}
> +
> +static inline struct mr_mfc *mr_mfc_find_any(struct mr_table *mrt,
> +					     int vifi, void *hasharg)
> +{
> +	return NULL;
> +}
>  #endif
> +
> +static inline void *mr_mfc_find(struct mr_table *mrt, void *hasharg)
> +{
> +	return mr_mfc_find_parent(mrt, hasharg, -1);
> +}
>  #endif
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 238a579..d0bf021 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -359,6 +359,16 @@ static void ipmr_new_table_set(struct mr_table *mrt,
>  #endif
>  }
>  
> +static struct mfc_cache_cmp_arg ipmr_mr_table_ops_cmparg_any = {
> +	.mfc_mcastgrp = htonl(INADDR_ANY),
> +	.mfc_origin = htonl(INADDR_ANY),
> +};
> +
> +static struct mr_table_ops ipmr_mr_table_ops = {
> +	.rht_params = &ipmr_rht_params,
> +	.cmparg_any = &ipmr_mr_table_ops_cmparg_any,
> +};
> +
>  static struct mr_table *ipmr_new_table(struct net *net, u32 id)
>  {
>  	struct mr_table *mrt;
> @@ -371,7 +381,7 @@ static struct mr_table *ipmr_new_table(struct net *net, u32 id)
>  	if (mrt)
>  		return mrt;
>  
> -	return mr_table_alloc(net, id, &ipmr_rht_params,
> +	return mr_table_alloc(net, id, &ipmr_mr_table_ops,
>  			      ipmr_expire_process, ipmr_new_table_set);
>  }
>  
> @@ -972,33 +982,8 @@ static struct mfc_cache *ipmr_cache_find(struct mr_table *mrt,
>  			.mfc_mcastgrp = mcastgrp,
>  			.mfc_origin = origin
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ipmr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		return (struct mfc_cache *)c;
> -
> -	return NULL;
> -}
> -
> -/* Look for a (*,*,oif) entry */
> -static struct mfc_cache *ipmr_cache_find_any_parent(struct mr_table *mrt,
> -						    int vifi)
> -{
> -	struct mfc_cache_cmp_arg arg = {
> -			.mfc_mcastgrp = htonl(INADDR_ANY),
> -			.mfc_origin = htonl(INADDR_ANY)
> -	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ipmr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		if (c->mfc_un.res.ttls[vifi] < 255)
> -			return (struct mfc_cache *)c;
>  
> -	return NULL;
> +	return mr_mfc_find(mrt, &arg);
>  }
>  
>  /* Look for a (*,G) entry */
> @@ -1009,27 +994,10 @@ static struct mfc_cache *ipmr_cache_find_any(struct mr_table *mrt,
>  			.mfc_mcastgrp = mcastgrp,
>  			.mfc_origin = htonl(INADDR_ANY)
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
>  
>  	if (mcastgrp == htonl(INADDR_ANY))
> -		goto skip;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ipmr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode) {
> -		struct mfc_cache *proxy;
> -
> -		if (c->mfc_un.res.ttls[vifi] < 255)
> -			return (struct mfc_cache *)c;
> -
> -		/* It's ok if the vifi is part of the static tree */
> -		proxy = ipmr_cache_find_any_parent(mrt, c->mfc_parent);
> -		if (proxy && proxy->_c.mfc_un.res.ttls[vifi] < 255)
> -			return (struct mfc_cache *)c;
> -	}
> -
> -skip:
> -	return ipmr_cache_find_any_parent(mrt, vifi);
> +		return mr_mfc_find_any_parent(mrt, vifi);
> +	return mr_mfc_find_any(mrt, vifi, &arg);
>  }
>  
>  /* Look for a (S,G,iif) entry if parent != -1 */
> @@ -1041,15 +1009,8 @@ static struct mfc_cache *ipmr_cache_find_parent(struct mr_table *mrt,
>  			.mfc_mcastgrp = mcastgrp,
>  			.mfc_origin = origin,
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
>  
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ipmr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		if (parent == -1 || parent == c->mfc_parent)
> -			return (struct mfc_cache *)c;
> -
> -	return NULL;
> +	return mr_mfc_find_parent(mrt, &arg, parent);
>  }
>  
>  /* Allocate a multicast cache entry */
> @@ -2010,7 +1971,7 @@ static void ip_mr_forward(struct net *net, struct mr_table *mrt,
>  		/* For an (*,G) entry, we only check that the incomming
>  		 * interface is part of the static tree.
>  		 */
> -		cache_proxy = ipmr_cache_find_any_parent(mrt, vif);
> +		cache_proxy = mr_mfc_find_any_parent(mrt, vif);
>  		if (cache_proxy &&
>  		    cache_proxy->_c.mfc_un.res.ttls[true_vifi] < 255)
>  			goto forward;
> diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
> index 3e21a58..172f92a 100644
> --- a/net/ipv4/ipmr_base.c
> +++ b/net/ipv4/ipmr_base.c
> @@ -29,7 +29,7 @@ EXPORT_SYMBOL(vif_device_init);
>  
>  struct mr_table *
>  mr_table_alloc(struct net *net, u32 id,
> -	       const struct rhashtable_params *rht_params,
> +	       struct mr_table_ops *ops,
>  	       void (*expire_func)(struct timer_list *t),
>  	       void (*table_set)(struct mr_table *mrt,
>  				 struct net *net))
> @@ -42,7 +42,8 @@ mr_table_alloc(struct net *net, u32 id,
>  	mrt->id = id;
>  	write_pnet(&mrt->net, net);
>  
> -	rhltable_init(&mrt->mfc_hash, rht_params);
> +	mrt->ops = *ops;
> +	rhltable_init(&mrt->mfc_hash, mrt->ops.rht_params);
>  	INIT_LIST_HEAD(&mrt->mfc_cache_list);
>  	INIT_LIST_HEAD(&mrt->mfc_unres_queue);
>  
> @@ -53,3 +54,52 @@ mr_table_alloc(struct net *net, u32 id,
>  	return mrt;
>  }
>  EXPORT_SYMBOL(mr_table_alloc);
> +
> +void *mr_mfc_find_parent(struct mr_table *mrt, void *hasharg, int parent)
> +{
> +	struct rhlist_head *tmp, *list;
> +	struct mr_mfc *c;
> +
> +	list = rhltable_lookup(&mrt->mfc_hash, hasharg, *mrt->ops.rht_params);
> +	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> +		if (parent == -1 || parent == c->mfc_parent)
> +			return c;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(mr_mfc_find_parent);
> +
> +void *mr_mfc_find_any_parent(struct mr_table *mrt, int vifi)
> +{
> +	struct rhlist_head *tmp, *list;
> +	struct mr_mfc *c;
> +
> +	list = rhltable_lookup(&mrt->mfc_hash, mrt->ops.cmparg_any,
> +			       *mrt->ops.rht_params);
> +	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> +		if (c->mfc_un.res.ttls[vifi] < 255)
> +			return c;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(mr_mfc_find_any_parent);
> +
> +void *mr_mfc_find_any(struct mr_table *mrt, int vifi, void *hasharg)
> +{
> +	struct rhlist_head *tmp, *list;
> +	struct mr_mfc *c, *proxy;
> +
> +	list = rhltable_lookup(&mrt->mfc_hash, hasharg, *mrt->ops.rht_params);
> +	rhl_for_each_entry_rcu(c, tmp, list, mnode) {
> +		if (c->mfc_un.res.ttls[vifi] < 255)
> +			return c;
> +
> +		/* It's ok if the vifi is part of the static tree */
> +		proxy = mr_mfc_find_any_parent(mrt, c->mfc_parent);
> +		if (proxy && proxy->mfc_un.res.ttls[vifi] < 255)
> +			return c;
> +	}
> +
> +	return mr_mfc_find_any_parent(mrt, vifi);
> +}
> +EXPORT_SYMBOL(mr_mfc_find_any);
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 4db4339..93a8c02 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -302,6 +302,16 @@ static void ip6mr_new_table_set(struct mr_table *mrt,
>  #endif
>  }
>  
> +static struct mfc6_cache_cmp_arg ip6mr_mr_table_ops_cmparg_any = {
> +	.mf6c_origin = IN6ADDR_ANY_INIT,
> +	.mf6c_mcastgrp = IN6ADDR_ANY_INIT,
> +};
> +
> +static struct mr_table_ops ip6mr_mr_table_ops = {
> +	.rht_params = &ip6mr_rht_params,
> +	.cmparg_any = &ip6mr_mr_table_ops_cmparg_any,
> +};
> +
>  static struct mr_table *ip6mr_new_table(struct net *net, u32 id)
>  {
>  	struct mr_table *mrt;
> @@ -310,7 +320,7 @@ static struct mr_table *ip6mr_new_table(struct net *net, u32 id)
>  	if (mrt)
>  		return mrt;
>  
> -	return mr_table_alloc(net, id, &ip6mr_rht_params,
> +	return mr_table_alloc(net, id, &ip6mr_mr_table_ops,
>  			      ipmr_expire_process, ip6mr_new_table_set);
>  }
>  
> @@ -988,33 +998,8 @@ static struct mfc6_cache *ip6mr_cache_find(struct mr_table *mrt,
>  		.mf6c_origin = *origin,
>  		.mf6c_mcastgrp = *mcastgrp,
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ip6mr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		return (struct mfc6_cache *)c;
>  
> -	return NULL;
> -}
> -
> -/* Look for a (*,*,oif) entry */
> -static struct mfc6_cache *ip6mr_cache_find_any_parent(struct mr_table *mrt,
> -						      mifi_t mifi)
> -{
> -	struct mfc6_cache_cmp_arg arg = {
> -		.mf6c_origin = in6addr_any,
> -		.mf6c_mcastgrp = in6addr_any,
> -	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ip6mr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		if (c->mfc_un.res.ttls[mifi] < 255)
> -			return (struct mfc6_cache *)c;
> -
> -	return NULL;
> +	return mr_mfc_find(mrt, &arg);
>  }
>  
>  /* Look for a (*,G) entry */
> @@ -1026,26 +1011,10 @@ static struct mfc6_cache *ip6mr_cache_find_any(struct mr_table *mrt,
>  		.mf6c_origin = in6addr_any,
>  		.mf6c_mcastgrp = *mcastgrp,
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -	struct mfc6_cache *proxy;
>  
>  	if (ipv6_addr_any(mcastgrp))
> -		goto skip;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ip6mr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode) {
> -		if (c->mfc_un.res.ttls[mifi] < 255)
> -			return (struct mfc6_cache *)c;
> -
> -		/* It's ok if the mifi is part of the static tree */
> -		proxy = ip6mr_cache_find_any_parent(mrt, c->mfc_parent);
> -		if (proxy && proxy->_c.mfc_un.res.ttls[mifi] < 255)
> -			return (struct mfc6_cache *)c;
> -	}
> -
> -skip:
> -	return ip6mr_cache_find_any_parent(mrt, mifi);
> +		return mr_mfc_find_any_parent(mrt, mifi);
> +	return mr_mfc_find_any(mrt, mifi, &arg);
>  }
>  
>  /* Look for a (S,G,iif) entry if parent != -1 */
> @@ -1059,20 +1028,11 @@ ip6mr_cache_find_parent(struct mr_table *mrt,
>  		.mf6c_origin = *origin,
>  		.mf6c_mcastgrp = *mcastgrp,
>  	};
> -	struct rhlist_head *tmp, *list;
> -	struct mr_mfc *c;
> -
> -	list = rhltable_lookup(&mrt->mfc_hash, &arg, ip6mr_rht_params);
> -	rhl_for_each_entry_rcu(c, tmp, list, mnode)
> -		if (parent == -1 || parent == c->mfc_parent)
> -			return (struct mfc6_cache *)c;
>  
> -	return NULL;
> +	return mr_mfc_find_parent(mrt, &arg, parent);
>  }
>  
> -/*
> - *	Allocate a multicast cache entry
> - */
> +/* Allocate a multicast cache entry */
>  static struct mfc6_cache *ip6mr_cache_alloc(void)
>  {
>  	struct mfc6_cache *c = kmem_cache_zalloc(mrt_cachep, GFP_KERNEL);
> @@ -2112,7 +2072,7 @@ static void ip6_mr_forward(struct net *net, struct mr_table *mrt,
>  		 * interface is part of the static tree.
>  		 */
>  		rcu_read_lock();
> -		cache_proxy = ip6mr_cache_find_any_parent(mrt, vif);
> +		cache_proxy = mr_mfc_find_any_parent(mrt, vif);
>  		if (cache_proxy &&
>  		    cache_proxy->_c.mfc_un.res.ttls[true_vifi] < 255) {
>  			rcu_read_unlock();
> 

Powered by blists - more mailing lists