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

[Pavel Tikhomirov]

On 27.10.2021 16:25, Cyrill Gorcunov wrote:
> Very good job, Pasha! I've just a few comments but I really like
> the current result.

Thanks!

> 
> 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?

It was never there, this hunk is a clean revert of

12b9b0869be8 ("drivers/net/ve: venet network device introduced")
diff --git a/include/linux/netdev_features.h 
b/include/linux/netdev_features.h

index 1d15a4688b71..fbc8d5dd9799 100644

--- a/include/linux/netdev_features.h

+++ b/include/linux/netdev_features.h

@@ -84,6 +84,9 @@ enum {

         NETIF_F_GRO_FRAGLIST_BIT,       /* Fraglist GRO */



         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 */



         NETIF_F_HW_HSR_TAG_INS_BIT,     /* Offload HSR tag insertion */

         NETIF_F_HW_HSR_TAG_RM_BIT,      /* Offload HSR tag removal */

> 
>> 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;

Wow, your construction is wrong =)

my: if (!A && !B)
yours: if (!A || !B)

But I get what you say, and I think "you way" if its don right looks not 
much better:

if (!B)
#ifdef CONFIG_VE
	if (!A)
#endif
		goto out;

> 
> 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?

Yes it's expected: we don't change net_device->features anymore so we 
can remove all ugly workarounds we had to make it work with features 
iteration logic.

> 
>> 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)

Same as with previous hunk =) I would rather leave it like this

> 
>> +++ 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;

I don't think it's strictly required, but ok.

> 
>> @@ -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?

It's already surrounded there. Look into nf_nat_redirect_ipv4.

> 
> 
> 	Cyrill
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.