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