Re: [PATCH nf-next 3/4] net: pass dst via net_device_path in dev_fill_forward_path()

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]

> Add dst_entry to tunnel device path, this will allow us to remove
> a duplicated route lookup.
> 
> This is a preparation patch to retrieve the tunnel route directly
> from the .fill_forward_path. This new dst_entry in the tunnel will be
> used by a follow up patch.
> 
> Since dst_release() works fine on NULL interface, this is still
> noop until the flowtable starts using this.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

just few nits inline. Fixing them:

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  include/linux/netdevice.h |  1 +
>  net/core/dev.c            | 45 +++++++++++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8db25b79573e..30dc78b4f1a3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -902,6 +902,7 @@ struct net_device_path {
>  			};
>  
>  			u8	l3_proto;
> +			struct dst_entry *dst;

nit: I guess if you move dst_entry pointer at the beginning of the tun struct
we avoid 7 bytes compiler padding.

>  		} tun;
>  		struct {
>  			enum {
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c1c1be1a6962..fed101f54a8a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -750,6 +750,23 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>  	return &stack->path[k];
>  }
>  
> +static void dev_fill_forward_path_release(struct net_device_path_stack *stack)
> +{
> +	struct net_device_path *path;
> +	int k;
> +
> +	for (k = stack->num_paths; k >= 0; k--) {
> +		path = &stack->path[k];
> +		switch (path->type) {
> +		case DEV_PATH_TUN:
> +			dst_release(path->tun.dst);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>  			  struct net_device_path_stack *stack)
>  {
> @@ -765,28 +782,38 @@ int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>  	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
>  		last_dev = ctx.dev;
>  		path = dev_fwd_path(stack);
> -		if (!path)
> -			return -1;
> +		if (!path) {
> +			ret = -1;

nit: I guess we can drop ret variable now since we do not use it in the return
path.

> +			goto err_out;
> +		}
>  
>  		memset(path, 0, sizeof(struct net_device_path));
>  		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
>  		if (ret < 0)
> -			return -1;
> +			goto err_out;
>  
> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
> -			return -1;
> +		if (WARN_ON_ONCE(last_dev == ctx.dev)) {
> +			ret = -1;
> +			goto err_out;
> +		}
>  	}
>  
>  	if (!ctx.dev)
> -		return ret;
> +		goto err_out;
>  
>  	path = dev_fwd_path(stack);
> -	if (!path)
> -		return -1;
> +	if (!path) {
> +		ret = -1;
> +		goto err_out;
> +	}
>  	path->type = DEV_PATH_ETHERNET;
>  	path->dev = ctx.dev;
>  
> -	return ret;
> +	return 0;
> +err_out:
> +	dev_fill_forward_path_release(stack);
> +
> +	return -1;
>  }
>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>  
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]