[Devel] [PATCH RH9 2/3] ve/net/features: put per-ve netdev features to separate struct member

[Cyrill Gorcunov]

Very good job, Pasha! I've just a few comments but I really like
the current result.

On Wed, Oct 27, 2021 at 04:05:20PM +0300, Pavel Tikhomirov wrote:
>  	NETIF_F_GRO_UDP_FWD_BIT,	/* Allow UDP GRO for forwarding */
> -	/* here goes NETIF_F_HW_MACSEC_BIT in ms, temporarily reverted */
> -					/* Offload MACsec operations */
> -	NETIF_F_VENET_BIT,		/* Device is venet device */

Should not we bring NETIF_F_HW_MACSEC_BIT back here?

> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 2b1d004d6a1f..dfd0a989c4ab 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -33,7 +33,10 @@ static inline int should_deliver(const struct net_bridge_port *p,
>  int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
>  	skb_push(skb, ETH_HLEN);
> -	if (!(skb->dev->features & NETIF_F_VENET) &&
> +	if (
> +#ifdef CONFIG_VE
> +	    !(skb->dev->ve_features & NETIF_F_VENET) &&
> +#endif
>  	    !is_skb_forwardable(skb->dev, skb))
>  		goto drop;

Maybe instead of such ugly constructions we could do something like

#ifdef CONFIG_VE
	if (!(skb->dev->ve_features & NETIF_F_VENET))
		goto drop;
#endif

	if (!is_skb_forwardable(skb->dev, skb))
		goto drop;

after all the compiler will have to test both conditions so I don't think
we improve anything if merge them into one ugly if statement. What do you
think?

> @@ -11425,7 +11425,7 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
>  		mask |= NETIF_F_CSUM_MASK;
>  	mask |= NETIF_F_VLAN_CHALLENGED;
>  
> -	all |= one & (NETIF_F_ONE_FOR_ALL|NETIF_F_CSUM_MASK|NETIF_F_VIRTUAL) & mask;
> +	all |= one & (NETIF_F_ONE_FOR_ALL | NETIF_F_CSUM_MASK) & mask;
>  	all &= one | ~NETIF_F_ALL_FOR_ALL;

This looks somehow suspicious, we no longer consider virtual bit here.
Are you sure it is expected?

> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index bf4d8cdb9725..54964c5d3f1f 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -378,7 +378,10 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
>  	if (fib_lookup(net, &fl4, &res, 0))
>  		goto last_resort;
>  	if (res.type != RTN_UNICAST &&
> -	    (!(dev->features & NETIF_F_VENET) ||
> +	    (
> +#ifdef CONFIG_VE
> +	     !(dev->ve_features & NETIF_F_VENET) ||
> +#endif
>  	     res.type != RTN_LOCAL || !IN_DEV_ACCEPT_LOCAL(idev)))
>  		goto e_inval;

Meh :) But seems the only option)

> +++ b/net/ipv4/ipip.c
> @@ -384,7 +384,9 @@ static void ipip_tunnel_setup(struct net_device *dev)
>  	netif_keep_dst(dev);
>  
>  	dev->features		|= IPIP_FEATURES;
> -	dev->features		|= NETIF_F_VIRTUAL;
> +#ifdef CONFIG_VE
> +	dev->ve_features = NETIF_F_VIRTUAL;
> +#endif

Please align assignment to be similar with code around, ie

	dev->ve_features	 = NETIF_F_VIRTUAL;

> @@ -1445,7 +1445,9 @@ static void ipip6_tunnel_setup(struct net_device *dev)
>  	dev->addr_len		= 4;
>  	dev->features		|= NETIF_F_LLTX;
>  	dev->features		|= SIT_FEATURES;
> -	dev->features		|= NETIF_F_VIRTUAL;
> +#ifdef CONFIG_VE
> +	dev->ve_features = NETIF_F_VIRTUAL;
> +#endif

Same here.

> +++ b/net/netfilter/nf_nat_redirect.c
> @@ -60,7 +60,7 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
>  				 * should use first nonloopback ifa in
>  				 * the list.
>  				 */
> -				if (skb->dev->features & NETIF_F_VENET) {
> +				if (skb->dev->ve_features & NETIF_F_VENET) {

#ifdef CONFIG_VE above?


	Cyrill