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
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
