* [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
@ 2014-12-22 16:03 Jesse Gross
2014-12-23 1:28 ` Tom Herbert
2014-12-24 4:52 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Jesse Gross @ 2014-12-22 16:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tom Herbert, Joe Stringer, Eric Dumazet
GSO isn't the only offload feature with restrictions that
potentially can't be expressed with the current features mechanism.
Checksum is another although it's a general issue that could in
theory apply to anything. Even if it may be possible to
implement these restrictions in other ways, it can result in
duplicate code or inefficient per-packet behavior.
This generalizes ndo_gso_check so that drivers can remove any
features that don't make sense for a given packet, similar to
netif_skb_features(). It also converts existing driver
restrictions to the new format, completing the work that was
done to support tunnel protocols since the issues apply to
checksums as well.
CC: Tom Herbert <therbert@google.com>
CC: Joe Stringer <joestringer@nicira.com>
CC: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jesse Gross <jesse@nicira.com>
Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 8 ++++---
drivers/net/ethernet/emulex/benet/be_main.c | 8 ++++---
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 10 +++++----
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 8 ++++---
include/linux/netdevice.h | 20 +++++++++--------
include/net/vxlan.h | 28 ++++++++++++++++++++----
net/core/dev.c | 28 ++++++++++++++++--------
7 files changed, 75 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 9f5e387..72eef9f 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12553,9 +12553,11 @@ static int bnx2x_get_phys_port_id(struct net_device *netdev,
return 0;
}
-static bool bnx2x_gso_check(struct sk_buff *skb, struct net_device *dev)
+static netdev_features_t bnx2x_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
{
- return vxlan_gso_check(skb);
+ return vxlan_features_check(skb, features);
}
static const struct net_device_ops bnx2x_netdev_ops = {
@@ -12589,7 +12591,7 @@ static const struct net_device_ops bnx2x_netdev_ops = {
#endif
.ndo_get_phys_port_id = bnx2x_get_phys_port_id,
.ndo_set_vf_link_state = bnx2x_set_vf_link_state,
- .ndo_gso_check = bnx2x_gso_check,
+ .ndo_features_check = bnx2x_features_check,
};
static int bnx2x_set_coherency_mask(struct bnx2x *bp)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 1960731..41a0a54 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -4459,9 +4459,11 @@ done:
adapter->vxlan_port_count--;
}
-static bool be_gso_check(struct sk_buff *skb, struct net_device *dev)
+static netdev_features_t be_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
{
- return vxlan_gso_check(skb);
+ return vxlan_features_check(skb, features);
}
#endif
@@ -4492,7 +4494,7 @@ static const struct net_device_ops be_netdev_ops = {
#ifdef CONFIG_BE2NET_VXLAN
.ndo_add_vxlan_port = be_add_vxlan_port,
.ndo_del_vxlan_port = be_del_vxlan_port,
- .ndo_gso_check = be_gso_check,
+ .ndo_features_check = be_features_check,
#endif
};
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 190cbd9..d0d6dc1 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2365,9 +2365,11 @@ static void mlx4_en_del_vxlan_port(struct net_device *dev,
queue_work(priv->mdev->workqueue, &priv->vxlan_del_task);
}
-static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
+static netdev_features_t mlx4_en_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
{
- return vxlan_gso_check(skb);
+ return vxlan_features_check(skb, features);
}
#endif
@@ -2400,7 +2402,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
#ifdef CONFIG_MLX4_EN_VXLAN
.ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
.ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
- .ndo_gso_check = mlx4_en_gso_check,
+ .ndo_features_check = mlx4_en_features_check,
#endif
};
@@ -2434,7 +2436,7 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
#ifdef CONFIG_MLX4_EN_VXLAN
.ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
.ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
- .ndo_gso_check = mlx4_en_gso_check,
+ .ndo_features_check = mlx4_en_features_check,
#endif
};
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 1aa25b1..9929b97 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -505,9 +505,11 @@ static void qlcnic_del_vxlan_port(struct net_device *netdev,
adapter->flags |= QLCNIC_DEL_VXLAN_PORT;
}
-static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device *dev)
+static netdev_features_t qlcnic_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
{
- return vxlan_gso_check(skb);
+ return vxlan_features_check(skb, features);
}
#endif
@@ -532,7 +534,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
#ifdef CONFIG_QLCNIC_VXLAN
.ndo_add_vxlan_port = qlcnic_add_vxlan_port,
.ndo_del_vxlan_port = qlcnic_del_vxlan_port,
- .ndo_gso_check = qlcnic_gso_check,
+ .ndo_features_check = qlcnic_features_check,
#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = qlcnic_poll_controller,
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c31f74d..679e6e9 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1012,12 +1012,15 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
* Callback to use for xmit over the accelerated station. This
* is used in place of ndo_start_xmit on accelerated net
* devices.
- * bool (*ndo_gso_check) (struct sk_buff *skb,
- * struct net_device *dev);
+ * netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
+ * struct net_device *dev
+ * netdev_features_t features);
* Called by core transmit path to determine if device is capable of
- * performing GSO on a packet. The device returns true if it is
- * able to GSO the packet, false otherwise. If the return value is
- * false the stack will do software GSO.
+ * performing offload operations on a given packet. This is to give
+ * the device an opportunity to implement any restrictions that cannot
+ * be otherwise expressed by feature flags. The check is called with
+ * the set of features that the stack has calculated and it returns
+ * those the driver believes to be appropriate.
*
* int (*ndo_switch_parent_id_get)(struct net_device *dev,
* struct netdev_phys_item_id *psid);
@@ -1178,8 +1181,9 @@ struct net_device_ops {
struct net_device *dev,
void *priv);
int (*ndo_get_lock_subclass)(struct net_device *dev);
- bool (*ndo_gso_check) (struct sk_buff *skb,
- struct net_device *dev);
+ netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features);
#ifdef CONFIG_NET_SWITCHDEV
int (*ndo_switch_parent_id_get)(struct net_device *dev,
struct netdev_phys_item_id *psid);
@@ -3611,8 +3615,6 @@ static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
netdev_features_t features)
{
return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
- (dev->netdev_ops->ndo_gso_check &&
- !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
(skb->ip_summed != CHECKSUM_UNNECESSARY)));
}
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 57cccd0..903461a 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -1,6 +1,9 @@
#ifndef __NET_VXLAN_H
#define __NET_VXLAN_H 1
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/if_vlan.h>
#include <linux/skbuff.h>
#include <linux/netdevice.h>
#include <linux/udp.h>
@@ -51,16 +54,33 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
__be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
__be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
-static inline bool vxlan_gso_check(struct sk_buff *skb)
+static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
+ netdev_features_t features)
{
- if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+ u8 l4_hdr = 0;
+
+ if (!skb->encapsulation)
+ return features;
+
+ switch (vlan_get_protocol(skb)) {
+ case htons(ETH_P_IP):
+ l4_hdr = ip_hdr(skb)->protocol;
+ break;
+ case htons(ETH_P_IPV6):
+ l4_hdr = ipv6_hdr(skb)->nexthdr;
+ break;
+ default:
+ return features;;
+ }
+
+ if ((l4_hdr == IPPROTO_UDP) &&
(skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
skb->inner_protocol != htons(ETH_P_TEB) ||
(skb_inner_mac_header(skb) - skb_transport_header(skb) !=
sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
- return false;
+ return features & ~(NETIF_F_ALL_CSUM | NETIF_F_GSO_MASK);
- return true;
+ return features;
}
/* IP header + UDP + VXLAN + Ethernet header */
diff --git a/net/core/dev.c b/net/core/dev.c
index f411c28..fc13f72 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2562,7 +2562,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
netdev_features_t netif_skb_features(struct sk_buff *skb)
{
- const struct net_device *dev = skb->dev;
+ struct net_device *dev = skb->dev;
netdev_features_t features = dev->features;
u16 gso_segs = skb_shinfo(skb)->gso_segs;
__be16 protocol = skb->protocol;
@@ -2570,11 +2570,19 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
features &= ~NETIF_F_GSO_MASK;
+ /* If encapsulation offload request, verify we are testing
+ * hardware encapsulation features instead of standard
+ * features for the netdev
+ */
+ if (skb->encapsulation)
+ features = netdev_intersect_features(features,
+ dev->hw_enc_features);
+
if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD)) {
struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
protocol = veh->h_vlan_encapsulated_proto;
} else if (!vlan_tx_tag_present(skb)) {
- return harmonize_features(skb, features);
+ goto finalize;
}
features = netdev_intersect_features(features,
@@ -2591,6 +2599,15 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
NETIF_F_HW_VLAN_CTAG_TX |
NETIF_F_HW_VLAN_STAG_TX);
+finalize:
+ if (dev->netdev_ops->ndo_features_check) {
+ netdev_features_t dev_features;
+
+ dev_features = dev->netdev_ops->ndo_features_check(skb, dev,
+ features);
+ features = netdev_intersect_features(features, dev_features);
+ }
+
return harmonize_features(skb, features);
}
EXPORT_SYMBOL(netif_skb_features);
@@ -2661,13 +2678,6 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
if (unlikely(!skb))
goto out_null;
- /* If encapsulation offload request, verify we are testing
- * hardware encapsulation features instead of standard
- * features for the netdev
- */
- if (skb->encapsulation)
- features &= dev->hw_enc_features;
-
if (netif_needs_gso(dev, skb, features)) {
struct sk_buff *segs;
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
2014-12-22 16:03 [PATCH net] net: Generalize ndo_gso_check to ndo_features_check Jesse Gross
@ 2014-12-23 1:28 ` Tom Herbert
2014-12-23 6:24 ` Sathya Perla
2014-12-24 4:52 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Tom Herbert @ 2014-12-23 1:28 UTC (permalink / raw)
To: Jesse Gross; +Cc: David Miller, Linux Netdev List, Joe Stringer, Eric Dumazet
On Mon, Dec 22, 2014 at 8:03 AM, Jesse Gross <jesse@nicira.com> wrote:
> GSO isn't the only offload feature with restrictions that
> potentially can't be expressed with the current features mechanism.
> Checksum is another although it's a general issue that could in
> theory apply to anything. Even if it may be possible to
> implement these restrictions in other ways, it can result in
> duplicate code or inefficient per-packet behavior.
>
> This generalizes ndo_gso_check so that drivers can remove any
> features that don't make sense for a given packet, similar to
> netif_skb_features(). It also converts existing driver
> restrictions to the new format, completing the work that was
> done to support tunnel protocols since the issues apply to
> checksums as well.
>
It's a nice feature, but I really hope that this is not used for
checksums. We already have a sufficiently general interface for that
and checksum is already computed in drivers to work around HW bugs.
Acked-by: Tom Herbert <therbert@google.com>
> CC: Tom Herbert <therbert@google.com>
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 8 ++++---
> drivers/net/ethernet/emulex/benet/be_main.c | 8 ++++---
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 10 +++++----
> drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 8 ++++---
> include/linux/netdevice.h | 20 +++++++++--------
> include/net/vxlan.h | 28 ++++++++++++++++++++----
> net/core/dev.c | 28 ++++++++++++++++--------
> 7 files changed, 75 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 9f5e387..72eef9f 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12553,9 +12553,11 @@ static int bnx2x_get_phys_port_id(struct net_device *netdev,
> return 0;
> }
>
> -static bool bnx2x_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t bnx2x_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
>
> static const struct net_device_ops bnx2x_netdev_ops = {
> @@ -12589,7 +12591,7 @@ static const struct net_device_ops bnx2x_netdev_ops = {
> #endif
> .ndo_get_phys_port_id = bnx2x_get_phys_port_id,
> .ndo_set_vf_link_state = bnx2x_set_vf_link_state,
> - .ndo_gso_check = bnx2x_gso_check,
> + .ndo_features_check = bnx2x_features_check,
> };
>
> static int bnx2x_set_coherency_mask(struct bnx2x *bp)
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 1960731..41a0a54 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -4459,9 +4459,11 @@ done:
> adapter->vxlan_port_count--;
> }
>
> -static bool be_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t be_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -4492,7 +4494,7 @@ static const struct net_device_ops be_netdev_ops = {
> #ifdef CONFIG_BE2NET_VXLAN
> .ndo_add_vxlan_port = be_add_vxlan_port,
> .ndo_del_vxlan_port = be_del_vxlan_port,
> - .ndo_gso_check = be_gso_check,
> + .ndo_features_check = be_features_check,
> #endif
> };
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 190cbd9..d0d6dc1 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2365,9 +2365,11 @@ static void mlx4_en_del_vxlan_port(struct net_device *dev,
> queue_work(priv->mdev->workqueue, &priv->vxlan_del_task);
> }
>
> -static bool mlx4_en_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t mlx4_en_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -2400,7 +2402,7 @@ static const struct net_device_ops mlx4_netdev_ops = {
> #ifdef CONFIG_MLX4_EN_VXLAN
> .ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
> .ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
> - .ndo_gso_check = mlx4_en_gso_check,
> + .ndo_features_check = mlx4_en_features_check,
> #endif
> };
>
> @@ -2434,7 +2436,7 @@ static const struct net_device_ops mlx4_netdev_ops_master = {
> #ifdef CONFIG_MLX4_EN_VXLAN
> .ndo_add_vxlan_port = mlx4_en_add_vxlan_port,
> .ndo_del_vxlan_port = mlx4_en_del_vxlan_port,
> - .ndo_gso_check = mlx4_en_gso_check,
> + .ndo_features_check = mlx4_en_features_check,
> #endif
> };
>
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> index 1aa25b1..9929b97 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
> @@ -505,9 +505,11 @@ static void qlcnic_del_vxlan_port(struct net_device *netdev,
> adapter->flags |= QLCNIC_DEL_VXLAN_PORT;
> }
>
> -static bool qlcnic_gso_check(struct sk_buff *skb, struct net_device *dev)
> +static netdev_features_t qlcnic_features_check(struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features)
> {
> - return vxlan_gso_check(skb);
> + return vxlan_features_check(skb, features);
> }
> #endif
>
> @@ -532,7 +534,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
> #ifdef CONFIG_QLCNIC_VXLAN
> .ndo_add_vxlan_port = qlcnic_add_vxlan_port,
> .ndo_del_vxlan_port = qlcnic_del_vxlan_port,
> - .ndo_gso_check = qlcnic_gso_check,
> + .ndo_features_check = qlcnic_features_check,
> #endif
> #ifdef CONFIG_NET_POLL_CONTROLLER
> .ndo_poll_controller = qlcnic_poll_controller,
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c31f74d..679e6e9 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1012,12 +1012,15 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * Callback to use for xmit over the accelerated station. This
> * is used in place of ndo_start_xmit on accelerated net
> * devices.
> - * bool (*ndo_gso_check) (struct sk_buff *skb,
> - * struct net_device *dev);
> + * netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
> + * struct net_device *dev
> + * netdev_features_t features);
> * Called by core transmit path to determine if device is capable of
> - * performing GSO on a packet. The device returns true if it is
> - * able to GSO the packet, false otherwise. If the return value is
> - * false the stack will do software GSO.
> + * performing offload operations on a given packet. This is to give
> + * the device an opportunity to implement any restrictions that cannot
> + * be otherwise expressed by feature flags. The check is called with
> + * the set of features that the stack has calculated and it returns
> + * those the driver believes to be appropriate.
> *
> * int (*ndo_switch_parent_id_get)(struct net_device *dev,
> * struct netdev_phys_item_id *psid);
> @@ -1178,8 +1181,9 @@ struct net_device_ops {
> struct net_device *dev,
> void *priv);
> int (*ndo_get_lock_subclass)(struct net_device *dev);
> - bool (*ndo_gso_check) (struct sk_buff *skb,
> - struct net_device *dev);
> + netdev_features_t (*ndo_features_check) (struct sk_buff *skb,
> + struct net_device *dev,
> + netdev_features_t features);
> #ifdef CONFIG_NET_SWITCHDEV
> int (*ndo_switch_parent_id_get)(struct net_device *dev,
> struct netdev_phys_item_id *psid);
> @@ -3611,8 +3615,6 @@ static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
> netdev_features_t features)
> {
> return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
> - (dev->netdev_ops->ndo_gso_check &&
> - !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
> unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
> (skb->ip_summed != CHECKSUM_UNNECESSARY)));
> }
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 57cccd0..903461a 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -1,6 +1,9 @@
> #ifndef __NET_VXLAN_H
> #define __NET_VXLAN_H 1
>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/if_vlan.h>
> #include <linux/skbuff.h>
> #include <linux/netdevice.h>
> #include <linux/udp.h>
> @@ -51,16 +54,33 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
> __be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
> __be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
>
> -static inline bool vxlan_gso_check(struct sk_buff *skb)
> +static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
> + netdev_features_t features)
> {
> - if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> + u8 l4_hdr = 0;
> +
> + if (!skb->encapsulation)
> + return features;
> +
> + switch (vlan_get_protocol(skb)) {
> + case htons(ETH_P_IP):
> + l4_hdr = ip_hdr(skb)->protocol;
> + break;
> + case htons(ETH_P_IPV6):
> + l4_hdr = ipv6_hdr(skb)->nexthdr;
> + break;
> + default:
> + return features;;
> + }
> +
> + if ((l4_hdr == IPPROTO_UDP) &&
> (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> skb->inner_protocol != htons(ETH_P_TEB) ||
> (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
> sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
> - return false;
> + return features & ~(NETIF_F_ALL_CSUM | NETIF_F_GSO_MASK);
>
> - return true;
> + return features;
> }
>
> /* IP header + UDP + VXLAN + Ethernet header */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f411c28..fc13f72 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2562,7 +2562,7 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
>
> netdev_features_t netif_skb_features(struct sk_buff *skb)
> {
> - const struct net_device *dev = skb->dev;
> + struct net_device *dev = skb->dev;
> netdev_features_t features = dev->features;
> u16 gso_segs = skb_shinfo(skb)->gso_segs;
> __be16 protocol = skb->protocol;
> @@ -2570,11 +2570,19 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
> if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
> features &= ~NETIF_F_GSO_MASK;
>
> + /* If encapsulation offload request, verify we are testing
> + * hardware encapsulation features instead of standard
> + * features for the netdev
> + */
> + if (skb->encapsulation)
> + features = netdev_intersect_features(features,
> + dev->hw_enc_features);
> +
> if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD)) {
> struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
> protocol = veh->h_vlan_encapsulated_proto;
> } else if (!vlan_tx_tag_present(skb)) {
> - return harmonize_features(skb, features);
> + goto finalize;
> }
>
> features = netdev_intersect_features(features,
> @@ -2591,6 +2599,15 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
> NETIF_F_HW_VLAN_CTAG_TX |
> NETIF_F_HW_VLAN_STAG_TX);
>
> +finalize:
> + if (dev->netdev_ops->ndo_features_check) {
> + netdev_features_t dev_features;
> +
> + dev_features = dev->netdev_ops->ndo_features_check(skb, dev,
> + features);
> + features = netdev_intersect_features(features, dev_features);
> + }
> +
> return harmonize_features(skb, features);
> }
> EXPORT_SYMBOL(netif_skb_features);
> @@ -2661,13 +2678,6 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
> if (unlikely(!skb))
> goto out_null;
>
> - /* If encapsulation offload request, verify we are testing
> - * hardware encapsulation features instead of standard
> - * features for the netdev
> - */
> - if (skb->encapsulation)
> - features &= dev->hw_enc_features;
> -
> if (netif_needs_gso(dev, skb, features)) {
> struct sk_buff *segs;
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
2014-12-23 1:28 ` Tom Herbert
@ 2014-12-23 6:24 ` Sathya Perla
2014-12-23 21:54 ` Tom Herbert
0 siblings, 1 reply; 6+ messages in thread
From: Sathya Perla @ 2014-12-23 6:24 UTC (permalink / raw)
To: Tom Herbert, Jesse Gross
Cc: David Miller, Linux Netdev List, Joe Stringer, Eric Dumazet
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Tom Herbert
>
> On Mon, Dec 22, 2014 at 8:03 AM, Jesse Gross <jesse@nicira.com> wrote:
> > GSO isn't the only offload feature with restrictions that
> > potentially can't be expressed with the current features mechanism.
> > Checksum is another although it's a general issue that could in
> > theory apply to anything. Even if it may be possible to
> > implement these restrictions in other ways, it can result in
> > duplicate code or inefficient per-packet behavior.
> >
> > This generalizes ndo_gso_check so that drivers can remove any
> > features that don't make sense for a given packet, similar to
> > netif_skb_features(). It also converts existing driver
> > restrictions to the new format, completing the work that was
> > done to support tunnel protocols since the issues apply to
> > checksums as well.
> >
> It's a nice feature, but I really hope that this is not used for
> checksums. We already have a sufficiently general interface for that
> and checksum is already computed in drivers to work around HW bugs.
>
The ndo_featureas_check() interface that includes a means to report
inability to compute inner checksums on some tunnel types, seems like
a useful feature. The Skyhawk-R NIC can support inner csum offload for
either vxlan or nv-gre, but not both simultaneously. So, this ndo_
is useful in reporting this situation.
This would also obviate the need to have extra code in the drivers to
compute csums in the above scenario.
thanks,
-Sathya
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
2014-12-23 6:24 ` Sathya Perla
@ 2014-12-23 21:54 ` Tom Herbert
0 siblings, 0 replies; 6+ messages in thread
From: Tom Herbert @ 2014-12-23 21:54 UTC (permalink / raw)
To: Sathya Perla
Cc: Jesse Gross, David Miller, Linux Netdev List, Joe Stringer,
Eric Dumazet
On Mon, Dec 22, 2014 at 10:24 PM, Sathya Perla <Sathya.Perla@emulex.com> wrote:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Tom Herbert
>>
>> On Mon, Dec 22, 2014 at 8:03 AM, Jesse Gross <jesse@nicira.com> wrote:
>> > GSO isn't the only offload feature with restrictions that
>> > potentially can't be expressed with the current features mechanism.
>> > Checksum is another although it's a general issue that could in
>> > theory apply to anything. Even if it may be possible to
>> > implement these restrictions in other ways, it can result in
>> > duplicate code or inefficient per-packet behavior.
>> >
>> > This generalizes ndo_gso_check so that drivers can remove any
>> > features that don't make sense for a given packet, similar to
>> > netif_skb_features(). It also converts existing driver
>> > restrictions to the new format, completing the work that was
>> > done to support tunnel protocols since the issues apply to
>> > checksums as well.
>> >
>> It's a nice feature, but I really hope that this is not used for
>> checksums. We already have a sufficiently general interface for that
>> and checksum is already computed in drivers to work around HW bugs.
>>
> The ndo_featureas_check() interface that includes a means to report
> inability to compute inner checksums on some tunnel types, seems like
> a useful feature. The Skyhawk-R NIC can support inner csum offload for
> either vxlan or nv-gre, but not both simultaneously. So, this ndo_
> is useful in reporting this situation.
> This would also obviate the need to have extra code in the drivers to
> compute csums in the above scenario.
>
You could use it for that, but many drivers already call
skb_checksum_help and I really doubt it makes sense to change all of
those. Besides that, I don't think we should be "encouraging" vendors
to continue developing NICs that do protocol specific checksums. The
general interface (NETIF_HW_CSUM) is incredibly simple and obviates a
whole bunch of complexity in drivers to figure out whether it can
checksum a packet.
Tom
> thanks,
> -Sathya
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
2014-12-22 16:03 [PATCH net] net: Generalize ndo_gso_check to ndo_features_check Jesse Gross
2014-12-23 1:28 ` Tom Herbert
@ 2014-12-24 4:52 ` David Miller
2014-12-24 5:11 ` Jesse Gross
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2014-12-24 4:52 UTC (permalink / raw)
To: jesse; +Cc: netdev, therbert, joestringer, edumazet
From: Jesse Gross <jesse@nicira.com>
Date: Mon, 22 Dec 2014 08:03:43 -0800
> GSO isn't the only offload feature with restrictions that
> potentially can't be expressed with the current features mechanism.
> Checksum is another although it's a general issue that could in
> theory apply to anything. Even if it may be possible to
> implement these restrictions in other ways, it can result in
> duplicate code or inefficient per-packet behavior.
>
> This generalizes ndo_gso_check so that drivers can remove any
> features that don't make sense for a given packet, similar to
> netif_skb_features(). It also converts existing driver
> restrictions to the new format, completing the work that was
> done to support tunnel protocols since the issues apply to
> checksums as well.
>
> CC: Tom Herbert <therbert@google.com>
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
I don't think this fixes the case which was the main impetus for Eric
Dumazet's patch.
The r8152 USB networking driver supports TSO, but has a length
restriction, and we weren't software segmenting when netif_needs_gso()
returns true, exactly because we didn't clear TSO from the feature
flags.
We really need to sort this out.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: Generalize ndo_gso_check to ndo_features_check
2014-12-24 4:52 ` David Miller
@ 2014-12-24 5:11 ` Jesse Gross
0 siblings, 0 replies; 6+ messages in thread
From: Jesse Gross @ 2014-12-24 5:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tom Herbert, Joe Stringer, Eric Dumazet
On Tue, Dec 23, 2014 at 11:52 PM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Mon, 22 Dec 2014 08:03:43 -0800
>
>> GSO isn't the only offload feature with restrictions that
>> potentially can't be expressed with the current features mechanism.
>> Checksum is another although it's a general issue that could in
>> theory apply to anything. Even if it may be possible to
>> implement these restrictions in other ways, it can result in
>> duplicate code or inefficient per-packet behavior.
>>
>> This generalizes ndo_gso_check so that drivers can remove any
>> features that don't make sense for a given packet, similar to
>> netif_skb_features(). It also converts existing driver
>> restrictions to the new format, completing the work that was
>> done to support tunnel protocols since the issues apply to
>> checksums as well.
>>
>> CC: Tom Herbert <therbert@google.com>
>> CC: Joe Stringer <joestringer@nicira.com>
>> CC: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
>
> I don't think this fixes the case which was the main impetus for Eric
> Dumazet's patch.
>
> The r8152 USB networking driver supports TSO, but has a length
> restriction, and we weren't software segmenting when netif_needs_gso()
> returns true, exactly because we didn't clear TSO from the feature
> flags.
I believe that this should behave exactly the same as Eric's patch in
this case. The driver would implement the length validation and return
the set of features with ANDed with ~NETIF_F_GSO_MASK. This is
combined with the features computed by netif_skb_features() used for
all future offload decisions, including skb_gso_segment().
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-24 5:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-22 16:03 [PATCH net] net: Generalize ndo_gso_check to ndo_features_check Jesse Gross
2014-12-23 1:28 ` Tom Herbert
2014-12-23 6:24 ` Sathya Perla
2014-12-23 21:54 ` Tom Herbert
2014-12-24 4:52 ` David Miller
2014-12-24 5:11 ` Jesse Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox