Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Fri, Nov 21, 2014 at 06:46:52PM +, Ben Hutchings wrote: On Wed, 2014-11-19 at 11:14 +0200, Michael S. Tsirkin wrote: On Thu, Oct 30, 2014 at 06:27:12PM +, Ben Hutchings wrote: IPv6 does not allow fragmentation by routers, so there is no fragmentation ID in the fixed header. UFO for IPv6 requires the ID to be passed separately, but there is no provision for this in the virtio net protocol. Until recently our software implementation of UFO/IPv6 generated a new ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet passed through a tap, which is even worse. Unfortunately there is no distinction between UFO/IPv4 and v6 features, so disable UFO on taps and virtio_net completely until we have a proper solution. We cannot depend on VM managers respecting the tap feature flags, so keep accepting UFO packets but log a warning the first time we do this. Signed-off-by: Ben Hutchings b...@decadent.org.uk Fixes: 916e4cf46d02 (ipv6: reuse ip6_frag_id from ip6_ufo_append_data) There's something I don't understand here. I see: NETIF_F_UFO_BIT,/* ... UDPv4 fragmentation */ this comment is wrong then? Yes. The patches drastically regress performance for UDPv4 for VMs only, but isn't it likely many other devices based their code on this comment? There's only one hardware driver that implements UFO (s2io), and it does handle IPv6. How about we disable UFO for IPv6 globally, and put the flag back in? We can then gradually add NETIF_F_UFO6_BIT for devices that actually support UFO for IPv6. Since the corresponding virtio feature bit is understood to include UFO/IPv6, and existing VMs rely on that, I don't see what this solves. Ben. I'm confused. Patching virtio has 0 effect on existing VMs - they are running old drivers anyway. Here's the proposal for guest side: - Add NETIF_F_UFO6_BIT, set in s2io. - Teach IPv6 to check NETIF_F_UFO6_BIT and not NETIF_F_UFO_BIT. What is accomplishes is good speed for virtio with UDP over IPv4, and correct, slower transmission for IPv6. Of course this does not help old guests but your patch to which I'm replying doesn't affect old guests either. Or did I miss something? -- Ben Hutchings Beware of bugs in the above code; I have only proved it correct, not tried it. - Donald Knuth ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Wed, 2014-11-19 at 11:14 +0200, Michael S. Tsirkin wrote: On Thu, Oct 30, 2014 at 06:27:12PM +, Ben Hutchings wrote: IPv6 does not allow fragmentation by routers, so there is no fragmentation ID in the fixed header. UFO for IPv6 requires the ID to be passed separately, but there is no provision for this in the virtio net protocol. Until recently our software implementation of UFO/IPv6 generated a new ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet passed through a tap, which is even worse. Unfortunately there is no distinction between UFO/IPv4 and v6 features, so disable UFO on taps and virtio_net completely until we have a proper solution. We cannot depend on VM managers respecting the tap feature flags, so keep accepting UFO packets but log a warning the first time we do this. Signed-off-by: Ben Hutchings b...@decadent.org.uk Fixes: 916e4cf46d02 (ipv6: reuse ip6_frag_id from ip6_ufo_append_data) There's something I don't understand here. I see: NETIF_F_UFO_BIT,/* ... UDPv4 fragmentation */ this comment is wrong then? Yes. The patches drastically regress performance for UDPv4 for VMs only, but isn't it likely many other devices based their code on this comment? There's only one hardware driver that implements UFO (s2io), and it does handle IPv6. How about we disable UFO for IPv6 globally, and put the flag back in? We can then gradually add NETIF_F_UFO6_BIT for devices that actually support UFO for IPv6. Since the corresponding virtio feature bit is understood to include UFO/IPv6, and existing VMs rely on that, I don't see what this solves. Ben. -- Ben Hutchings Beware of bugs in the above code; I have only proved it correct, not tried it. - Donald Knuth signature.asc Description: This is a digitally signed message part ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On 11/19/2014 05:14 PM, Michael S. Tsirkin wrote: On Thu, Oct 30, 2014 at 06:27:12PM +, Ben Hutchings wrote: IPv6 does not allow fragmentation by routers, so there is no fragmentation ID in the fixed header. UFO for IPv6 requires the ID to be passed separately, but there is no provision for this in the virtio net protocol. Until recently our software implementation of UFO/IPv6 generated a new ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet passed through a tap, which is even worse. Unfortunately there is no distinction between UFO/IPv4 and v6 features, so disable UFO on taps and virtio_net completely until we have a proper solution. We cannot depend on VM managers respecting the tap feature flags, so keep accepting UFO packets but log a warning the first time we do this. Signed-off-by: Ben Hutchings b...@decadent.org.uk Fixes: 916e4cf46d02 (ipv6: reuse ip6_frag_id from ip6_ufo_append_data) There's something I don't understand here. I see: NETIF_F_UFO_BIT,/* ... UDPv4 fragmentation */ this comment is wrong then? Looks wrong, at least ufo6 depends check this bit in ip6_output.c ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Thu, Oct 30, 2014 at 06:27:12PM +, Ben Hutchings wrote:
IPv6 does not allow fragmentation by routers, so there is no
fragmentation ID in the fixed header. UFO for IPv6 requires the ID to
be passed separately, but there is no provision for this in the virtio
net protocol.
Until recently our software implementation of UFO/IPv6 generated a new
ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet
passed through a tap, which is even worse.
Unfortunately there is no distinction between UFO/IPv4 and v6
features, so disable UFO on taps and virtio_net completely until we
have a proper solution.
We cannot depend on VM managers respecting the tap feature flags, so
keep accepting UFO packets but log a warning the first time we do
this.
Signed-off-by: Ben Hutchings b...@decadent.org.uk
Fixes: 916e4cf46d02 (ipv6: reuse ip6_frag_id from ip6_ufo_append_data)
There's something I don't understand here. I see:
NETIF_F_UFO_BIT,/* ... UDPv4 fragmentation */
this comment is wrong then?
The patches drastically regress performance for UDPv4 for VMs only, but
isn't it likely many other devices based their code on this comment?
How about we disable UFO for IPv6 globally, and put the
flag back in?
We can then gradually add NETIF_F_UFO6_BIT for devices that
actually support UFO for IPv6.
Thoughts?
---
drivers/net/macvtap.c| 13 +
drivers/net/tun.c| 19 +++
drivers/net/virtio_net.c | 24 ++--
3 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 65e2892..2aeaa61 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
static const struct proto_ops macvtap_socket_ops;
#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
- NETIF_F_TSO6 | NETIF_F_UFO)
+ NETIF_F_TSO6)
#define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
#define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
@@ -569,6 +569,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ pr_warn_once(macvtap: %s: using disabled UFO feature;
please fix this program\n,
+ current-comm);
gso_type = SKB_GSO_UDP;
break;
default:
@@ -614,8 +616,6 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff
*skb,
vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo-gso_type SKB_GSO_TCPV6)
vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (sinfo-gso_type SKB_GSO_UDP)
- vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_UDP;
else
BUG();
if (sinfo-gso_type SKB_GSO_TCP_ECN)
@@ -950,9 +950,6 @@ static int set_offload(struct macvtap_queue *q, unsigned
long arg)
if (arg TUN_F_TSO6)
feature_mask |= NETIF_F_TSO6;
}
-
- if (arg TUN_F_UFO)
- feature_mask |= NETIF_F_UFO;
}
/* tun/tap driver inverts the usage for TSO offloads, where
@@ -963,7 +960,7 @@ static int set_offload(struct macvtap_queue *q, unsigned
long arg)
* When user space turns off TSO, we turn off GSO/LRO so that
* user-space will not receive TSO frames.
*/
- if (feature_mask (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
+ if (feature_mask (NETIF_F_TSO | NETIF_F_TSO6))
features |= RX_OFFLOADS;
else
features = ~RX_OFFLOADS;
@@ -1064,7 +1061,7 @@ static long macvtap_ioctl(struct file *file, unsigned
int cmd,
case TUNSETOFFLOAD:
/* let the user check for future flags */
if (arg ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
- TUN_F_TSO_ECN | TUN_F_UFO))
+ TUN_F_TSO_ECN))
return -EINVAL;
rtnl_lock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..280d3d2 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -174,7 +174,7 @@ struct tun_struct {
struct net_device *dev;
netdev_features_t set_features;
#define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6|NETIF_F_UFO)
+ NETIF_F_TSO6)
int vnet_hdr_sz;
int sndbuf;
@@ -1149,8 +1149,18 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
skb_shinfo(skb)-gso_type = SKB_GSO_TCPV6;
break;
[PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
IPv6 does not allow fragmentation by routers, so there is no
fragmentation ID in the fixed header. UFO for IPv6 requires the ID to
be passed separately, but there is no provision for this in the virtio
net protocol.
Until recently our software implementation of UFO/IPv6 generated a new
ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet
passed through a tap, which is even worse.
Unfortunately there is no distinction between UFO/IPv4 and v6
features, so disable UFO on taps and virtio_net completely until we
have a proper solution.
We cannot depend on VM managers respecting the tap feature flags, so
keep accepting UFO packets but log a warning the first time we do
this.
Signed-off-by: Ben Hutchings b...@decadent.org.uk
Fixes: 916e4cf46d02 (ipv6: reuse ip6_frag_id from ip6_ufo_append_data)
---
drivers/net/macvtap.c| 13 +
drivers/net/tun.c| 19 +++
drivers/net/virtio_net.c | 24 ++--
3 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 65e2892..2aeaa61 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
static const struct proto_ops macvtap_socket_ops;
#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
- NETIF_F_TSO6 | NETIF_F_UFO)
+ NETIF_F_TSO6)
#define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
#define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
@@ -569,6 +569,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ pr_warn_once(macvtap: %s: using disabled UFO feature;
please fix this program\n,
+current-comm);
gso_type = SKB_GSO_UDP;
break;
default:
@@ -614,8 +616,6 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff
*skb,
vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo-gso_type SKB_GSO_TCPV6)
vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (sinfo-gso_type SKB_GSO_UDP)
- vnet_hdr-gso_type = VIRTIO_NET_HDR_GSO_UDP;
else
BUG();
if (sinfo-gso_type SKB_GSO_TCP_ECN)
@@ -950,9 +950,6 @@ static int set_offload(struct macvtap_queue *q, unsigned
long arg)
if (arg TUN_F_TSO6)
feature_mask |= NETIF_F_TSO6;
}
-
- if (arg TUN_F_UFO)
- feature_mask |= NETIF_F_UFO;
}
/* tun/tap driver inverts the usage for TSO offloads, where
@@ -963,7 +960,7 @@ static int set_offload(struct macvtap_queue *q, unsigned
long arg)
* When user space turns off TSO, we turn off GSO/LRO so that
* user-space will not receive TSO frames.
*/
- if (feature_mask (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
+ if (feature_mask (NETIF_F_TSO | NETIF_F_TSO6))
features |= RX_OFFLOADS;
else
features = ~RX_OFFLOADS;
@@ -1064,7 +1061,7 @@ static long macvtap_ioctl(struct file *file, unsigned int
cmd,
case TUNSETOFFLOAD:
/* let the user check for future flags */
if (arg ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
- TUN_F_TSO_ECN | TUN_F_UFO))
+ TUN_F_TSO_ECN))
return -EINVAL;
rtnl_lock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..280d3d2 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -174,7 +174,7 @@ struct tun_struct {
struct net_device *dev;
netdev_features_t set_features;
#define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6|NETIF_F_UFO)
+ NETIF_F_TSO6)
int vnet_hdr_sz;
int sndbuf;
@@ -1149,8 +1149,18 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
skb_shinfo(skb)-gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ {
+ static bool warned;
+
+ if (!warned) {
+ warned = true;
+ netdev_warn(tun-dev,
+ %s: using disabled UFO feature;
please fix this program\n,
+ current-comm);
+ }
skb_shinfo(skb)-gso_type = SKB_GSO_UDP;
break;
+
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Thu, 2014-10-30 at 18:27 +, Ben Hutchings wrote:
+ {
+ static bool warned;
+
+ if (!warned) {
+ warned = true;
+ netdev_warn(tun-dev,
+ %s: using disabled UFO feature;
please fix this program\n,
+ current-comm);
+ }
It might be time to add netdev_warn_once() ;)
Alternatively, you could use
pr_warn_once(%s: using disabled UFO feature; please fix this
program\n,
tun-dev-name, current-comm);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Thu, 2014-10-30 at 11:47 -0700, Eric Dumazet wrote:
On Thu, 2014-10-30 at 18:27 +, Ben Hutchings wrote:
+ {
+ static bool warned;
+
+ if (!warned) {
+ warned = true;
+ netdev_warn(tun-dev,
+ %s: using disabled UFO feature;
please fix this program\n,
+ current-comm);
+ }
It might be time to add netdev_warn_once() ;)
Could do. I'm trying to make small fixes that are suitable for stable.
Alternatively, you could use
pr_warn_once(%s: using disabled UFO feature; please fix this
program\n,
tun-dev-name, current-comm);
That's missing a %s: , but yes that would also work.
Ben.
--
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.
signature.asc
Description: This is a digitally signed message part
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
On Thu, 2014-10-30 at 22:20 +, Ben Hutchings wrote: Could do. I'm trying to make small fixes that are suitable for stable. Oh right, makes sense ;) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
