* Re: [PATCH net,stable v2] vhost: fix skb leak in handle_rx()
From: Michael S. Tsirkin @ 2017-11-29 15:31 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <1511965404-23289-1-git-send-email-wexu@redhat.com>
On Wed, Nov 29, 2017 at 09:23:24AM -0500, wexu@redhat.com wrote:
> From: Wei Xu <wexu@redhat.com>
>
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>
> Eventually we figured out that it was a skb leak in handle_rx()
> when sending packets to the VM. This usually happens when a guest
> can not drain out vq as fast as vhost fills in, afterwards it sets
> off the traffic jam and leaks skb(s) which occurs as no headcount
> to send on the vq from vhost side.
>
> This can be avoided by making sure we have got enough headcount
> before actually consuming a skb from the batched rx array while
> transmitting, which is simply done by moving checking the zero
> headcount a bit ahead.
>
> Also strengthen the small possibility of leak in case of recvmsg()
> fails by freeing the skb.
>
> Signed-off-by: Wei Xu <wexu@redhat.com>
> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
> drivers/vhost/net.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> v2:
> - add Matthew as the reporter, thanks matthew.
> - moving zero headcount check ahead instead of defer consuming skb
> due to jason and mst's comment.
> - add freeing skb in favor of recvmsg() fails.
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8d626d7..e302e08 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> /* On error, stop handling until the next kick. */
> if (unlikely(headcount < 0))
> goto out;
> - if (nvq->rx_array)
> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> - /* On overrun, truncate and discard */
> - if (unlikely(headcount > UIO_MAXIOV)) {
> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> - err = sock->ops->recvmsg(sock, &msg,
> - 1, MSG_DONTWAIT | MSG_TRUNC);
> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> - continue;
> - }
> /* OK, now we need to know about added descriptors. */
> if (!headcount) {
> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> @@ -800,6 +790,18 @@ static void handle_rx(struct vhost_net *net)
> * they refilled. */
> goto out;
> }
> + if (nvq->rx_array)
> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> + /* On overrun, truncate and discard */
> + if (unlikely(headcount > UIO_MAXIOV)) {
> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> + err = sock->ops->recvmsg(sock, &msg,
> + 1, MSG_DONTWAIT | MSG_TRUNC);
> + if (unlikely(err != 1))
Why 1? How is receiving 1 byte special or even possible?
Also, I wouldn't put an unlikely here. It's all error handling code anyway.
> + kfree_skb((struct sk_buff *)msg.msg_control);
You do not need a cast here.
Also, is it really safe to refer to msg_control here?
I'd rather keep a copy of the skb pointer and use it than assume
caller did not change it. But also see below.
> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> + continue;
> + }
> /* We don't need to be notified again. */
> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> fixup = msg.msg_iter;
> @@ -818,6 +820,7 @@ static void handle_rx(struct vhost_net *net)
> pr_debug("Discarded rx packet: "
> " len %d, expected %zd\n", err, sock_len);
> vhost_discard_vq_desc(vq, headcount);
> + kfree_skb((struct sk_buff *)msg.msg_control);
You do not need a cast here.
Also, we have
ret = tun_put_user(tun, tfile, skb, to);
if (unlikely(ret < 0))
kfree_skb(skb);
else
consume_skb(skb);
return ret;
So it looks like recvmsg actually always consumes the skb.
So I was wrong when I said you need to kfree it after
recv msg, and your original patch was good.
Jason, what do you think?
> continue;
> }
> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH v18 01/10] idr: add #include <linux/bug.h>
From: Matthew Wilcox @ 2017-11-30 0:58 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
Masahiro Yamada, linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1511963726-34070-2-git-send-email-wei.w.wang@intel.com>
On Wed, Nov 29, 2017 at 09:55:17PM +0800, Wei Wang wrote:
> The <linux/bug.h> was removed from radix-tree.h by the following commit:
> f5bba9d11a256ad2a1c2f8e7fc6aabe6416b7890.
>
> Since that commit, tools/testing/radix-tree/ couldn't pass compilation
> due to: tools/testing/radix-tree/idr.c:17: undefined reference to
> WARN_ON_ONCE. This patch adds the bug.h header to idr.h to solve the
> issue.
Thanks; I sent this same patch out yesterday.
Unfortunately, you didn't cc the author of this breakage, Masahiro Yamada.
I want to highlight that these kinds of header cleanups are risky,
and very low reward. I really don't want to see patches going all over
the tree randomly touching header files. If we've got a real problem
to solve, then sure. But I want to see a strong justification for any
more header file cleanups.
^ permalink raw reply
* Re: [PATCH net,stable v2] vhost: fix skb leak in handle_rx()
From: Jason Wang @ 2017-11-30 2:46 UTC (permalink / raw)
To: Michael S. Tsirkin, wexu; +Cc: mjrosato, netdev, linux-kernel, virtualization
In-Reply-To: <20171129172156-mutt-send-email-mst@kernel.org>
On 2017年11月29日 23:31, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:23:24AM -0500,wexu@redhat.com wrote:
>> From: Wei Xu<wexu@redhat.com>
>>
>> Matthew found a roughly 40% tcp throughput regression with commit
>> c67df11f(vhost_net: try batch dequing from skb array) as discussed
>> in the following thread:
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>>
>> Eventually we figured out that it was a skb leak in handle_rx()
>> when sending packets to the VM. This usually happens when a guest
>> can not drain out vq as fast as vhost fills in, afterwards it sets
>> off the traffic jam and leaks skb(s) which occurs as no headcount
>> to send on the vq from vhost side.
>>
>> This can be avoided by making sure we have got enough headcount
>> before actually consuming a skb from the batched rx array while
>> transmitting, which is simply done by moving checking the zero
>> headcount a bit ahead.
>>
>> Also strengthen the small possibility of leak in case of recvmsg()
>> fails by freeing the skb.
>>
>> Signed-off-by: Wei Xu<wexu@redhat.com>
>> Reported-by: Matthew Rosato<mjrosato@linux.vnet.ibm.com>
>> ---
>> drivers/vhost/net.c | 23 +++++++++++++----------
>> 1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> v2:
>> - add Matthew as the reporter, thanks matthew.
>> - moving zero headcount check ahead instead of defer consuming skb
>> due to jason and mst's comment.
>> - add freeing skb in favor of recvmsg() fails.
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 8d626d7..e302e08 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
>> /* On error, stop handling until the next kick. */
>> if (unlikely(headcount < 0))
>> goto out;
>> - if (nvq->rx_array)
>> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>> - /* On overrun, truncate and discard */
>> - if (unlikely(headcount > UIO_MAXIOV)) {
>> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>> - err = sock->ops->recvmsg(sock, &msg,
>> - 1, MSG_DONTWAIT | MSG_TRUNC);
>> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
>> - continue;
>> - }
>> /* OK, now we need to know about added descriptors. */
>> if (!headcount) {
>> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>> @@ -800,6 +790,18 @@ static void handle_rx(struct vhost_net *net)
>> * they refilled. */
>> goto out;
>> }
>> + if (nvq->rx_array)
>> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>> + /* On overrun, truncate and discard */
>> + if (unlikely(headcount > UIO_MAXIOV)) {
>> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>> + err = sock->ops->recvmsg(sock, &msg,
>> + 1, MSG_DONTWAIT | MSG_TRUNC);
>> + if (unlikely(err != 1))
> Why 1? How is receiving 1 byte special or even possible?
> Also, I wouldn't put an unlikely here. It's all error handling code anyway.
>
>> + kfree_skb((struct sk_buff *)msg.msg_control);
> You do not need a cast here.
> Also, is it really safe to refer to msg_control here?
> I'd rather keep a copy of the skb pointer and use it than assume
> caller did not change it. But also see below.
>
>> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
>> + continue;
>> + }
>> /* We don't need to be notified again. */
>> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
>> fixup = msg.msg_iter;
>> @@ -818,6 +820,7 @@ static void handle_rx(struct vhost_net *net)
>> pr_debug("Discarded rx packet: "
>> " len %d, expected %zd\n", err, sock_len);
>> vhost_discard_vq_desc(vq, headcount);
>> + kfree_skb((struct sk_buff *)msg.msg_control);
> You do not need a cast here.
>
> Also, we have
>
> ret = tun_put_user(tun, tfile, skb, to);
> if (unlikely(ret < 0))
> kfree_skb(skb);
> else
> consume_skb(skb);
>
> return ret;
>
> So it looks like recvmsg actually always consumes the skb.
> So I was wrong when I said you need to kfree it after
> recv msg, and your original patch was good.
>
> Jason, what do you think?
>
tun_recvmsg() has the following check:
static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t
total_len,
int flags)
{
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = __tun_get(tfile);
int ret;
if (!tun)
return -EBADFD;
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
goto out;
}
And tun_do_read() has:
if (!iov_iter_count(to))
return 0;
So I think we need free skb in those cases.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jason Wang @ 2017-11-30 3:29 UTC (permalink / raw)
To: Jesse Brandeburg, virtualization
Cc: Jakub Kicinski, mst, Singhai, Anjali, Hannes Frederic Sowa,
netdev, Achiad, Peter Waskiewicz Jr, Sridhar Samudrala,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171128112722.00003716@intel.com>
On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> to ease configuration of a VM and that would enable live migration of
> passthrough network SR-IOV devices.
>
> Today we have SR-IOV network devices (VFs) that can be passed into a VM
> in order to enable high performance networking direct within the VM.
> The problem I am trying to address is that this configuration is
> generally difficult to live-migrate. There is documentation [1]
> indicating that some OS/Hypervisor vendors will support live migration
> of a system with a direct assigned networking device. The problem I
> see with these implementations is that the network configuration
> requirements that are passed on to the owner of the VM are quite
> complicated. You have to set up bonding, you have to configure it to
> enslave two interfaces, those interfaces (one is virtio-net, the other
> is SR-IOV device/driver like ixgbevf) must support MAC address changes
> requested in the VM, and on and on...
>
> So, on to the proposal:
> Modify virtio-net driver to be a single VM network device that
> enslaves an SR-IOV network device (inside the VM) with the same MAC
> address. This would cause the virtio-net driver to appear and work like
> a simplified bonding/team driver. The live migration problem would be
> solved just like today's bonding solution, but the VM user's networking
> config would be greatly simplified.
>
> At it's simplest, it would appear something like this in the VM.
>
> ==========
> = vnet0 =
> =============
> (virtio- = |
> net) = |
> = ==========
> = = ixgbef =
> ========== ==========
>
> (forgive the ASCII art)
>
> The fast path traffic would prefer the ixgbevf or other SR-IOV device
> path, and fall back to virtio's transmit/receive when migrating.
>
> Compared to today's options this proposal would
> 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> speeds
> 2) simplify end user configuration in the VM (most if not all of the
> set up to enable migration would be done in the hypervisor)
> 3) allow live migration via a simple link down and maybe a PCI
> hot-unplug of the SR-IOV device, with failover to the virtio-net
> driver core
> 4) allow vendor agnostic hardware acceleration, and live migration
> between vendors if the VM os has driver support for all the required
> SR-IOV devices.
>
> Runtime operation proposed:
> - <in either order> virtio-net driver loads, SR-IOV driver loads
> - virtio-net finds other NICs that match it's MAC address by
> both examining existing interfaces, and sets up a new device notifier
> - virtio-net enslaves the first NIC with the same MAC address
> - virtio-net brings up the slave, and makes it the "preferred" path
> - virtio-net follows the behavior of an active backup bond/team
> - virtio-net acts as the interface to the VM
> - live migration initiates
> - link goes down on SR-IOV, or SR-IOV device is removed
> - failover to virtio-net as primary path
> - migration continues to new host
> - new host is started with virio-net as primary
> - if no SR-IOV, virtio-net stays primary
> - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> - virtio-net notices new NIC and starts over at enslave step above
>
> Future ideas (brainstorming):
> - Optimize Fast east-west by having special rules to direct east-west
> traffic through virtio-net traffic path
>
> Thanks for reading!
> Jesse
Cc netdev.
Interesting, and this method is actually used by netvsc now:
commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
Author: stephen hemminger <stephen@networkplumber.org>
Date: Tue Aug 1 19:58:53 2017 -0700
netvsc: transparent VF management
This patch implements transparent fail over from synthetic NIC to
SR-IOV virtual function NIC in Hyper-V environment. It is a better
alternative to using bonding as is done now. Instead, the receive and
transmit fail over is done internally inside the driver.
Using bonding driver has lots of issues because it depends on the
script being run early enough in the boot process and with sufficient
information to make the association. This patch moves all that
functionality into the kernel.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
If my understanding is correct there's no need to for any extension of
virtio spec. If this is true, maybe you can start to prepare the patch?
Thanks
>
> [1]
> https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jakub Kicinski @ 2017-11-30 3:51 UTC (permalink / raw)
To: Jason Wang
Cc: mst, Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <0fb552d4-1bfc-e130-4fc1-87b83873916d@redhat.com>
On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> > to ease configuration of a VM and that would enable live migration of
> > passthrough network SR-IOV devices.
> >
> > Today we have SR-IOV network devices (VFs) that can be passed into a VM
> > in order to enable high performance networking direct within the VM.
> > The problem I am trying to address is that this configuration is
> > generally difficult to live-migrate. There is documentation [1]
> > indicating that some OS/Hypervisor vendors will support live migration
> > of a system with a direct assigned networking device. The problem I
> > see with these implementations is that the network configuration
> > requirements that are passed on to the owner of the VM are quite
> > complicated. You have to set up bonding, you have to configure it to
> > enslave two interfaces, those interfaces (one is virtio-net, the other
> > is SR-IOV device/driver like ixgbevf) must support MAC address changes
> > requested in the VM, and on and on...
> >
> > So, on to the proposal:
> > Modify virtio-net driver to be a single VM network device that
> > enslaves an SR-IOV network device (inside the VM) with the same MAC
> > address. This would cause the virtio-net driver to appear and work like
> > a simplified bonding/team driver. The live migration problem would be
> > solved just like today's bonding solution, but the VM user's networking
> > config would be greatly simplified.
> >
> > At it's simplest, it would appear something like this in the VM.
> >
> > ==========
> > = vnet0 =
> > =============
> > (virtio- = |
> > net) = |
> > = ==========
> > = = ixgbef =
> > ========== ==========
> >
> > (forgive the ASCII art)
> >
> > The fast path traffic would prefer the ixgbevf or other SR-IOV device
> > path, and fall back to virtio's transmit/receive when migrating.
> >
> > Compared to today's options this proposal would
> > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > speeds
> > 2) simplify end user configuration in the VM (most if not all of the
> > set up to enable migration would be done in the hypervisor)
> > 3) allow live migration via a simple link down and maybe a PCI
> > hot-unplug of the SR-IOV device, with failover to the virtio-net
> > driver core
> > 4) allow vendor agnostic hardware acceleration, and live migration
> > between vendors if the VM os has driver support for all the required
> > SR-IOV devices.
> >
> > Runtime operation proposed:
> > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > - virtio-net finds other NICs that match it's MAC address by
> > both examining existing interfaces, and sets up a new device notifier
> > - virtio-net enslaves the first NIC with the same MAC address
> > - virtio-net brings up the slave, and makes it the "preferred" path
> > - virtio-net follows the behavior of an active backup bond/team
> > - virtio-net acts as the interface to the VM
> > - live migration initiates
> > - link goes down on SR-IOV, or SR-IOV device is removed
> > - failover to virtio-net as primary path
> > - migration continues to new host
> > - new host is started with virio-net as primary
> > - if no SR-IOV, virtio-net stays primary
> > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > - virtio-net notices new NIC and starts over at enslave step above
> >
> > Future ideas (brainstorming):
> > - Optimize Fast east-west by having special rules to direct east-west
> > traffic through virtio-net traffic path
> >
> > Thanks for reading!
> > Jesse
>
> Cc netdev.
>
> Interesting, and this method is actually used by netvsc now:
>
> commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> Author: stephen hemminger <stephen@networkplumber.org>
> Date: Tue Aug 1 19:58:53 2017 -0700
>
> netvsc: transparent VF management
>
> This patch implements transparent fail over from synthetic NIC to
> SR-IOV virtual function NIC in Hyper-V environment. It is a better
> alternative to using bonding as is done now. Instead, the receive and
> transmit fail over is done internally inside the driver.
>
> Using bonding driver has lots of issues because it depends on the
> script being run early enough in the boot process and with sufficient
> information to make the association. This patch moves all that
> functionality into the kernel.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> If my understanding is correct there's no need to for any extension of
> virtio spec. If this is true, maybe you can start to prepare the patch?
IMHO this is as close to policy in the kernel as one can get. User
land has all the information it needs to instantiate that bond/team
automatically. In fact I'm trying to discuss this with NetworkManager
folks and Red Hat right now:
https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
Can we flip the argument and ask why is the kernel supposed to be
responsible for this? It's not like we run DHCP out of the kernel
on new interfaces...
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Stephen Hemminger @ 2017-11-30 4:10 UTC (permalink / raw)
To: Jakub Kicinski
Cc: mst, Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171129195138.63512ead@cakuba.netronome.com>
On Wed, 29 Nov 2017 19:51:38 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > Hi, I'd like to get some feedback on a proposal to enhance
> > > virtio-net to ease configuration of a VM and that would enable
> > > live migration of passthrough network SR-IOV devices.
> > >
> > > Today we have SR-IOV network devices (VFs) that can be passed
> > > into a VM in order to enable high performance networking direct
> > > within the VM. The problem I am trying to address is that this
> > > configuration is generally difficult to live-migrate. There is
> > > documentation [1] indicating that some OS/Hypervisor vendors will
> > > support live migration of a system with a direct assigned
> > > networking device. The problem I see with these implementations
> > > is that the network configuration requirements that are passed on
> > > to the owner of the VM are quite complicated. You have to set up
> > > bonding, you have to configure it to enslave two interfaces,
> > > those interfaces (one is virtio-net, the other is SR-IOV
> > > device/driver like ixgbevf) must support MAC address changes
> > > requested in the VM, and on and on...
> > >
> > > So, on to the proposal:
> > > Modify virtio-net driver to be a single VM network device that
> > > enslaves an SR-IOV network device (inside the VM) with the same
> > > MAC address. This would cause the virtio-net driver to appear and
> > > work like a simplified bonding/team driver. The live migration
> > > problem would be solved just like today's bonding solution, but
> > > the VM user's networking config would be greatly simplified.
> > >
> > > At it's simplest, it would appear something like this in the VM.
> > >
> > > ==========
> > > = vnet0 =
> > > =============
> > > (virtio- = |
> > > net) = |
> > > = ==========
> > > = = ixgbef =
> > > ========== ==========
> > >
> > > (forgive the ASCII art)
> > >
> > > The fast path traffic would prefer the ixgbevf or other SR-IOV
> > > device path, and fall back to virtio's transmit/receive when
> > > migrating.
> > >
> > > Compared to today's options this proposal would
> > > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > > speeds
> > > 2) simplify end user configuration in the VM (most if not all of
> > > the set up to enable migration would be done in the hypervisor)
> > > 3) allow live migration via a simple link down and maybe a PCI
> > > hot-unplug of the SR-IOV device, with failover to the
> > > virtio-net driver core
> > > 4) allow vendor agnostic hardware acceleration, and live migration
> > > between vendors if the VM os has driver support for all the
> > > required SR-IOV devices.
> > >
> > > Runtime operation proposed:
> > > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > > - virtio-net finds other NICs that match it's MAC address by
> > > both examining existing interfaces, and sets up a new device
> > > notifier
> > > - virtio-net enslaves the first NIC with the same MAC address
> > > - virtio-net brings up the slave, and makes it the "preferred"
> > > path
> > > - virtio-net follows the behavior of an active backup bond/team
> > > - virtio-net acts as the interface to the VM
> > > - live migration initiates
> > > - link goes down on SR-IOV, or SR-IOV device is removed
> > > - failover to virtio-net as primary path
> > > - migration continues to new host
> > > - new host is started with virio-net as primary
> > > - if no SR-IOV, virtio-net stays primary
> > > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > > - virtio-net notices new NIC and starts over at enslave step above
> > >
> > > Future ideas (brainstorming):
> > > - Optimize Fast east-west by having special rules to direct
> > > east-west traffic through virtio-net traffic path
> > >
> > > Thanks for reading!
> > > Jesse
> >
> > Cc netdev.
> >
> > Interesting, and this method is actually used by netvsc now:
> >
> > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > Author: stephen hemminger <stephen@networkplumber.org>
> > Date: Tue Aug 1 19:58:53 2017 -0700
> >
> > netvsc: transparent VF management
> >
> > This patch implements transparent fail over from synthetic NIC
> > to SR-IOV virtual function NIC in Hyper-V environment. It is a
> > better alternative to using bonding as is done now. Instead, the
> > receive and transmit fail over is done internally inside the driver.
> >
> > Using bonding driver has lots of issues because it depends on
> > the script being run early enough in the boot process and with
> > sufficient information to make the association. This patch moves
> > all that functionality into the kernel.
> >
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > If my understanding is correct there's no need to for any extension
> > of virtio spec. If this is true, maybe you can start to prepare the
> > patch?
>
> IMHO this is as close to policy in the kernel as one can get. User
> land has all the information it needs to instantiate that bond/team
> automatically. In fact I'm trying to discuss this with NetworkManager
> folks and Red Hat right now:
>
> https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
>
> Can we flip the argument and ask why is the kernel supposed to be
> responsible for this? It's not like we run DHCP out of the kernel
> on new interfaces...
Although "policy should not be in the kernel" is a a great mantra,
it is not practical in the real world.
If you think it can be solved in userspace, then you haven't had to
deal with four different network initialization
systems, multiple orchestration systems and customers on ancient
Enterprise distributions.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jakub Kicinski @ 2017-11-30 4:21 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171129201009.4d48dc6d@xeon-e3>
On Wed, 29 Nov 2017 20:10:09 -0800, Stephen Hemminger wrote:
> On Wed, 29 Nov 2017 19:51:38 -0800 Jakub Kicinski wrote:
> > On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > > Author: stephen hemminger <stephen@networkplumber.org>
> > > Date: Tue Aug 1 19:58:53 2017 -0700
> > >
> > > netvsc: transparent VF management
> > >
> > > This patch implements transparent fail over from synthetic NIC
> > > to SR-IOV virtual function NIC in Hyper-V environment. It is a
> > > better alternative to using bonding as is done now. Instead, the
> > > receive and transmit fail over is done internally inside the driver.
> > >
> > > Using bonding driver has lots of issues because it depends on
> > > the script being run early enough in the boot process and with
> > > sufficient information to make the association. This patch moves
> > > all that functionality into the kernel.
> > >
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > If my understanding is correct there's no need to for any extension
> > > of virtio spec. If this is true, maybe you can start to prepare the
> > > patch?
> >
> > IMHO this is as close to policy in the kernel as one can get. User
> > land has all the information it needs to instantiate that bond/team
> > automatically. In fact I'm trying to discuss this with NetworkManager
> > folks and Red Hat right now:
> >
> > https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
> >
> > Can we flip the argument and ask why is the kernel supposed to be
> > responsible for this? It's not like we run DHCP out of the kernel
> > on new interfaces...
>
> Although "policy should not be in the kernel" is a a great mantra,
> it is not practical in the real world.
>
> If you think it can be solved in userspace, then you haven't had to
> deal with four different network initialization
> systems, multiple orchestration systems and customers on ancient
> Enterprise distributions.
I would accept that argument if anyone ever tried to get those
Enterprise distros to handle this use case. From conversations I
had it seemed like no one ever did, and SR-IOV+virtio bonding is
what has been done to solve this since day 1 of SR-IOV networking.
For practical reasons it's easier to push this into the kernel,
because vendors rarely employ developers of the user space
orchestrations systems. Is that not the real problem here,
potentially? :)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net,stable v2] vhost: fix skb leak in handle_rx()
From: Wei Xu @ 2017-11-30 4:45 UTC (permalink / raw)
To: Jason Wang; +Cc: mjrosato, netdev, mst, linux-kernel, virtualization
In-Reply-To: <6eb987d4-dac1-3da8-a748-11fab1da0100@redhat.com>
On Wed, Nov 29, 2017 at 10:43:33PM +0800, Jason Wang wrote:
>
>
> On 2017年11月29日 22:23, wexu@redhat.com wrote:
> > From: Wei Xu <wexu@redhat.com>
> >
> > Matthew found a roughly 40% tcp throughput regression with commit
> > c67df11f(vhost_net: try batch dequing from skb array) as discussed
> > in the following thread:
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
> >
> > Eventually we figured out that it was a skb leak in handle_rx()
> > when sending packets to the VM. This usually happens when a guest
> > can not drain out vq as fast as vhost fills in, afterwards it sets
> > off the traffic jam and leaks skb(s) which occurs as no headcount
> > to send on the vq from vhost side.
> >
> > This can be avoided by making sure we have got enough headcount
> > before actually consuming a skb from the batched rx array while
> > transmitting, which is simply done by moving checking the zero
> > headcount a bit ahead.
> >
> > Also strengthen the small possibility of leak in case of recvmsg()
> > fails by freeing the skb.
> >
> > Signed-off-by: Wei Xu <wexu@redhat.com>
> > Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/net.c | 23 +++++++++++++----------
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > v2:
> > - add Matthew as the reporter, thanks matthew.
> > - moving zero headcount check ahead instead of defer consuming skb
> > due to jason and mst's comment.
> > - add freeing skb in favor of recvmsg() fails.
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 8d626d7..e302e08 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> > /* On error, stop handling until the next kick. */
> > if (unlikely(headcount < 0))
> > goto out;
> > - if (nvq->rx_array)
> > - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > - /* On overrun, truncate and discard */
> > - if (unlikely(headcount > UIO_MAXIOV)) {
> > - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > - err = sock->ops->recvmsg(sock, &msg,
> > - 1, MSG_DONTWAIT | MSG_TRUNC);
> > - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > - continue;
> > - }
> > /* OK, now we need to know about added descriptors. */
> > if (!headcount) {
> > if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > @@ -800,6 +790,18 @@ static void handle_rx(struct vhost_net *net)
> > * they refilled. */
> > goto out;
> > }
> > + if (nvq->rx_array)
> > + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > + /* On overrun, truncate and discard */
> > + if (unlikely(headcount > UIO_MAXIOV)) {
> > + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > + err = sock->ops->recvmsg(sock, &msg,
> > + 1, MSG_DONTWAIT | MSG_TRUNC);
> > + if (unlikely(err != 1))
> > + kfree_skb((struct sk_buff *)msg.msg_control);
>
> I think we'd better fix this in tun/tap (better in another patch) otherwise
> it lead to an odd API: some case skb were freed in recvmsg() but caller
> still need to deal with the rest case.
Right, it is better to handle it in recvmsg().
Wei
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v18 01/10] idr: add #include <linux/bug.h>
From: Michal Hocko @ 2017-11-30 7:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, Masahiro Yamada,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171130005817.GA14785@bombadil.infradead.org>
On Wed 29-11-17 16:58:17, Matthew Wilcox wrote:
> On Wed, Nov 29, 2017 at 09:55:17PM +0800, Wei Wang wrote:
> > The <linux/bug.h> was removed from radix-tree.h by the following commit:
> > f5bba9d11a256ad2a1c2f8e7fc6aabe6416b7890.
> >
> > Since that commit, tools/testing/radix-tree/ couldn't pass compilation
> > due to: tools/testing/radix-tree/idr.c:17: undefined reference to
> > WARN_ON_ONCE. This patch adds the bug.h header to idr.h to solve the
> > issue.
>
> Thanks; I sent this same patch out yesterday.
>
> Unfortunately, you didn't cc the author of this breakage, Masahiro Yamada.
> I want to highlight that these kinds of header cleanups are risky,
> and very low reward. I really don't want to see patches going all over
> the tree randomly touching header files. If we've got a real problem
> to solve, then sure. But I want to see a strong justification for any
> more header file cleanups.
I agree. It usually requires unexpected combination of config options to
uncover some nasty include dependencies. So these patches might break
build while their additional value is quite questionable.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-11-30 8:08 UTC (permalink / raw)
To: Jason Wang
Cc: Jakub Kicinski, mst, Sridhar Samudrala, Hannes Frederic Sowa,
netdev, virtualization, Achiad, Peter Waskiewicz Jr,
Singhai, Anjali, Andy Gospodarek, Or Gerlitz
In-Reply-To: <0fb552d4-1bfc-e130-4fc1-87b83873916d@redhat.com>
On 30 November 2017 at 05:29, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年11月29日 03:27, Jesse Brandeburg wrote:
>>
>> Hi, I'd like to get some feedback on a proposal to enhance virtio-net
>> to ease configuration of a VM and that would enable live migration of
>> passthrough network SR-IOV devices.
>>
>> Today we have SR-IOV network devices (VFs) that can be passed into a VM
>> in order to enable high performance networking direct within the VM.
>> The problem I am trying to address is that this configuration is
>> generally difficult to live-migrate. There is documentation [1]
>> indicating that some OS/Hypervisor vendors will support live migration
>> of a system with a direct assigned networking device. The problem I
>> see with these implementations is that the network configuration
>> requirements that are passed on to the owner of the VM are quite
>> complicated. You have to set up bonding, you have to configure it to
>> enslave two interfaces, those interfaces (one is virtio-net, the other
>> is SR-IOV device/driver like ixgbevf) must support MAC address changes
>> requested in the VM, and on and on...
>>
>> So, on to the proposal:
>> Modify virtio-net driver to be a single VM network device that
>> enslaves an SR-IOV network device (inside the VM) with the same MAC
>> address. This would cause the virtio-net driver to appear and work like
>> a simplified bonding/team driver. The live migration problem would be
>> solved just like today's bonding solution, but the VM user's networking
>> config would be greatly simplified.
>>
>> At it's simplest, it would appear something like this in the VM.
>>
>> ==========
>> = vnet0 =
>> =============
>> (virtio- = |
>> net) = |
>> = ==========
>> = = ixgbef =
>> ========== ==========
>>
>> (forgive the ASCII art)
>>
>> The fast path traffic would prefer the ixgbevf or other SR-IOV device
>> path, and fall back to virtio's transmit/receive when migrating.
>>
>> Compared to today's options this proposal would
>> 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
>> speeds
>> 2) simplify end user configuration in the VM (most if not all of the
>> set up to enable migration would be done in the hypervisor)
>> 3) allow live migration via a simple link down and maybe a PCI
>> hot-unplug of the SR-IOV device, with failover to the virtio-net
>> driver core
>> 4) allow vendor agnostic hardware acceleration, and live migration
>> between vendors if the VM os has driver support for all the required
>> SR-IOV devices.
>>
>> Runtime operation proposed:
>> - <in either order> virtio-net driver loads, SR-IOV driver loads
>> - virtio-net finds other NICs that match it's MAC address by
>> both examining existing interfaces, and sets up a new device notifier
>> - virtio-net enslaves the first NIC with the same MAC address
>> - virtio-net brings up the slave, and makes it the "preferred" path
>> - virtio-net follows the behavior of an active backup bond/team
>> - virtio-net acts as the interface to the VM
>> - live migration initiates
>> - link goes down on SR-IOV, or SR-IOV device is removed
>> - failover to virtio-net as primary path
>> - migration continues to new host
>> - new host is started with virio-net as primary
>> - if no SR-IOV, virtio-net stays primary
>> - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
>> - virtio-net notices new NIC and starts over at enslave step above
>>
>> Future ideas (brainstorming):
>> - Optimize Fast east-west by having special rules to direct east-west
>> traffic through virtio-net traffic path
>>
>> Thanks for reading!
>> Jesse
>
>
> Cc netdev.
>
> Interesting, and this method is actually used by netvsc now:
>
> commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> Author: stephen hemminger <stephen@networkplumber.org>
> Date: Tue Aug 1 19:58:53 2017 -0700
>
> netvsc: transparent VF management
>
> This patch implements transparent fail over from synthetic NIC to
> SR-IOV virtual function NIC in Hyper-V environment. It is a better
> alternative to using bonding as is done now. Instead, the receive and
> transmit fail over is done internally inside the driver.
>
> Using bonding driver has lots of issues because it depends on the
> script being run early enough in the boot process and with sufficient
> information to make the association. This patch moves all that
> functionality into the kernel.
>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> If my understanding is correct there's no need to for any extension of
> virtio spec. If this is true, maybe you can start to prepare the patch?
>
> Thanks
>
>>
>> [1]
>>
>> https://access.redhat.com/documentation/en-us/red_hat_virtualization/4.1/html/virtual_machine_management_guide/sect-migrating_virtual_machines_between_hosts
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
>
I do not see why to couple the solution with any specific para-virt technology.
Not with netvsc, nor with virt-io.
One may wish to implement the routing between the VMs and the HV
without any PV device at all, e.g using VF representors and PCIe
loopback, as done with ASAP2-direct.
This method is actually much more efficient in CPU utilization (on the
expense of PCIe BW utilization).
So let's try to first specify the problems that need to be resolved in
order to support Live Migration with SR-IOV rather than rely on
already-done-work (netvsc) without understanding if/why it was right
from the beginning.
To my understanding the problems are the following:
1) DMA: with SR-IOV devices write directly into the guests memory
which yields dirty guest pages that are not marked as dirty for the
host CPU MMU, thus preventing the migration pre-copy phase from
starting while the guest is running on the source machine.
2) Guest network interface persistency: VF detachment causes VF driver
PCI remove which causes the VF netdev to disappear. If that VF netdev
is a guest primary interface (has an IP), sockets using it will break.
Re problem #1:
So far in this mail thread, it was taken for granted that the way to
resolve it is to have a PV device as backup for the pre-copy phase.
In addition to tying the solution with para-virt being in place (which
as already said seems a wrong enforcement to me), it does not really
solve the problem, rather partially works around it.
It just mitigates the problem from long service downtime to long
service degradation time.
To really stab the problem in its heart we need to just mark the guest
DMA written pages as dirty.
Alexander Duyck already initiated patches to address it ~two years ago
(https://groups.google.com/forum/#!topic/linux.kernel/aIQOsh2oJEk) but
un-fortunately they were abandoned.
The simplest way I can think of to resolve it is to have the guest VF
driver just read-modify-write some word of each DMA page before
passing it to the stack by netif_rx().
To limit the performance impact of this operation we can signal the VM
to start doing it only upon pre-copy phase start.
Re. problem #2:
Indeed the best way to address it seems to be to enslave the VF driver
netdev under a persistent anchor netdev.
And it's indeed desired to allow (but not enforce) PV netdev and VF
netdev to work in conjunction.
And it's indeed desired that this enslavement logic work out-of-the box.
But in case of PV+VF some configurable policies must be in place (and
they'd better be generic rather than differ per PV technology).
For example - based on which characteristics should the PV+VF coupling
be done? netvsc uses MAC address, but that might not always be the
desire.
Another example - when to use PV and when to use VF? One may want to
use PV only if VF is gone/down, while others may want to use PV also
when VF is up, e.g for multicasting.
I think the right way to address it is to have a new dedicated module
for this purpose.
Have it automatically enslave PV and VF netdevs according to user
configured policy. Enslave the VF even if there is no PV device at
all.
This way we get:
1) Optimal migration performance
2) A PV agnostic (VM may be migrated even from one PV technology to
another) and HW device agnostic solution
A dedicated generic module will also enforce a lower common
denominator of guest netdev features, preventing migration dependency
on source/guest machine capabilities.
3) Out-of-the box solution yet with generic methods for policy setting
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net,stable v2] vhost: fix skb leak in handle_rx()
From: Wei Xu @ 2017-11-30 9:39 UTC (permalink / raw)
To: Jason Wang
Cc: mjrosato, netdev, virtualization, linux-kernel,
Michael S. Tsirkin
In-Reply-To: <7f6f050d-381d-c123-8cc2-16423e205fb0@redhat.com>
On Thu, Nov 30, 2017 at 10:46:17AM +0800, Jason Wang wrote:
>
>
> On 2017年11月29日 23:31, Michael S. Tsirkin wrote:
> > On Wed, Nov 29, 2017 at 09:23:24AM -0500,wexu@redhat.com wrote:
> > > From: Wei Xu<wexu@redhat.com>
> > >
> > > Matthew found a roughly 40% tcp throughput regression with commit
> > > c67df11f(vhost_net: try batch dequing from skb array) as discussed
> > > in the following thread:
> > > https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
> > >
> > > Eventually we figured out that it was a skb leak in handle_rx()
> > > when sending packets to the VM. This usually happens when a guest
> > > can not drain out vq as fast as vhost fills in, afterwards it sets
> > > off the traffic jam and leaks skb(s) which occurs as no headcount
> > > to send on the vq from vhost side.
> > >
> > > This can be avoided by making sure we have got enough headcount
> > > before actually consuming a skb from the batched rx array while
> > > transmitting, which is simply done by moving checking the zero
> > > headcount a bit ahead.
> > >
> > > Also strengthen the small possibility of leak in case of recvmsg()
> > > fails by freeing the skb.
> > >
> > > Signed-off-by: Wei Xu<wexu@redhat.com>
> > > Reported-by: Matthew Rosato<mjrosato@linux.vnet.ibm.com>
> > > ---
> > > drivers/vhost/net.c | 23 +++++++++++++----------
> > > 1 file changed, 13 insertions(+), 10 deletions(-)
> > >
> > > v2:
> > > - add Matthew as the reporter, thanks matthew.
> > > - moving zero headcount check ahead instead of defer consuming skb
> > > due to jason and mst's comment.
> > > - add freeing skb in favor of recvmsg() fails.
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 8d626d7..e302e08 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> > > /* On error, stop handling until the next kick. */
> > > if (unlikely(headcount < 0))
> > > goto out;
> > > - if (nvq->rx_array)
> > > - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > > - /* On overrun, truncate and discard */
> > > - if (unlikely(headcount > UIO_MAXIOV)) {
> > > - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > > - err = sock->ops->recvmsg(sock, &msg,
> > > - 1, MSG_DONTWAIT | MSG_TRUNC);
> > > - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > > - continue;
> > > - }
> > > /* OK, now we need to know about added descriptors. */
> > > if (!headcount) {
> > > if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > > @@ -800,6 +790,18 @@ static void handle_rx(struct vhost_net *net)
> > > * they refilled. */
> > > goto out;
> > > }
> > > + if (nvq->rx_array)
> > > + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > > + /* On overrun, truncate and discard */
> > > + if (unlikely(headcount > UIO_MAXIOV)) {
> > > + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > > + err = sock->ops->recvmsg(sock, &msg,
> > > + 1, MSG_DONTWAIT | MSG_TRUNC);
> > > + if (unlikely(err != 1))
> > Why 1? How is receiving 1 byte special or even possible?
> > Also, I wouldn't put an unlikely here. It's all error handling code anyway.
Vhost is dropping the skb by invoking a 1 byte recvmsg() here, while it
is kind of weird to free skb since it would have been freed in recvmsg()
for most cases, and the return value doesn't make sense too much.
> >
> > > + kfree_skb((struct sk_buff *)msg.msg_control);
> > You do not need a cast here.
Yes, exactly, I missed it.
> > Also, is it really safe to refer to msg_control here?
> > I'd rather keep a copy of the skb pointer and use it than assume
> > caller did not change it. But also see below.
It should be safe since msg is a local variable here, the callee has no
chance to modify it, except rx_array is not used by vhost and then it
becomes uncertain, but I don't know what the case is. Isn't vhost using
rx_array for all kinds of devices? Any clue rings the bell?
> >
> > > + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > > + continue;
> > > + }
> > > /* We don't need to be notified again. */
> > > iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> > > fixup = msg.msg_iter;
> > > @@ -818,6 +820,7 @@ static void handle_rx(struct vhost_net *net)
> > > pr_debug("Discarded rx packet: "
> > > " len %d, expected %zd\n", err, sock_len);
> > > vhost_discard_vq_desc(vq, headcount);
> > > + kfree_skb((struct sk_buff *)msg.msg_control);
> > You do not need a cast here.
> >
> > Also, we have
> >
> > ret = tun_put_user(tun, tfile, skb, to);
> > if (unlikely(ret < 0))
> > kfree_skb(skb);
> > else
> > consume_skb(skb);
> >
> > return ret;
> >
> > So it looks like recvmsg actually always consumes the skb.
> > So I was wrong when I said you need to kfree it after
> > recv msg, and your original patch was good.
OK, I will repost it.
BTW, Per Jason's comments below, vhost has passed in proper flag,
iov, etc to both tun and tap device, so it would not be an issue.
The only case probably be missed would be removing tun dynamically
with traffic from vhost, while I am not sure how this can be reproduced,
I remember someone reported he got a similar issue by repeatedly
creating and destroying 1000+ VMs simultaneously. Has these kinds of
issues been fixed already? Or that might come about still?
if (!tun)
return -EBADFD;
Anyway, what about do it in recvmsg() and in another patch later on?
Wei
> >
> > Jason, what do you think?
> >
>
> tun_recvmsg() has the following check:
>
> static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t
> total_len,
> int flags)
> {
> struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> struct tun_struct *tun = __tun_get(tfile);
> int ret;
>
> if (!tun)
> return -EBADFD;
>
> if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> ret = -EINVAL;
> goto out;
> }
>
> And tun_do_read() has:
>
> if (!iov_iter_count(to))
> return 0;
>
> So I think we need free skb in those cases.
>
> Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net,stable v2] vhost: fix skb leak in handle_rx()
From: Michael S. Tsirkin @ 2017-11-30 13:25 UTC (permalink / raw)
To: Jason Wang; +Cc: mjrosato, netdev, wexu, linux-kernel, virtualization
In-Reply-To: <7f6f050d-381d-c123-8cc2-16423e205fb0@redhat.com>
On Thu, Nov 30, 2017 at 10:46:17AM +0800, Jason Wang wrote:
>
>
> On 2017年11月29日 23:31, Michael S. Tsirkin wrote:
> > On Wed, Nov 29, 2017 at 09:23:24AM -0500,wexu@redhat.com wrote:
> > > From: Wei Xu<wexu@redhat.com>
> > >
> > > Matthew found a roughly 40% tcp throughput regression with commit
> > > c67df11f(vhost_net: try batch dequing from skb array) as discussed
> > > in the following thread:
> > > https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
> > >
> > > Eventually we figured out that it was a skb leak in handle_rx()
> > > when sending packets to the VM. This usually happens when a guest
> > > can not drain out vq as fast as vhost fills in, afterwards it sets
> > > off the traffic jam and leaks skb(s) which occurs as no headcount
> > > to send on the vq from vhost side.
> > >
> > > This can be avoided by making sure we have got enough headcount
> > > before actually consuming a skb from the batched rx array while
> > > transmitting, which is simply done by moving checking the zero
> > > headcount a bit ahead.
> > >
> > > Also strengthen the small possibility of leak in case of recvmsg()
> > > fails by freeing the skb.
> > >
> > > Signed-off-by: Wei Xu<wexu@redhat.com>
> > > Reported-by: Matthew Rosato<mjrosato@linux.vnet.ibm.com>
> > > ---
> > > drivers/vhost/net.c | 23 +++++++++++++----------
> > > 1 file changed, 13 insertions(+), 10 deletions(-)
> > >
> > > v2:
> > > - add Matthew as the reporter, thanks matthew.
> > > - moving zero headcount check ahead instead of defer consuming skb
> > > due to jason and mst's comment.
> > > - add freeing skb in favor of recvmsg() fails.
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 8d626d7..e302e08 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
> > > /* On error, stop handling until the next kick. */
> > > if (unlikely(headcount < 0))
> > > goto out;
> > > - if (nvq->rx_array)
> > > - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > > - /* On overrun, truncate and discard */
> > > - if (unlikely(headcount > UIO_MAXIOV)) {
> > > - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > > - err = sock->ops->recvmsg(sock, &msg,
> > > - 1, MSG_DONTWAIT | MSG_TRUNC);
> > > - pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > > - continue;
> > > - }
> > > /* OK, now we need to know about added descriptors. */
> > > if (!headcount) {
> > > if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > > @@ -800,6 +790,18 @@ static void handle_rx(struct vhost_net *net)
> > > * they refilled. */
> > > goto out;
> > > }
> > > + if (nvq->rx_array)
> > > + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> > > + /* On overrun, truncate and discard */
> > > + if (unlikely(headcount > UIO_MAXIOV)) {
> > > + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> > > + err = sock->ops->recvmsg(sock, &msg,
> > > + 1, MSG_DONTWAIT | MSG_TRUNC);
> > > + if (unlikely(err != 1))
> > Why 1? How is receiving 1 byte special or even possible?
> > Also, I wouldn't put an unlikely here. It's all error handling code anyway.
> >
> > > + kfree_skb((struct sk_buff *)msg.msg_control);
> > You do not need a cast here.
> > Also, is it really safe to refer to msg_control here?
> > I'd rather keep a copy of the skb pointer and use it than assume
> > caller did not change it. But also see below.
> >
> > > + pr_debug("Discarded rx packet: len %zd\n", sock_len);
> > > + continue;
> > > + }
> > > /* We don't need to be notified again. */
> > > iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> > > fixup = msg.msg_iter;
> > > @@ -818,6 +820,7 @@ static void handle_rx(struct vhost_net *net)
> > > pr_debug("Discarded rx packet: "
> > > " len %d, expected %zd\n", err, sock_len);
> > > vhost_discard_vq_desc(vq, headcount);
> > > + kfree_skb((struct sk_buff *)msg.msg_control);
> > You do not need a cast here.
> >
> > Also, we have
> >
> > ret = tun_put_user(tun, tfile, skb, to);
> > if (unlikely(ret < 0))
> > kfree_skb(skb);
> > else
> > consume_skb(skb);
> >
> > return ret;
> >
> > So it looks like recvmsg actually always consumes the skb.
> > So I was wrong when I said you need to kfree it after
> > recv msg, and your original patch was good.
> >
> > Jason, what do you think?
> >
>
> tun_recvmsg() has the following check:
>
> static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t
> total_len,
> int flags)
> {
> struct tun_file *tfile = container_of(sock, struct tun_file, socket);
> struct tun_struct *tun = __tun_get(tfile);
> int ret;
>
> if (!tun)
> return -EBADFD;
>
> if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
> ret = -EINVAL;
> goto out;
> }
>
> And tun_do_read() has:
>
> if (!iov_iter_count(to))
> return 0;
>
> So I think we need free skb in those cases.
>
> Thanks
So it's a mess for callers. Let's free within tun then?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-11-30 13:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171129195138.63512ead@cakuba.netronome.com>
On Wed, Nov 29, 2017 at 07:51:38PM -0800, Jakub Kicinski wrote:
> On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> > > to ease configuration of a VM and that would enable live migration of
> > > passthrough network SR-IOV devices.
> > >
> > > Today we have SR-IOV network devices (VFs) that can be passed into a VM
> > > in order to enable high performance networking direct within the VM.
> > > The problem I am trying to address is that this configuration is
> > > generally difficult to live-migrate. There is documentation [1]
> > > indicating that some OS/Hypervisor vendors will support live migration
> > > of a system with a direct assigned networking device. The problem I
> > > see with these implementations is that the network configuration
> > > requirements that are passed on to the owner of the VM are quite
> > > complicated. You have to set up bonding, you have to configure it to
> > > enslave two interfaces, those interfaces (one is virtio-net, the other
> > > is SR-IOV device/driver like ixgbevf) must support MAC address changes
> > > requested in the VM, and on and on...
> > >
> > > So, on to the proposal:
> > > Modify virtio-net driver to be a single VM network device that
> > > enslaves an SR-IOV network device (inside the VM) with the same MAC
> > > address. This would cause the virtio-net driver to appear and work like
> > > a simplified bonding/team driver. The live migration problem would be
> > > solved just like today's bonding solution, but the VM user's networking
> > > config would be greatly simplified.
> > >
> > > At it's simplest, it would appear something like this in the VM.
> > >
> > > ==========
> > > = vnet0 =
> > > =============
> > > (virtio- = |
> > > net) = |
> > > = ==========
> > > = = ixgbef =
> > > ========== ==========
> > >
> > > (forgive the ASCII art)
> > >
> > > The fast path traffic would prefer the ixgbevf or other SR-IOV device
> > > path, and fall back to virtio's transmit/receive when migrating.
> > >
> > > Compared to today's options this proposal would
> > > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > > speeds
> > > 2) simplify end user configuration in the VM (most if not all of the
> > > set up to enable migration would be done in the hypervisor)
> > > 3) allow live migration via a simple link down and maybe a PCI
> > > hot-unplug of the SR-IOV device, with failover to the virtio-net
> > > driver core
> > > 4) allow vendor agnostic hardware acceleration, and live migration
> > > between vendors if the VM os has driver support for all the required
> > > SR-IOV devices.
> > >
> > > Runtime operation proposed:
> > > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > > - virtio-net finds other NICs that match it's MAC address by
> > > both examining existing interfaces, and sets up a new device notifier
> > > - virtio-net enslaves the first NIC with the same MAC address
> > > - virtio-net brings up the slave, and makes it the "preferred" path
> > > - virtio-net follows the behavior of an active backup bond/team
> > > - virtio-net acts as the interface to the VM
> > > - live migration initiates
> > > - link goes down on SR-IOV, or SR-IOV device is removed
> > > - failover to virtio-net as primary path
> > > - migration continues to new host
> > > - new host is started with virio-net as primary
> > > - if no SR-IOV, virtio-net stays primary
> > > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > > - virtio-net notices new NIC and starts over at enslave step above
> > >
> > > Future ideas (brainstorming):
> > > - Optimize Fast east-west by having special rules to direct east-west
> > > traffic through virtio-net traffic path
> > >
> > > Thanks for reading!
> > > Jesse
> >
> > Cc netdev.
> >
> > Interesting, and this method is actually used by netvsc now:
> >
> > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > Author: stephen hemminger <stephen@networkplumber.org>
> > Date: Tue Aug 1 19:58:53 2017 -0700
> >
> > netvsc: transparent VF management
> >
> > This patch implements transparent fail over from synthetic NIC to
> > SR-IOV virtual function NIC in Hyper-V environment. It is a better
> > alternative to using bonding as is done now. Instead, the receive and
> > transmit fail over is done internally inside the driver.
> >
> > Using bonding driver has lots of issues because it depends on the
> > script being run early enough in the boot process and with sufficient
> > information to make the association. This patch moves all that
> > functionality into the kernel.
> >
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > If my understanding is correct there's no need to for any extension of
> > virtio spec. If this is true, maybe you can start to prepare the patch?
>
> IMHO this is as close to policy in the kernel as one can get. User
> land has all the information it needs to instantiate that bond/team
> automatically.
It does have this info (MAC addresses match) but where's the policy
here? IMHO the policy has been set by the hypervisor already.
From hypervisor POV adding passthrough is a commitment not to migrate
until guest stops using the passthrough device.
Within the guest, the bond is required for purely functional reasons - just to
maintain a link up since we know SRIOV will will go away. Maintaining an
uninterrupted connection is not a policy - it's what networking is
about.
> In fact I'm trying to discuss this with NetworkManager
> folks and Red Hat right now:
>
> https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
I thought we should do it too, for a while.
But now, I think that the real issue is this: kernel exposes what looks
like two network devices to userspace, but in fact it is just one
backend device, just exposed by hypervisor in a weird way for
compatibility reasons.
For example you will not get a better reliability or throughput by using
both of them - the only bonding mode that makes sense is fail over. As
another example, if the underlying physical device lost its link, trying
to use virtio won't help - it's only useful when the passthrough device
is gone for good. As another example, there is no point in not
configuring a bond. As a last example, depending on how the backend is
configured, virtio might not even work when the pass-through device is
active.
So from that point of view, showing two network devices to userspace is
a bug that we are asking userspace to work around.
> Can we flip the argument and ask why is the kernel supposed to be
> responsible for this?
Because if we show a single device to userspace the number of
misconfigured guests will go down, and we won't lose any useful
flexibility.
> It's not like we run DHCP out of the kernel
> on new interfaces...
Because one can set up a static IP, IPv6 doesn't always need DHCP, etc.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-11-30 14:11 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <CAEHy93LneRXNWo64uyN2dfyDX6t18WN+YwVF2RXXLRnr0=Jhww@mail.gmail.com>
On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
> Re. problem #2:
> Indeed the best way to address it seems to be to enslave the VF driver
> netdev under a persistent anchor netdev.
> And it's indeed desired to allow (but not enforce) PV netdev and VF
> netdev to work in conjunction.
> And it's indeed desired that this enslavement logic work out-of-the box.
> But in case of PV+VF some configurable policies must be in place (and
> they'd better be generic rather than differ per PV technology).
> For example - based on which characteristics should the PV+VF coupling
> be done? netvsc uses MAC address, but that might not always be the
> desire.
It's a policy but not guest userspace policy.
The hypervisor certainly knows.
Are you concerned that someone might want to create two devices with the
same MAC for an unrelated reason? If so, hypervisor could easily set a
flag in the virtio device to say "this is a backup, use MAC to find
another device".
> Another example - when to use PV and when to use VF? One may want to
> use PV only if VF is gone/down, while others may want to use PV also
> when VF is up, e.g for multicasting.
There are a bunch of configurations where these two devices share
the same physical backend. In that case there is no point
in multicasting through both devices, in fact, PV might
not even work at all when passthrough is active.
IMHO these cases are what's worth handling in the kernel.
When there are two separate backends, we are getting into policy and
it's best to leave this to userspace (and it's unlikely network manager
will automatically do the right thing here, either).
> I think the right way to address it is to have a new dedicated module
> for this purpose.
> Have it automatically enslave PV and VF netdevs according to user
> configured policy. Enslave the VF even if there is no PV device at
> all.
>
> This way we get:
> 1) Optimal migration performance
This remains to be proved.
> 2) A PV agnostic (VM may be migrated even from one PV technology to
> another)
Yes - in theory kvm could expose both a virtio and a hyperv device. But what
would be the point? Just do the abstraction in the hypervisor.
> and HW device agnostic solution
That's useful but I don't think we need to involve userspace for that.
HW abstraction is kernel's job.
> A dedicated generic module will also enforce a lower common
> denominator of guest netdev features, preventing migration dependency
> on source/guest machine capabilities.
If all we are discussing is where this code should live, then I
do not really care. Let's implement it in virtio, then if we
find we have a lot of common we can factor it out.
> 3) Out-of-the box solution yet with generic methods for policy setting
>
>
> Thanks
There's no real policy that guest can set though. All setting happens
on the hypervisor side.
--
MST
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-11-30 14:14 UTC (permalink / raw)
To: Jason Wang
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <0fb552d4-1bfc-e130-4fc1-87b83873916d@redhat.com>
On Thu, Nov 30, 2017 at 11:29:56AM +0800, Jason Wang wrote:
> If my understanding is correct there's no need to for any extension of
> virtio spec.
There appears to be a concern that some existing configurations
might use same MAC for an unrelated reason. Not sure what
that could be, but for sure, we could add a feature flag.
That needs to be approved by the virtio TC, but it's
just a single line in the spec no big deal, we can help here.
> If this is true, maybe you can start to prepare the patch?
Yes, please do. We can add a safeguard of a feature bit on top.
--
MST
^ permalink raw reply
* Re: [PATCH v18 05/10] xbitmap: add more operations
From: Matthew Wilcox @ 2017-11-30 14:39 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, quan.xu,
nilal, riel, cornelia.huck, mhocko, linux-kernel, amit.shah,
pbonzini, akpm, mgorman
In-Reply-To: <201711302235.FAJ57385.OFJHOVQOFtMSFL@I-love.SAKURA.ne.jp>
On Thu, Nov 30, 2017 at 10:35:03PM +0900, Tetsuo Handa wrote:
> According to xb_set_bit(), it seems to me that we are trying to avoid memory allocation
> for "struct ida_bitmap" when all set bits within a 1024-bits bitmap reside in the first
> 61 bits.
>
> But does such saving help? Is there characteristic bias that majority of set bits resides
> in the first 61 bits, for "bit" is "unsigned long" which holds a page number (isn't it)?
> If no such bias, wouldn't eliminating radix_tree_exception() case and always storing
> "struct ida_bitmap" simplifies the code (and make the processing faster)?
It happens all the time. The vast majority of users of the IDA set
low bits. Also, it's the first 62 bits -- going up to 63 bits with the
XArray rewrite.
I do plan to redo the xbitmap on top of the XArray; I'm just trying to
get the XArray merged first. The IDA and xbitmap code will share much
more code when that happens.
^ permalink raw reply
* RE: [PATCH v18 07/10] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wang, Wei W @ 2017-11-30 16:25 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, willy@infradead.org, quan.xu@aliyun.com,
nilal@redhat.com, riel@redhat.com, cornelia.huck@de.ibm.com,
mhocko@kernel.org, linux-kernel@vger.kernel.org, amit.shah
In-Reply-To: <201711301935.EHF86450.MSFLOOHFJtFOQV@I-love.SAKURA.ne.jp>
On Thursday, November 30, 2017 6:36 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
> > +static inline int xb_set_page(struct virtio_balloon *vb,
> > + struct page *page,
> > + unsigned long *pfn_min,
> > + unsigned long *pfn_max)
> > +{
> > + unsigned long pfn = page_to_pfn(page);
> > + int ret;
> > +
> > + *pfn_min = min(pfn, *pfn_min);
> > + *pfn_max = max(pfn, *pfn_max);
> > +
> > + do {
> > + ret = xb_preload_and_set_bit(&vb->page_xb, pfn,
> > + GFP_NOWAIT | __GFP_NOWARN);
>
> It is a bit of pity that __GFP_NOWARN here is applied to only xb_preload().
> Memory allocation by xb_set_bit() will after all emit warnings. Maybe
>
> xb_init(&vb->page_xb);
> vb->page_xb.gfp_mask |= __GFP_NOWARN;
>
> is tolerable? Or, unconditionally apply __GFP_NOWARN at xb_init()?
>
Please have a check this one: radix_tree_node_alloc()
In our case, I think the code path goes to
if (!gfpflags_allow_blocking(gfp_mask) && !in_interrupt()) {
...
ret = kmem_cache_alloc(radix_tree_node_cachep,
gfp_mask | __GFP_NOWARN);
...
goto out;
}
So I think the __GFP_NOWARN is already there.
> static inline void xb_init(struct xb *xb)
> {
> INIT_RADIX_TREE(&xb->xbrt, IDR_RT_MARKER | GFP_NOWAIT);
> }
>
> > + } while (unlikely(ret == -EAGAIN));
> > +
> > + return ret;
> > +}
> > +
>
>
>
> > @@ -172,11 +283,18 @@ static unsigned fill_balloon(struct virtio_balloon
> *vb, size_t num)
> > vb->num_pfns = 0;
> >
> > while ((page = balloon_page_pop(&pages))) {
> > + if (use_sg) {
> > + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) {
> > + __free_page(page);
> > + break;
>
> You cannot "break;" without consuming all pages in "pages".
Right, I think it should be "continue" here. Thanks.
>
> > + }
> > + } else {
> > + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > + }
> > +
> > balloon_page_enqueue(&vb->vb_dev_info, page);
> >
> > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > -
> > - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > if (!virtio_has_feature(vb->vdev,
> >
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
>
>
>
> > @@ -212,9 +334,12 @@ static unsigned leak_balloon(struct virtio_balloon
> *vb, size_t num)
> > struct page *page;
> > struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> > LIST_HEAD(pages);
> > + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
>
> You can pass use_sg as an argument to leak_balloon(). Then, you won't need
> to define leak_balloon_sg_oom(). Since xbitmap allocation does not use
> __GFP_DIRECT_RECLAIM, it is safe to reuse leak_balloon() for OOM path.
> Just be sure to pass use_sg == false because memory allocation for use_sg ==
> true likely fails when called from OOM path. (But trying use_sg == true for
> OOM path and then fallback to use_sg == false is not bad?)
>
But once the SG mechanism is in use, we cannot use the non-SG mechanism, because the interface between the guest and host is not the same for SG and non-SG. Methods to make the host support both mechanisms at the same time would complicate the interface and implementation.
I also think the old non-SG mechanism should be deprecated to use since its implementation isn't perfect in some sense, e.g. it balloons pages using outbuf which implies that no changes are allowed to the balloon pages and this isn't true for ballooning. The new mechanism (SG) has changed it to use inbuf.
So I think using leak_balloon_sg_oom() would be better. Is there any reason that we couldn't use it?
Best,
Wei
^ permalink raw reply
* Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
From: Michael S. Tsirkin @ 2017-11-30 19:38 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1511963726-34070-7-git-send-email-wei.w.wang@intel.com>
On Wed, Nov 29, 2017 at 09:55:22PM +0800, Wei Wang wrote:
> Current virtqueue_add API implementation is based on the scatterlist
> struct, which uses kaddr. This is inadequate to all the use case of
> vring. For example:
> - Some usages don't use IOMMU, in this case the user can directly pass
> in a physical address in hand, instead of going through the sg
> implementation (e.g. the VIRTIO_BALLOON_F_SG feature)
> - Sometimes, a guest physical page may not have a kaddr (e.g. high
> memory) but need to use vring (e.g. the VIRTIO_BALLOON_F_FREE_PAGE_VQ
> feature)
>
> The new API virtqueue_add_one_desc enables the caller to assign a vring
> desc with a physical address and len. Also, factor out the common code
> with virtqueue_add in vring_set_avail.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
You previously managed without this patch, and it's preferable
IMHO since this patchset is already too big.
I don't really understand what is wrong with virtio_add_sgs + sg_set_page.
I don't think is assumes a kaddr.
> ---
> drivers/virtio/virtio_ring.c | 94 +++++++++++++++++++++++++++++++++++---------
> include/linux/virtio.h | 6 +++
> 2 files changed, 81 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index eb30f3e..0b87123 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -257,6 +257,79 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
> return desc;
> }
>
> +static void vring_set_avail(struct virtqueue *_vq,
> + unsigned int i)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + unsigned int avail;
> +
> + avail = vq->avail_idx_shadow & (vq->vring.num - 1);
> + vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, i);
> +
> + /*
> + * Descriptors and available array need to be set before we expose the
> + * new available array entries.
> + */
> + virtio_wmb(vq->weak_barriers);
> + vq->avail_idx_shadow++;
> + vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev,
> + vq->avail_idx_shadow);
> + vq->num_added++;
> +
> + pr_debug("Added buffer head %i to %p\n", i, vq);
> +
> + /*
> + * This is very unlikely, but theoretically possible. Kick
> + * just in case.
> + */
> + if (unlikely(vq->num_added == (1 << 16) - 1))
> + virtqueue_kick(_vq);
> +}
> +
> +int virtqueue_add_one_desc(struct virtqueue *_vq,
> + uint64_t addr,
> + uint32_t len,
> + bool in_desc,
> + void *data)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_desc *desc;
> + unsigned int i;
> +
> + START_USE(vq);
> + BUG_ON(data == NULL);
> +
> + if (unlikely(vq->broken)) {
> + END_USE(vq);
> + return -EIO;
> + }
> +
> + if (_vq->num_free < 1) {
> + END_USE(vq);
> + return -ENOSPC;
> + }
> +
> + i = vq->free_head;
> + desc = &vq->vring.desc[i];
> + desc->addr = cpu_to_virtio64(_vq->vdev, addr);
> + desc->len = cpu_to_virtio32(_vq->vdev, len);
> + if (in_desc)
> + desc->flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_WRITE);
> + else
> + desc->flags = 0;
> + vq->desc_state[i].data = data;
> + vq->desc_state[i].indir_desc = NULL;
> + vq->free_head = virtio16_to_cpu(_vq->vdev, vq->vring.desc[i].next);
> + _vq->num_free--;
> +
> + vring_set_avail(_vq, i);
> +
> + END_USE(vq);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_add_one_desc);
> +
> static inline int virtqueue_add(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> unsigned int total_sg,
> @@ -269,7 +342,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
> struct scatterlist *sg;
> struct vring_desc *desc;
> - unsigned int i, n, avail, descs_used, uninitialized_var(prev), err_idx;
> + unsigned int i, n, descs_used, uninitialized_var(prev), err_idx;
> int head;
> bool indirect;
>
> @@ -395,26 +468,9 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> else
> vq->desc_state[head].indir_desc = ctx;
>
> - /* Put entry in available array (but don't update avail->idx until they
> - * do sync). */
> - avail = vq->avail_idx_shadow & (vq->vring.num - 1);
> - vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
> -
> - /* Descriptors and available array need to be set before we expose the
> - * new available array entries. */
> - virtio_wmb(vq->weak_barriers);
> - vq->avail_idx_shadow++;
> - vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
> - vq->num_added++;
> -
> - pr_debug("Added buffer head %i to %p\n", head, vq);
> + vring_set_avail(_vq, head);
> END_USE(vq);
>
> - /* This is very unlikely, but theoretically possible. Kick
> - * just in case. */
> - if (unlikely(vq->num_added == (1 << 16) - 1))
> - virtqueue_kick(_vq);
> -
> return 0;
>
> unmap_release:
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 988c735..1d89996 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -35,6 +35,12 @@ struct virtqueue {
> void *priv;
> };
>
> +int virtqueue_add_one_desc(struct virtqueue *_vq,
> + uint64_t addr,
> + uint32_t len,
> + bool in_desc,
> + void *data);
> +
> int virtqueue_add_outbuf(struct virtqueue *vq,
> struct scatterlist sg[], unsigned int num,
> void *data,
> --
> 2.7.4
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jakub Kicinski @ 2017-11-30 20:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171130153522-mutt-send-email-mst@kernel.org>
On Thu, 30 Nov 2017 15:54:40 +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 07:51:38PM -0800, Jakub Kicinski wrote:
> > On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > > Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> > > > to ease configuration of a VM and that would enable live migration of
> > > > passthrough network SR-IOV devices.
> > > >
> > > > Today we have SR-IOV network devices (VFs) that can be passed into a VM
> > > > in order to enable high performance networking direct within the VM.
> > > > The problem I am trying to address is that this configuration is
> > > > generally difficult to live-migrate. There is documentation [1]
> > > > indicating that some OS/Hypervisor vendors will support live migration
> > > > of a system with a direct assigned networking device. The problem I
> > > > see with these implementations is that the network configuration
> > > > requirements that are passed on to the owner of the VM are quite
> > > > complicated. You have to set up bonding, you have to configure it to
> > > > enslave two interfaces, those interfaces (one is virtio-net, the other
> > > > is SR-IOV device/driver like ixgbevf) must support MAC address changes
> > > > requested in the VM, and on and on...
> > > >
> > > > So, on to the proposal:
> > > > Modify virtio-net driver to be a single VM network device that
> > > > enslaves an SR-IOV network device (inside the VM) with the same MAC
> > > > address. This would cause the virtio-net driver to appear and work like
> > > > a simplified bonding/team driver. The live migration problem would be
> > > > solved just like today's bonding solution, but the VM user's networking
> > > > config would be greatly simplified.
> > > >
> > > > At it's simplest, it would appear something like this in the VM.
> > > >
> > > > ==========
> > > > = vnet0 =
> > > > =============
> > > > (virtio- = |
> > > > net) = |
> > > > = ==========
> > > > = = ixgbef =
> > > > ========== ==========
> > > >
> > > > (forgive the ASCII art)
> > > >
> > > > The fast path traffic would prefer the ixgbevf or other SR-IOV device
> > > > path, and fall back to virtio's transmit/receive when migrating.
> > > >
> > > > Compared to today's options this proposal would
> > > > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > > > speeds
> > > > 2) simplify end user configuration in the VM (most if not all of the
> > > > set up to enable migration would be done in the hypervisor)
> > > > 3) allow live migration via a simple link down and maybe a PCI
> > > > hot-unplug of the SR-IOV device, with failover to the virtio-net
> > > > driver core
> > > > 4) allow vendor agnostic hardware acceleration, and live migration
> > > > between vendors if the VM os has driver support for all the required
> > > > SR-IOV devices.
> > > >
> > > > Runtime operation proposed:
> > > > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > > > - virtio-net finds other NICs that match it's MAC address by
> > > > both examining existing interfaces, and sets up a new device notifier
> > > > - virtio-net enslaves the first NIC with the same MAC address
> > > > - virtio-net brings up the slave, and makes it the "preferred" path
> > > > - virtio-net follows the behavior of an active backup bond/team
> > > > - virtio-net acts as the interface to the VM
> > > > - live migration initiates
> > > > - link goes down on SR-IOV, or SR-IOV device is removed
> > > > - failover to virtio-net as primary path
> > > > - migration continues to new host
> > > > - new host is started with virio-net as primary
> > > > - if no SR-IOV, virtio-net stays primary
> > > > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > > > - virtio-net notices new NIC and starts over at enslave step above
> > > >
> > > > Future ideas (brainstorming):
> > > > - Optimize Fast east-west by having special rules to direct east-west
> > > > traffic through virtio-net traffic path
> > > >
> > > > Thanks for reading!
> > > > Jesse
> > >
> > > Cc netdev.
> > >
> > > Interesting, and this method is actually used by netvsc now:
> > >
> > > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > > Author: stephen hemminger <stephen@networkplumber.org>
> > > Date: Tue Aug 1 19:58:53 2017 -0700
> > >
> > > netvsc: transparent VF management
> > >
> > > This patch implements transparent fail over from synthetic NIC to
> > > SR-IOV virtual function NIC in Hyper-V environment. It is a better
> > > alternative to using bonding as is done now. Instead, the receive and
> > > transmit fail over is done internally inside the driver.
> > >
> > > Using bonding driver has lots of issues because it depends on the
> > > script being run early enough in the boot process and with sufficient
> > > information to make the association. This patch moves all that
> > > functionality into the kernel.
> > >
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > If my understanding is correct there's no need to for any extension of
> > > virtio spec. If this is true, maybe you can start to prepare the patch?
> >
> > IMHO this is as close to policy in the kernel as one can get. User
> > land has all the information it needs to instantiate that bond/team
> > automatically.
>
> It does have this info (MAC addresses match) but where's the policy
> here? IMHO the policy has been set by the hypervisor already.
> From hypervisor POV adding passthrough is a commitment not to migrate
> until guest stops using the passthrough device.
>
> Within the guest, the bond is required for purely functional reasons - just to
> maintain a link up since we know SRIOV will will go away. Maintaining an
> uninterrupted connection is not a policy - it's what networking is
> about.
>
> > In fact I'm trying to discuss this with NetworkManager
> > folks and Red Hat right now:
> >
> > https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
>
> I thought we should do it too, for a while.
>
> But now, I think that the real issue is this: kernel exposes what looks
> like two network devices to userspace, but in fact it is just one
> backend device, just exposed by hypervisor in a weird way for
> compatibility reasons.
>
> For example you will not get a better reliability or throughput by using
> both of them - the only bonding mode that makes sense is fail over.
Yes, I'm talking about fail over.
> As another example, if the underlying physical device lost its link, trying
> to use virtio won't help - it's only useful when the passthrough device
> is gone for good. As another example, there is no point in not
> configuring a bond. As a last example, depending on how the backend is
> configured, virtio might not even work when the pass-through device is
> active.
>
> So from that point of view, showing two network devices to userspace is
> a bug that we are asking userspace to work around.
I'm confused by what you're saying here. IIRC the question is whether
we expose 2 netdevs or 3. There will always be a virtio netdev and a
VF netdev. I assume you're not suggesting hiding the VF netdev. So
the question is do we expose a VF netdev and a combo virtio netdev
which is also a bond or do we expose a VF netdev a virtio netdev, and a
active/passive bond/team which is a well understood and architecturally
correct construct.
> > Can we flip the argument and ask why is the kernel supposed to be
> > responsible for this?
>
> Because if we show a single device to userspace the number of
> misconfigured guests will go down, and we won't lose any useful
> flexibility.
Again, single device?
> > It's not like we run DHCP out of the kernel
> > on new interfaces...
>
> Because one can set up a static IP, IPv6 doesn't always need DHCP, etc.
But we don't handle LACP, etc.
Look, as much as I don't like this, I'm not going to argue about this to
death. I just find it very dishonest to claim kernel *has to* do it,
when no one seem to have made any honest attempts to solve this in user
space for the last 10 years :/
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v18 01/10] idr: add #include <linux/bug.h>
From: Andrew Morton @ 2017-11-30 21:49 UTC (permalink / raw)
To: Matthew Wilcox
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, quan.xu, nilal, riel, cornelia.huck, mhocko,
Masahiro Yamada, linux-kernel, amit.shah, pbonzini, mgorman
In-Reply-To: <20171130005817.GA14785@bombadil.infradead.org>
On Wed, 29 Nov 2017 16:58:17 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Nov 29, 2017 at 09:55:17PM +0800, Wei Wang wrote:
> > The <linux/bug.h> was removed from radix-tree.h by the following commit:
> > f5bba9d11a256ad2a1c2f8e7fc6aabe6416b7890.
> >
> > Since that commit, tools/testing/radix-tree/ couldn't pass compilation
> > due to: tools/testing/radix-tree/idr.c:17: undefined reference to
> > WARN_ON_ONCE. This patch adds the bug.h header to idr.h to solve the
> > issue.
>
> Thanks; I sent this same patch out yesterday.
>
> Unfortunately, you didn't cc the author of this breakage, Masahiro Yamada.
> I want to highlight that these kinds of header cleanups are risky,
> and very low reward. I really don't want to see patches going all over
> the tree randomly touching header files. If we've got a real problem
> to solve, then sure. But I want to see a strong justification for any
> more header file cleanups.
I tend to disagree. We accumulate more and more cruft over time so it
is good to be continually hacking away at it. These little build
breaks happen occasionally but they are trivially and quickly fixed.
If a small minority of these cleanups require a followup patch which
consumes a global ten person minutes then that seems an acceptable
price to pay. Says the guy who pays most of that price :)
^ permalink raw reply
* Re: [PATCH 02/13] fbdev: add remove_conflicting_pci_framebuffers()
From: Sudip Mukherjee @ 2017-11-30 23:49 UTC (permalink / raw)
To: Daniel Vetter
Cc: Teddy Wang, David Airlie, Greg KH, amd-gfx,
Michał Mirosław, dri-devel, Alex Deucher,
virtualization, Christian König
In-Reply-To: <20171129095634.ymeld4l7zaehbuvn@phenom.ffwll.local>
Hi Daniel,
On Wed, Nov 29, 2017 at 10:56:34AM +0100, Daniel Vetter wrote:
> On Tue, Nov 28, 2017 at 12:30:30PM +0000, Sudip Mukherjee wrote:
> > On Tue, Nov 28, 2017 at 12:32:38PM +0100, Greg KH wrote:
> > > On Tue, Nov 28, 2017 at 11:22:17AM +0100, Daniel Vetter wrote:
> > > > On Mon, Nov 27, 2017 at 08:52:19PM +0000, Sudip Mukherjee wrote:
> > > > > On Mon, Nov 27, 2017 at 11:27:59AM +0100, Daniel Vetter wrote:
> > > > > > On Fri, Nov 24, 2017 at 06:53:31PM +0100, Michał Mirosław wrote:
> > > > > > > Almost all drivers using remove_conflicting_framebuffers() wrap it with
> > > > > > > the same code. Extract common part from PCI drivers into separate
> > > > > > > remove_conflicting_pci_framebuffers().
> > > > > > >
<snip>
> > > >
> > > > Greg?
> > >
> > > Yes, if no one is working to get it out of staging, that means no one
> > > cares about it, and it needs to be removed from the tree.
> >
> > Agreed. I was not working on getting it out of staging as there is no
> > place for it to go.
> > But, Teddy Wang told me that they have a working drm driver for it, but
> > it is not atomic like Daniel was asking for. If it is ok, then I can send
> > in a patch to remove the existing sm750 from staging and add the new sm750
> > drm driver to staging. And after it is ready, we can go ahead with moving
> > it out of staging to drm.
>
> Please keep the todo item that it needs to be converted to atomic. And
> tbh, it's probably faster if you just submit it to dri-devel, assuming you
> have time to work on it. For small drivers we tend to be fairly quick in
> getting them into good enough shape.
I have received the driver from Teddy and pushed it to
https://github.com/sudipm-mukherjee/parport/tree/drm_smi for your first
look into it. It is not even building with next-20171130 and has lots of
build warnings. I will have to do a lot of work on it before I can even
submit it to dri-devel.
Time will be the problem, as this is not part of my day job.
>
> Staging is also a major pain for drm subsystem refactorings, I really,
> really, really prefer we don't add more than the vbox pain we have
> already.
I am hoping that after seeing it, you will agree to have it in staging. :)
--
Regards
Sudip
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-01 5:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Hannes Frederic Sowa, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171130124816.7d534cf3@cakuba.netronome.com>
On Thu, Nov 30, 2017 at 12:48:22PM -0800, Jakub Kicinski wrote:
> On Thu, 30 Nov 2017 15:54:40 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 29, 2017 at 07:51:38PM -0800, Jakub Kicinski wrote:
> > > On Thu, 30 Nov 2017 11:29:56 +0800, Jason Wang wrote:
> > > > On 2017年11月29日 03:27, Jesse Brandeburg wrote:
> > > > > Hi, I'd like to get some feedback on a proposal to enhance virtio-net
> > > > > to ease configuration of a VM and that would enable live migration of
> > > > > passthrough network SR-IOV devices.
> > > > >
> > > > > Today we have SR-IOV network devices (VFs) that can be passed into a VM
> > > > > in order to enable high performance networking direct within the VM.
> > > > > The problem I am trying to address is that this configuration is
> > > > > generally difficult to live-migrate. There is documentation [1]
> > > > > indicating that some OS/Hypervisor vendors will support live migration
> > > > > of a system with a direct assigned networking device. The problem I
> > > > > see with these implementations is that the network configuration
> > > > > requirements that are passed on to the owner of the VM are quite
> > > > > complicated. You have to set up bonding, you have to configure it to
> > > > > enslave two interfaces, those interfaces (one is virtio-net, the other
> > > > > is SR-IOV device/driver like ixgbevf) must support MAC address changes
> > > > > requested in the VM, and on and on...
> > > > >
> > > > > So, on to the proposal:
> > > > > Modify virtio-net driver to be a single VM network device that
> > > > > enslaves an SR-IOV network device (inside the VM) with the same MAC
> > > > > address. This would cause the virtio-net driver to appear and work like
> > > > > a simplified bonding/team driver. The live migration problem would be
> > > > > solved just like today's bonding solution, but the VM user's networking
> > > > > config would be greatly simplified.
> > > > >
> > > > > At it's simplest, it would appear something like this in the VM.
> > > > >
> > > > > ==========
> > > > > = vnet0 =
> > > > > =============
> > > > > (virtio- = |
> > > > > net) = |
> > > > > = ==========
> > > > > = = ixgbef =
> > > > > ========== ==========
> > > > >
> > > > > (forgive the ASCII art)
> > > > >
> > > > > The fast path traffic would prefer the ixgbevf or other SR-IOV device
> > > > > path, and fall back to virtio's transmit/receive when migrating.
> > > > >
> > > > > Compared to today's options this proposal would
> > > > > 1) make virtio-net more sticky, allow fast path traffic at SR-IOV
> > > > > speeds
> > > > > 2) simplify end user configuration in the VM (most if not all of the
> > > > > set up to enable migration would be done in the hypervisor)
> > > > > 3) allow live migration via a simple link down and maybe a PCI
> > > > > hot-unplug of the SR-IOV device, with failover to the virtio-net
> > > > > driver core
> > > > > 4) allow vendor agnostic hardware acceleration, and live migration
> > > > > between vendors if the VM os has driver support for all the required
> > > > > SR-IOV devices.
> > > > >
> > > > > Runtime operation proposed:
> > > > > - <in either order> virtio-net driver loads, SR-IOV driver loads
> > > > > - virtio-net finds other NICs that match it's MAC address by
> > > > > both examining existing interfaces, and sets up a new device notifier
> > > > > - virtio-net enslaves the first NIC with the same MAC address
> > > > > - virtio-net brings up the slave, and makes it the "preferred" path
> > > > > - virtio-net follows the behavior of an active backup bond/team
> > > > > - virtio-net acts as the interface to the VM
> > > > > - live migration initiates
> > > > > - link goes down on SR-IOV, or SR-IOV device is removed
> > > > > - failover to virtio-net as primary path
> > > > > - migration continues to new host
> > > > > - new host is started with virio-net as primary
> > > > > - if no SR-IOV, virtio-net stays primary
> > > > > - hypervisor can hot-add SR-IOV NIC, with same MAC addr as virtio
> > > > > - virtio-net notices new NIC and starts over at enslave step above
> > > > >
> > > > > Future ideas (brainstorming):
> > > > > - Optimize Fast east-west by having special rules to direct east-west
> > > > > traffic through virtio-net traffic path
> > > > >
> > > > > Thanks for reading!
> > > > > Jesse
> > > >
> > > > Cc netdev.
> > > >
> > > > Interesting, and this method is actually used by netvsc now:
> > > >
> > > > commit 0c195567a8f6e82ea5535cd9f1d54a1626dd233e
> > > > Author: stephen hemminger <stephen@networkplumber.org>
> > > > Date: Tue Aug 1 19:58:53 2017 -0700
> > > >
> > > > netvsc: transparent VF management
> > > >
> > > > This patch implements transparent fail over from synthetic NIC to
> > > > SR-IOV virtual function NIC in Hyper-V environment. It is a better
> > > > alternative to using bonding as is done now. Instead, the receive and
> > > > transmit fail over is done internally inside the driver.
> > > >
> > > > Using bonding driver has lots of issues because it depends on the
> > > > script being run early enough in the boot process and with sufficient
> > > > information to make the association. This patch moves all that
> > > > functionality into the kernel.
> > > >
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > > Signed-off-by: David S. Miller <davem@davemloft.net>
> > > >
> > > > If my understanding is correct there's no need to for any extension of
> > > > virtio spec. If this is true, maybe you can start to prepare the patch?
> > >
> > > IMHO this is as close to policy in the kernel as one can get. User
> > > land has all the information it needs to instantiate that bond/team
> > > automatically.
> >
> > It does have this info (MAC addresses match) but where's the policy
> > here? IMHO the policy has been set by the hypervisor already.
> > From hypervisor POV adding passthrough is a commitment not to migrate
> > until guest stops using the passthrough device.
> >
> > Within the guest, the bond is required for purely functional reasons - just to
> > maintain a link up since we know SRIOV will will go away. Maintaining an
> > uninterrupted connection is not a policy - it's what networking is
> > about.
> >
> > > In fact I'm trying to discuss this with NetworkManager
> > > folks and Red Hat right now:
> > >
> > > https://mail.gnome.org/archives/networkmanager-list/2017-November/msg00038.html
> >
> > I thought we should do it too, for a while.
> >
> > But now, I think that the real issue is this: kernel exposes what looks
> > like two network devices to userspace, but in fact it is just one
> > backend device, just exposed by hypervisor in a weird way for
> > compatibility reasons.
> >
> > For example you will not get a better reliability or throughput by using
> > both of them - the only bonding mode that makes sense is fail over.
>
> Yes, I'm talking about fail over.
>
> > As another example, if the underlying physical device lost its link, trying
> > to use virtio won't help - it's only useful when the passthrough device
> > is gone for good. As another example, there is no point in not
> > configuring a bond. As a last example, depending on how the backend is
> > configured, virtio might not even work when the pass-through device is
> > active.
> >
> > So from that point of view, showing two network devices to userspace is
> > a bug that we are asking userspace to work around.
>
> I'm confused by what you're saying here. IIRC the question is whether
> we expose 2 netdevs or 3. There will always be a virtio netdev and a
> VF netdev. I assume you're not suggesting hiding the VF netdev.
Passthrough is a better term, it does not have to be a VF.
All I am saying is these are not two independent devices.
It's a good point - ideally we would hide it completely. Not sure we
can.
> So
> the question is do we expose a VF netdev and a combo virtio netdev
> which is also a bond or do we expose a VF netdev a virtio netdev, and a
> active/passive bond/team which is a well understood and architecturally
> correct construct.
It's a well understood construct for bonding but it is not exactly what
we are dealing with here. What we have is a single device with two ways
to access it.
> > > Can we flip the argument and ask why is the kernel supposed to be
> > > responsible for this?
> >
> > Because if we show a single device to userspace the number of
> > misconfigured guests will go down, and we won't lose any useful
> > flexibility.
>
> Again, single device?
>
> > > It's not like we run DHCP out of the kernel
> > > on new interfaces...
> >
> > Because one can set up a static IP, IPv6 doesn't always need DHCP, etc.
>
> But we don't handle LACP, etc.
>
> Look, as much as I don't like this, I'm not going to argue about this to
> death. I just find it very dishonest to claim kernel *has to* do it,
kernel does not *have* to do it. It might be better to do it in kernel though.
> when no one seem to have made any honest attempts to solve this in user
> space for the last 10 years :/
Each time I tried to convince userspace maintainers I get the kind of
pushback you seem to have encountered. And the reason is that they are
used to manage bonding for actual multiple ethernet ports with all the
complexity this entails. Maybe it wasn't an honest attempt in that I
didn't actually post patches, but there you are.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH net,stable v3] vhost: fix a few skb leaks
From: wexu @ 2017-12-01 5:54 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
From: Wei Xu <wexu@redhat.com>
Matthew found a roughly 40% tcp throughput regression with commit
c67df11f(vhost_net: try batch dequing from skb array) as discussed
in the following thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
This is v3.
v3:
- move freeing skb from vhost to tun/tap recvmsg() to not
confuse the callers.
v2:
- add Matthew as the reporter, thanks matthew.
- moving zero headcount check ahead instead of defer consuming skb
due to jason and mst's comment.
- add freeing skb in favor of recvmsg() fails.
Wei Xu (3):
vhost: fix skb leak in handle_rx()
tun: free skb in early errors
tap: free skb if flags error
drivers/net/tap.c | 6 +++++-
drivers/net/tun.c | 14 +++++++++++---
drivers/vhost/net.c | 20 ++++++++++----------
3 files changed, 26 insertions(+), 14 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: wexu @ 2017-12-01 5:54 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512107669-27572-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
Matthew found a roughly 40% tcp throughput regression with commit
c67df11f(vhost_net: try batch dequing from skb array) as discussed
in the following thread:
https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
Eventually we figured out that it was a skb leak in handle_rx()
when sending packets to the VM. This usually happens when a guest
can not drain out vq as fast as vhost fills in, afterwards it sets
off the traffic jam and leaks skb(s) which occurs as no headcount
to send on the vq from vhost side.
This can be avoided by making sure we have got enough headcount
before actually consuming a skb from the batched rx array while
transmitting, which is simply done by moving checking the zero
headcount a bit ahead.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/vhost/net.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8d626d7..c7bdeb6 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
/* On error, stop handling until the next kick. */
if (unlikely(headcount < 0))
goto out;
- if (nvq->rx_array)
- msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
- /* On overrun, truncate and discard */
- if (unlikely(headcount > UIO_MAXIOV)) {
- iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
- err = sock->ops->recvmsg(sock, &msg,
- 1, MSG_DONTWAIT | MSG_TRUNC);
- pr_debug("Discarded rx packet: len %zd\n", sock_len);
- continue;
- }
/* OK, now we need to know about added descriptors. */
if (!headcount) {
if (unlikely(vhost_enable_notify(&net->dev, vq))) {
@@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
* they refilled. */
goto out;
}
+ if (nvq->rx_array)
+ msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
+ /* On overrun, truncate and discard */
+ if (unlikely(headcount > UIO_MAXIOV)) {
+ iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
+ err = sock->ops->recvmsg(sock, &msg,
+ 1, MSG_DONTWAIT | MSG_TRUNC);
+ pr_debug("Discarded rx packet: len %zd\n", sock_len);
+ continue;
+ }
/* We don't need to be notified again. */
iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
fixup = msg.msg_iter;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/3] tun: free skb in early errors
From: wexu @ 2017-12-01 5:54 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel; +Cc: mjrosato, wexu, mst
In-Reply-To: <1512107669-27572-1-git-send-email-wexu@redhat.com>
From: Wei Xu <wexu@redhat.com>
tun_recvmsg() supports accepting skb by msg_control after
commit ac77cfd4258f ("tun: support receiving skb through msg_control"),
the skb if presented should be freed within the function, otherwise it
would be leaked.
Signed-off-by: Wei Xu <wexu@redhat.com>
Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
drivers/net/tun.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6a7bde9..5563430 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2067,14 +2067,17 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
{
struct tun_file *tfile = container_of(sock, struct tun_file, socket);
struct tun_struct *tun = tun_get(tfile);
+ struct sk_buff *skb = m->msg_control;
int ret;
- if (!tun)
- return -EBADFD;
+ if (!tun) {
+ ret = -EBADFD;
+ goto out_free_skb;
+ }
if (flags & ~(MSG_DONTWAIT|MSG_TRUNC|MSG_ERRQUEUE)) {
ret = -EINVAL;
- goto out;
+ goto out_free_skb;
}
if (flags & MSG_ERRQUEUE) {
ret = sock_recv_errqueue(sock->sk, m, total_len,
@@ -2087,6 +2090,11 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
m->msg_flags |= MSG_TRUNC;
ret = flags & MSG_TRUNC ? ret : total_len;
}
+ goto out;
+
+out_free_skb:
+ if (skb)
+ kfree_skb(skb);
out:
tun_put(tun);
return ret;
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox