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

[Cyrill Gorcunov]

On Wed, Oct 27, 2021 at 05:08:07PM +0300, Pavel Tikhomirov wrote:
> > > 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;
> 

Indeed, thanks!

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

Great!

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

Yes please, this makes the code more consistent with previous lines.

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

Great! Thanks!