Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin m...@redhat.com Date: Tue, 29 Jun 2010 16:04:39 +0300 Since using the module involves updating the management tools as well, if we go down this route it will be much less painful for everyone to do push it upstream. Ok, you can make your case to Patrick McHardy and if he'll merge it into his netfilter GIT tree I guess I'll have to take it :) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote: On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote: From: Michael S. Tsirkinm...@redhat.com Date: Mon, 28 Jun 2010 13:08:07 +0300 Userspace virtio server has the following hack so guests rely on it, and we have to replicate it, too: Use port number to detect incoming IPv4 DHCP response packets, and fill in the checksum for these. The issue we are solving is that on linux guests, some apps that use recvmsg with AF_PACKET sockets, don't know how to handle CHECKSUM_PARTIAL; The interface to return the relevant information was added in 8dc4194474159660d7f37c495e3fc3f10d0db8cc, and older userspace does not use it. One important user of recvmsg with AF_PACKET is dhclient, so we add a work-around just for DHCP. Don't bother applying the hack to IPv6 as userspace virtio does not have a work-around for that - let's hope guests will do the right thing wrt IPv6. Signed-off-by: Michael S. Tsirkinm...@redhat.com Yikes, this is awful too. Nothing in the kernel should be mucking around with procotol packets like this by default. In particular, what the heck does port 67 mean? Locally I can use it for whatever I want for my own purposes, I don't have to follow the conventions for service ports as specified by the IETF. But I can't have the packet checksum state be left alone for port 67 traffic on a box using virtio because you have this hack there. And yes it's broken on machines using the qemu thing, but at least the hack there is restricted to userspace. Yes, and I think it was a mistake to add the hack there. This is what prevented applications from using the new interface in the 3 years since it was first introduced. It's far more complicated than that. dhclient is part of ISC's DHCP package. They do not have a public SCM and instead require you to join their Software Guild to get access to it. This problem was identified in one distribution and the patch was pushed upstream but because they did not have a public SCM, most other distributions did not see the fix until it appeared in a release. ISC has a pretty long release cycle historically. ISC's had the fix for a long time but there was a 3-year gap in their releases and since their SCM isn't public, users are stuck with the last release. This hack makes sense in QEMU as we have a few hacks like this to fix broken guests. A primary use of virtualization is to run old applications so it makes sense for us to do that. I don't think it makes sense for vhost to do this. These guests are so old that they don't have the requisite features to achieve really high performance anyway. I've always thought making vhost totally transparent was a bad idea and this is one of the reasons. We can do a lot of ugly things in userspace that we shouldn't be doing in the kernel. Regards, Anthony Liguori ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
On Wed, Jun 30, 2010 at 05:08:11PM -0500, Anthony Liguori wrote: On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote: On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote: From: Michael S. Tsirkinm...@redhat.com Date: Mon, 28 Jun 2010 13:08:07 +0300 Userspace virtio server has the following hack so guests rely on it, and we have to replicate it, too: Use port number to detect incoming IPv4 DHCP response packets, and fill in the checksum for these. The issue we are solving is that on linux guests, some apps that use recvmsg with AF_PACKET sockets, don't know how to handle CHECKSUM_PARTIAL; The interface to return the relevant information was added in 8dc4194474159660d7f37c495e3fc3f10d0db8cc, and older userspace does not use it. One important user of recvmsg with AF_PACKET is dhclient, so we add a work-around just for DHCP. Don't bother applying the hack to IPv6 as userspace virtio does not have a work-around for that - let's hope guests will do the right thing wrt IPv6. Signed-off-by: Michael S. Tsirkinm...@redhat.com Yikes, this is awful too. Nothing in the kernel should be mucking around with procotol packets like this by default. In particular, what the heck does port 67 mean? Locally I can use it for whatever I want for my own purposes, I don't have to follow the conventions for service ports as specified by the IETF. But I can't have the packet checksum state be left alone for port 67 traffic on a box using virtio because you have this hack there. And yes it's broken on machines using the qemu thing, but at least the hack there is restricted to userspace. Yes, and I think it was a mistake to add the hack there. This is what prevented applications from using the new interface in the 3 years since it was first introduced. It's far more complicated than that. dhclient is part of ISC's DHCP package. They do not have a public SCM and instead require you to join their Software Guild to get access to it. This problem was identified in one distribution and the patch was pushed upstream but because they did not have a public SCM, most other distributions did not see the fix until it appeared in a release. ISC has a pretty long release cycle historically. ISC's had the fix for a long time but there was a 3-year gap in their releases and since their SCM isn't public, users are stuck with the last release. This hack makes sense in QEMU as we have a few hacks like this to fix broken guests. A primary use of virtualization is to run old applications so it makes sense for us to do that. IMO it was wrong to put it in qemu: originally, if a distro shipped a broken virtio/dhclient combo, it was it's own bug to fix. But now that qemu has shipped the work-around for so long, broken guests seemed work. So we *still* see the bug re-surface in new guests. And since they are fairly new, it is interesting to get decent performance from them now. I don't think it makes sense for vhost to do this. These guests are so old that they don't have the requisite features to achieve really high performance anyway. I've always thought making vhost totally transparent was a bad idea and this is one of the reasons. It does not have to be fully transparent. You can insert your own ring in the middle, and copy descriptors around. And we stop on errors and let userspace handle. This will come handy if we get e.g. virtio bug that we need to work around. We can do a lot of ugly things in userspace that we shouldn't be doing in the kernel. Regards, Anthony Liguori QEMU is only userspace for the host. It is the hardware for the guest. So IMO we should not be doing the ugly things there either. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
On 06/30/2010 05:31 PM, Michael S. Tsirkin wrote: On Wed, Jun 30, 2010 at 05:08:11PM -0500, Anthony Liguori wrote: On 06/29/2010 08:04 AM, Michael S. Tsirkin wrote: On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote: From: Michael S. Tsirkinm...@redhat.com Date: Mon, 28 Jun 2010 13:08:07 +0300 Userspace virtio server has the following hack so guests rely on it, and we have to replicate it, too: Use port number to detect incoming IPv4 DHCP response packets, and fill in the checksum for these. The issue we are solving is that on linux guests, some apps that use recvmsg with AF_PACKET sockets, don't know how to handle CHECKSUM_PARTIAL; The interface to return the relevant information was added in 8dc4194474159660d7f37c495e3fc3f10d0db8cc, and older userspace does not use it. One important user of recvmsg with AF_PACKET is dhclient, so we add a work-around just for DHCP. Don't bother applying the hack to IPv6 as userspace virtio does not have a work-around for that - let's hope guests will do the right thing wrt IPv6. Signed-off-by: Michael S. Tsirkinm...@redhat.com Yikes, this is awful too. Nothing in the kernel should be mucking around with procotol packets like this by default. In particular, what the heck does port 67 mean? Locally I can use it for whatever I want for my own purposes, I don't have to follow the conventions for service ports as specified by the IETF. But I can't have the packet checksum state be left alone for port 67 traffic on a box using virtio because you have this hack there. And yes it's broken on machines using the qemu thing, but at least the hack there is restricted to userspace. Yes, and I think it was a mistake to add the hack there. This is what prevented applications from using the new interface in the 3 years since it was first introduced. It's far more complicated than that. dhclient is part of ISC's DHCP package. They do not have a public SCM and instead require you to join their Software Guild to get access to it. This problem was identified in one distribution and the patch was pushed upstream but because they did not have a public SCM, most other distributions did not see the fix until it appeared in a release. ISC has a pretty long release cycle historically. ISC's had the fix for a long time but there was a 3-year gap in their releases and since their SCM isn't public, users are stuck with the last release. This hack makes sense in QEMU as we have a few hacks like this to fix broken guests. A primary use of virtualization is to run old applications so it makes sense for us to do that. IMO it was wrong to put it in qemu: originally, if a distro shipped a broken virtio/dhclient combo, it was it's own bug to fix. But now that qemu has shipped the work-around for so long, broken guests seemed work. The guests were broken before qemu implemented this. virtio-net had checksum offload long before it was ever implemented in qemu. Not even lguest implemented it because the interfaces weren't available in tun/tap. I'm not sure how Rusty ever tested it. We only discovered this bug after checksum offload was implemented in tun/tap and we were able to enable it in QEMU. At that point, the guests had shipped and were in the wild. The real problem was that we implemented a feature in a guest driver without having a backend implementation and then shipped the code. Shipping untested code is a recipe for failure. If we had implemented the front-end feature only when a backend implementation was available, we would have caught this, fixed it in the guests, and not be in the situation because there wouldn't be these broken guests. So we *still* see the bug re-surface in new guests. Which guests? Newer versions of dhclient should work as expected. And since they are fairly new, it is interesting to get decent performance from them now. I don't think it makes sense for vhost to do this. These guests are so old that they don't have the requisite features to achieve really high performance anyway. I've always thought making vhost totally transparent was a bad idea and this is one of the reasons. It does not have to be fully transparent. You can insert your own ring in the middle, and copy descriptors around. And we stop on errors and let userspace handle. This will come handy if we get e.g. virtio bug that we need to work around. I mean from a UI perspective. IOW, if users have to explicitly choose to use vhost-net, then it's okay to force them to use newer guests that support vhost-net. However, if we make it transparent, then it has to support everything that QEMU virtio has ever supported which is problematic for exactly the reasons you are now encountering. We can do a lot of ugly things in userspace that we shouldn't be doing in the kernel. Regards, Anthony
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin m...@redhat.com Date: Mon, 28 Jun 2010 13:08:07 +0300 Userspace virtio server has the following hack so guests rely on it, and we have to replicate it, too: Use port number to detect incoming IPv4 DHCP response packets, and fill in the checksum for these. The issue we are solving is that on linux guests, some apps that use recvmsg with AF_PACKET sockets, don't know how to handle CHECKSUM_PARTIAL; The interface to return the relevant information was added in 8dc4194474159660d7f37c495e3fc3f10d0db8cc, and older userspace does not use it. One important user of recvmsg with AF_PACKET is dhclient, so we add a work-around just for DHCP. Don't bother applying the hack to IPv6 as userspace virtio does not have a work-around for that - let's hope guests will do the right thing wrt IPv6. Signed-off-by: Michael S. Tsirkin m...@redhat.com Yikes, this is awful too. Nothing in the kernel should be mucking around with procotol packets like this by default. In particular, what the heck does port 67 mean? Locally I can use it for whatever I want for my own purposes, I don't have to follow the conventions for service ports as specified by the IETF. But I can't have the packet checksum state be left alone for port 67 traffic on a box using virtio because you have this hack there. And yes it's broken on machines using the qemu thing, but at least the hack there is restricted to userspace. I really don't want anything in the kernel that looks like this. These applications are broken, and we've provided a way for them to work properly. What's the point of having fixed applications if all of these hacks grow like fungus over every virtualization transport? It just means that people won't fix the apps, since they don't have to. There is no incentive, and the mechanism we created to properly handle this loses it's value. At best, you can write a netfilter module that mucks up the packet checksum state in these situations. At least in that case, you can make it generic (it mangles iff a packet matches a certain rule, so for your virtio guests you'd make it match for DHCP frames) instead of being some hard-coded DHCP thing by design. And since this is so cleanly seperated and portable you don't even need to push it upstream. It's a temporary workaround for a temporary problem. You can just delete it as soon as the majority of guests have the fixed dhcp. The qemu crap should disappear similarly. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
On Tue, Jun 29, 2010 at 12:36:47AM -0700, David Miller wrote: From: Michael S. Tsirkin m...@redhat.com Date: Mon, 28 Jun 2010 13:08:07 +0300 Userspace virtio server has the following hack so guests rely on it, and we have to replicate it, too: Use port number to detect incoming IPv4 DHCP response packets, and fill in the checksum for these. The issue we are solving is that on linux guests, some apps that use recvmsg with AF_PACKET sockets, don't know how to handle CHECKSUM_PARTIAL; The interface to return the relevant information was added in 8dc4194474159660d7f37c495e3fc3f10d0db8cc, and older userspace does not use it. One important user of recvmsg with AF_PACKET is dhclient, so we add a work-around just for DHCP. Don't bother applying the hack to IPv6 as userspace virtio does not have a work-around for that - let's hope guests will do the right thing wrt IPv6. Signed-off-by: Michael S. Tsirkin m...@redhat.com Yikes, this is awful too. Nothing in the kernel should be mucking around with procotol packets like this by default. In particular, what the heck does port 67 mean? Locally I can use it for whatever I want for my own purposes, I don't have to follow the conventions for service ports as specified by the IETF. But I can't have the packet checksum state be left alone for port 67 traffic on a box using virtio because you have this hack there. And yes it's broken on machines using the qemu thing, but at least the hack there is restricted to userspace. Yes, and I think it was a mistake to add the hack there. This is what prevented applications from using the new interface in the 3 years since it was first introduced. I really don't want anything in the kernel that looks like this. These applications are broken, and we've provided a way for them to work properly. What's the point of having fixed applications if all of these hacks grow like fungus over every virtualization transport? It just means that people won't fix the apps, since they don't have to. There is no incentive, and the mechanism we created to properly handle this loses it's value. At best, you can write a netfilter module that mucks up the packet checksum state in these situations. At least in that case, you can make it generic (it mangles iff a packet matches a certain rule, so for your virtio guests you'd make it match for DHCP frames) instead of being some hard-coded DHCP thing by design. Nod. One question on implementation: why does skb_checksum_help set the checksum state to CHECKSUM_NONE? Shouldn't it be CHECKSUM_COMPLETE? And since this is so cleanly seperated and portable you don't even need to push it upstream. It's a temporary workaround for a temporary problem. You can just delete it as soon as the majority of guests have the fixed dhcp. The qemu crap should disappear similarly. Since using the module involves updating the management tools as well, if we go down this route it will be much less painful for everyone to do push it upstream. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCHv2] vhost-net: add dhclient work-around from userspace
Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:
Use port number to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.
The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.
Don't bother applying the hack to IPv6 as userspace virtio does not
have a work-around for that - let's hope guests will do the right
thing wrt IPv6.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
Dave, I'm going to put this patch on the vhost tree,
no need for you to bother merging it - you'll get
it with a pull request.
drivers/vhost/net.c | 44 +++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cc19595..03bba6a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -24,6 +24,10 @@
#include linux/if_tun.h
#include linux/if_macvlan.h
+#include linux/ip.h
+#include linux/udp.h
+#include linux/netdevice.h
+
#include net/sock.h
#include vhost.h
@@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net-dev.mm);
}
+static int peek_head(struct sock *sk)
+{
+ struct sk_buff *skb;
+
+ lock_sock(sk);
+ skb = skb_peek(sk-sk_receive_queue);
+ if (unlikely(!skb)) {
+ release_sock(sk);
+ return 0;
+ }
+ /* Userspace virtio server has the following hack so
+* guests rely on it, and we have to replicate it, too: */
+ /* Use port number to detect incoming IPv4 DHCP response packets,
+* and fill in the checksum. */
+
+ /* The issue we are solving is that on linux guests, some apps
+* that use recvmsg with AF_PACKET sockets, don't know how to
+* handle CHECKSUM_PARTIAL;
+* The interface to return the relevant information was added in
+* 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+* and older userspace does not use it.
+* One important user of recvmsg with AF_PACKET is dhclient,
+* so we add a work-around just for DHCP. */
+ if (skb-ip_summed == CHECKSUM_PARTIAL
+ skb_headlen(skb) = skb_transport_offset(skb) +
+ sizeof(struct udphdr)
+ udp_hdr(skb)-dest == htons(68)
+ skb_network_header_len(skb) = sizeof(struct iphdr)
+ ip_hdr(skb)-protocol == IPPROTO_UDP
+ skb-protocol == htons(ETH_P_IP)) {
+ skb_checksum_help(skb);
+ /* Restore ip_summed value: tun passes it to user. */
+ skb-ip_summed = CHECKSUM_PARTIAL;
+ }
+ release_sock(sk);
+ return 1;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_rx(struct vhost_net *net)
@@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
vq-log : NULL;
- for (;;) {
+ while (peek_head(sock-sk)) {
head = vhost_get_vq_desc(net-dev, vq, vq-iov,
ARRAY_SIZE(vq-iov),
out, in,
--
1.7.1.12.g42b7f
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] vhost-net: add dhclient work-around from userspace
On Mon, 2010-06-28 at 13:08 +0300, Michael S. Tsirkin wrote:
Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:
Use port number to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.
The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.
Don't bother applying the hack to IPv6 as userspace virtio does not
have a work-around for that - let's hope guests will do the right
thing wrt IPv6.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
Dave, I'm going to put this patch on the vhost tree,
no need for you to bother merging it - you'll get
it with a pull request.
drivers/vhost/net.c | 44 +++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cc19595..03bba6a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -24,6 +24,10 @@
#include linux/if_tun.h
#include linux/if_macvlan.h
+#include linux/ip.h
+#include linux/udp.h
+#include linux/netdevice.h
+
#include net/sock.h
#include vhost.h
@@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net-dev.mm);
}
+static int peek_head(struct sock *sk)
This routine is doing more than just peeking the head of sk's receive
queue. May be this should be named similar to what qemu calls
'work_around_broken_dhclient()'
+{
+ struct sk_buff *skb;
+
+ lock_sock(sk);
+ skb = skb_peek(sk-sk_receive_queue);
+ if (unlikely(!skb)) {
+ release_sock(sk);
+ return 0;
+ }
+ /* Userspace virtio server has the following hack so
+ * guests rely on it, and we have to replicate it, too: */
+ /* Use port number to detect incoming IPv4 DHCP response packets,
+ * and fill in the checksum. */
+
+ /* The issue we are solving is that on linux guests, some apps
+ * that use recvmsg with AF_PACKET sockets, don't know how to
+ * handle CHECKSUM_PARTIAL;
+ * The interface to return the relevant information was added in
+ * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+ * and older userspace does not use it.
+ * One important user of recvmsg with AF_PACKET is dhclient,
+ * so we add a work-around just for DHCP. */
+ if (skb-ip_summed == CHECKSUM_PARTIAL
+ skb_headlen(skb) = skb_transport_offset(skb) +
+ sizeof(struct udphdr)
+ udp_hdr(skb)-dest == htons(68)
+ skb_network_header_len(skb) = sizeof(struct iphdr)
+ ip_hdr(skb)-protocol == IPPROTO_UDP
+ skb-protocol == htons(ETH_P_IP)) {
Isn't it more logical to check for skb-protocol, followed by ip_hdr and
then udp_hdr?
+ skb_checksum_help(skb);
+ /* Restore ip_summed value: tun passes it to user. */
+ skb-ip_summed = CHECKSUM_PARTIAL;
+ }
+ release_sock(sk);
+ return 1;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_rx(struct vhost_net *net)
@@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
vq_log = unlikely(vhost_has_feature(net-dev, VHOST_F_LOG_ALL)) ?
vq-log : NULL;
- for (;;) {
+ while (peek_head(sock-sk)) {
head = vhost_get_vq_desc(net-dev, vq, vq-iov,
ARRAY_SIZE(vq-iov),
out, in,
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
