* Guest bridge setup variations
From: Arnd Bergmann @ 2009-12-08 16:07 UTC (permalink / raw)
To: virtualization; +Cc: qemu-devel
As promised, here is my small writeup on which setups I feel
are important in the long run for server-type guests. This
does not cover -net user, which is really for desktop kinds
of applications where you do not want to connect into the
guest from another IP address.
I can see four separate setups that we may or may not want to
support, the main difference being how the forwarding between
guests happens:
1. The current setup, with a bridge and tun/tap devices on ports
of the bridge. This is what Gerhard's work on access controls is
focused on and the only option where the hypervisor actually
is in full control of the traffic between guests. CPU utilization should
be highest this way, and network management can be a burden,
because the controls are done through a Linux, libvirt and/or Director
specific interface.
2. Using macvlan as a bridging mechanism, replacing the bridge
and tun/tap entirely. This should offer the best performance on
inter-guest communication, both in terms of throughput and
CPU utilization, but offer no access control for this traffic at all.
Performance of guest-external traffic should be slightly better
than bridge/tap.
3. Doing the bridging in the NIC using macvlan in passthrough
mode. This lowers the CPU utilization further compared to 2,
at the expense of limiting throughput by the performance of
the PCIe interconnect to the adapter. Whether or not this
is a win is workload dependent. Access controls now happen
in the NIC. Currently, this is not supported yet, due to lack of
device drivers, but it will be an important scenario in the future
according to some people.
4. Using macvlan for actual VEPA on the outbound interface.
This is mostly interesting because it makes the network access
controls visible in an external switch that is already managed.
CPU utilization and guest-external throughput should be
identical to 3, but inter-guest latency can only be worse because
all frames go through the external switch.
In case 2 through 4, we have the choice between macvtap and
the raw packet interface for connecting macvlan to qemu.
Raw sockets are better tested right now, while macvtap has
better permission management (i.e. it does not require
CAP_NET_ADMIN). Neither one is upstream though at the
moment. The raw driver only requires qemu patches, while
macvtap requires both a new kernel driver and a trivial change
in qemu.
In all four cases, vhost-net could be used to move the workload
from user space into the kernel, which may be an advantage.
The decision for or against vhost-net is entirely independent of
the other decisions.
Arnd
^ permalink raw reply
* Re: [PATCH] virtio spec: add virtio-blk max sectors feature
From: Avi Kivity @ 2009-12-08 9:50 UTC (permalink / raw)
To: Avishay Traeger1; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <OFD98AF5A9.4296A91C-ONC2257686.00353E3A-C2257686.00357FE1@il.ibm.com>
On 12/08/2009 11:44 AM, Avishay Traeger1 wrote:
>
>> Thanks!
>> Rusty.
>>
> Sure. Avi - do you want me to resubmit the kvm and qemu patches?
>
You mean the virtio and qemu patches. That's up to their maintainers
(Rusty and Anthony).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH] virtio spec: add virtio-blk max sectors feature
From: Avishay Traeger1 @ 2009-12-08 9:44 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, Avi Kivity, kvm, qemu-devel
In-Reply-To: <200912081618.00714.rusty@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> wrote on 12/08/2009 07:48:00 AM:
> Avishay; this would be the total sectors in an I/O, as separate from
SIZE_MAX
> (maximum size of any single scatterlist entry) and SEG_MAX (maximum
number of
> scatterlist entries)?
Correct. In the guest virtblk driver, it changes the call to
blk_queue_max_sectors().
> Seems like a reasonable idea; esp if you need it.
I do.
> Thanks!
> Rusty.
Sure. Avi - do you want me to resubmit the kvm and qemu patches?
Thanks,
Avishay
^ permalink raw reply
* Re: [PATCH] virtio spec: add virtio-blk max sectors feature
From: Rusty Russell @ 2009-12-08 5:48 UTC (permalink / raw)
To: Avi Kivity; +Cc: Avishay Traeger1, qemu-devel, kvm, virtualization
In-Reply-To: <4B178BCE.7030003@redhat.com>
On Thu, 3 Dec 2009 08:28:38 pm Avi Kivity wrote:
> On 12/03/2009 10:42 AM, Avishay Traeger1 wrote:
> > I previously submitted a patch to have the guest virtio-blk driver get the
> > value for the maximum I/O size from the host bdrv, rather than assume that
> > there is no limit. Avi requested that I first patch the virtio spec
> > (http://ozlabs.org/~rusty/virtio-spec/). Below is that patch.
> >
> > Please CC me on replies, as I am not subscribed.
> >
> >
>
> Copying Rusty and virtualization@.
Thanks Avi...
Avishay; this would be the total sectors in an I/O, as separate from SIZE_MAX
(maximum size of any single scatterlist entry) and SEG_MAX (maximum number of
scatterlist entries)?
Seems like a reasonable idea; esp if you need it.
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Rusty Russell @ 2009-12-08 1:16 UTC (permalink / raw)
To: Amit Shah; +Cc: Shirley Ma, Michael S. Tsirkin, virtualization
In-Reply-To: <20091204110246.GA16363@amit-x200.redhat.com>
On Fri, 4 Dec 2009 09:32:46 pm Amit Shah wrote:
> On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> > On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > > The console could be flooded with data from the host; handle
> > > > > > this situation by buffering the data.
> > > > >
> > > > > All this complexity makes me really wonder if we should just
> > > > > have the host say the max # ports it will ever use, and just do this
> > > > > really dumbly. Yes, it's a limitation, but it'd be much simpler.
> > > >
> > > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > > And then the buffering will be done inside the vqs themselves?
> > >
> > > Well < 128 (two vqs per port). The config would say (with a feature bit)
> > > how many vq pairs there are.
> >
> > Sure. This was how the previous versions behaved as well.
>
> I forgot one detail:
>
> http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html
>
> Some API changes are needed to pre-declare the number of vqs and the
> selectively enable them as ports get added.
Couldn't we make it that all vqs *exist*, they're just unused unless the
bitmap (or whatever) indicates?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio: set pci bus master enable bit
From: Jesse Barnes @ 2009-12-04 23:10 UTC (permalink / raw)
To: Rusty Russell
Cc: Mark McLoughlin, kvm, Michael S. Tsirkin, Yan Vugenfirer,
linux-kernel, virtualization, Anthony Liguori
In-Reply-To: <200912011034.02251.rusty@rustcorp.com.au>
On Tue, 1 Dec 2009 10:34:01 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Mon, 30 Nov 2009 02:22:01 am Michael S. Tsirkin wrote:
> > As all virtio devices perform DMA, we
> > must enable bus mastering for them to be
> > spec compliant.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/virtio/virtio_pci.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci.c
> > b/drivers/virtio/virtio_pci.c index 28d9cf7..717bae1 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -648,6 +648,7 @@ static int __devinit virtio_pci_probe(struct
> > pci_dev *pci_dev, goto out_req_regions;
> >
> > pci_set_drvdata(pci_dev, vp_dev);
> > + pci_set_master(pci_dev);
>
> I can believe this, but I have no idea if it's right. I've applied
> it, and hope Jesse will comment if there's something wrong with it.
Nope, nothing wrong with that afaict.
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Amit Shah @ 2009-12-04 11:02 UTC (permalink / raw)
To: Rusty Russell; +Cc: Shirley Ma, Michael S. Tsirkin, virtualization
In-Reply-To: <20091203034325.GA5340@amit-x200.redhat.com>
On (Thu) Dec 03 2009 [09:13:25], Amit Shah wrote:
> On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> > On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > > The console could be flooded with data from the host; handle
> > > > > this situation by buffering the data.
> > > >
> > > > All this complexity makes me really wonder if we should just
> > > > have the host say the max # ports it will ever use, and just do this
> > > > really dumbly. Yes, it's a limitation, but it'd be much simpler.
> > >
> > > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > > And then the buffering will be done inside the vqs themselves?
> >
> > Well < 128 (two vqs per port). The config would say (with a feature bit)
> > how many vq pairs there are.
>
> Sure. This was how the previous versions behaved as well.
I forgot one detail:
http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06079.html
Some API changes are needed to pre-declare the number of vqs and the
selectively enable them as ports get added.
How I think this could work is:
<device drv>
probe:
- get max_nr_ports from config_get
- declare the intent to use max_nr_ports * 2 vqs with callbacks
associated with half of them (so this allows us to have 512 vqs with 256
callbacks with the x86 MSI limit.
<virtio_pci>:
request_msix_vectors for all max_nr_ports vectors
new functions to enable / disable vqs
Amit
^ permalink raw reply
* [RFC] macvlan: add tap device backend
From: Arnd Bergmann @ 2009-12-03 17:52 UTC (permalink / raw)
Cc: Edge Virtual Bridging, Herbert Xu, Arnd Bergmann,
Michael S. Tsirkin, Fischer, Anna, netdev, bridge, linux-kernel,
David S. Miller", Anthony Liguori, virtualization,
Stephen Hemminger
In-Reply-To: <1259862720-28432-1-git-send-email-arnd@arndb.de>
This is a second prototype of a new interface into the network
stack, to eventually replace tun/tap and the bridge driver
in certain virtual machine setups. The code has changed
significantly, but the goals still remain.
Background
----------
The 'Edge Virtual Bridging' working group is discussing ways to overcome
the limitation of virtual bridges in hypervisors. One important part
of this is the Virtual Ethernet Port Aggregator (VEPA), as described in
http://www.ieee802.org/1/files/public/docs2009/new-evb-congdon-vepa-modular-0709-v01.pdf
In short, the idea of VEPA is that virtual machines do not communicate
with each other through direct bridging in the hypervisor but only via
an external managed switch that is already well integrated into the data
center, including network filtering, accounting and monitoring. While
we can do most of that efficiently in the Linux bridge code, doing it
externally simplifies the overall setup.
The macvlan code itself now has a bridge-like mode that allows its
slaves to communicate directly, without any of the overhead or the
features of the bridge code.
Related work
------------
The Linux bridge code can now be configured to hairpin mode as the
remote end for macvlan in VEPA mode, thanks to patches from Anna
Fischer. The respective patches to implement VEPA itself in the
bridge code did not get merged because macvlan can do this better.
Or Gerlitz posted a "raw" packet socket backend for qemu to deal with this,
at http://marc.info/?l=qemu-devel&m=124653801212767 and at least three
other people have done a similar functionality independently.
For use with qemu, that raw socket on macvlan should have similar
performance characteristics as macvtap, but would not allow managing
priviledges or doing some of the planned features.
This driver
-----------
While the other approaches should work as well, doing it using a tap
interface should give additional benefits:
* We can keep using the optimizations for jumbo frames that we have put
into the tun/tap driver.
* No need for root permissions that packet sockets need, just use 'ip
link add link type macvtap' to create a new device and give it the right
permissions using udev (using one tap per macvlan netdev).
* support for multiqueue network adapters by opening the tap device
multiple times, using one file descriptor per guest CPU/network
queue/interrupt (if the adapter supports multiple queues on a single
MAC address).
* support for zero-copy receive/transmit using async I/O on the tap device
(if the adapter supports per MAC rx queues).
* The same framework in macvlan can be used to add a third backend
into a future kernel based virtio-net implementation.
This version of the driver does not support any of those features,
but they all appear possible to add ;).
The driver is currently called 'macvtap', which seems to have
stuck, although I would prefer if someone came up with a clearer
name for this thing.
The code is pretty much complete for integration into the
staging tree, but it has seen zero testing since the last
major rework that changed the code to be based around struct
sock as Michael suggested. I'm pretty sure that it does not
work as is, but I don't expect large changes to be necessary.
I'm posting this mainly to solicit feedback. Not sure how
chances for integration into 2.6.33 are, given that the merge
window is opening already.
Cc: Patrick McHardy <kaber@trash.net>
Cc: Stephen Hemminger <shemminger@linux-foundation.org>
Cc: David S. Miller" <davem@davemloft.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Or Gerlitz <ogerlitz@voltaire.com>
Cc: "Fischer, Anna" <anna.fischer@hp.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: bridge@lists.linux-foundation.org
Cc: linux-kernel@vger.kernel.org
Cc: Edge Virtual Bridging <evb@yahoogroups.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/Kconfig | 12 +
drivers/net/Makefile | 1 +
drivers/net/macvtap.c | 561 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 574 insertions(+), 0 deletions(-)
create mode 100644 drivers/net/macvtap.c
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 0bbd5ae..c072027 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -90,6 +90,18 @@ config MACVLAN
To compile this driver as a module, choose M here: the module
will be called macvlan.
+config MACVTAP
+ tristate "MAC-VLAN based tap driver (EXPERIMENTAL)"
+ depends on MACVLAN
+ help
+ This adds a specialized tap character device driver that is based
+ on the MAC-VLAN network interface, called macvtap. A macvtap device
+ can be added in the same way as a macvlan device, using 'type
+ macvlan', and then be accessed through the tap user space interface.
+
+ To compile this driver as a module, choose M here: the module
+ will be called macvtap.
+
config EQUALIZER
tristate "EQL (serial line load balancing) support"
---help---
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 246323d..ae553fb 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -167,6 +167,7 @@ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o
obj-$(CONFIG_DUMMY) += dummy.o
obj-$(CONFIG_IFB) += ifb.o
obj-$(CONFIG_MACVLAN) += macvlan.o
+obj-$(CONFIG_MACVTAP) += macvtap.o
obj-$(CONFIG_DE600) += de600.o
obj-$(CONFIG_DE620) += de620.o
obj-$(CONFIG_LANCE) += lance.o
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
new file mode 100644
index 0000000..c61b504
--- /dev/null
+++ b/drivers/net/macvtap.c
@@ -0,0 +1,561 @@
+#include <linux/etherdevice.h>
+#include <linux/if_macvlan.h>
+#include <linux/nsproxy.h>
+#include <linux/compat.h>
+#include <linux/if_tun.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <linux/cache.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/wait.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+
+#include <net/net_namespace.h>
+#include <net/rtnetlink.h>
+#include <net/sock.h>
+
+/*
+ * A macvtap queue is the central object of this driver, it connects
+ * an open character device to a macvlan interface. There can be
+ * multiple queues on one interface, which map back to queues
+ * implemented in hardware on the underlying device.
+ *
+ * macvtap_proto is used to allocate queues through the sock allocation
+ * mechanism.
+ *
+ * TODO: multiqueue support is currently not implemented, even though
+ * macvtap is basically prepared for that. We will need to add this
+ * here as well as in virtio-net and qemu to get line rate on 10gbit
+ * adapters from a guest.
+ */
+struct macvtap_queue {
+ struct sock sk;
+ wait_queue_head_t wait;
+ struct macvlan_dev *vlan;
+ struct file *file;
+};
+
+static struct proto macvtap_proto = {
+ .name = "macvtap",
+ .owner = THIS_MODULE,
+ .obj_size = sizeof (struct macvtap_queue),
+};
+
+/*
+ * Minor number matches netdev->ifindex, so need a potentially
+ * large value. This also makes it possible to split the
+ * tap functionality out again in the future by offering it
+ * from other drivers besides macvtap. As long as every device
+ * only has one tap, the interface numbers assure that the
+ * device nodes are unique.
+ */
+static unsigned int macvtap_major;
+#define MACVTAP_NUM_DEVS 65536
+static struct class *macvtap_class;
+static struct cdev macvtap_cdev;
+
+/*
+ * RCU usage:
+ * The macvtap_queue is referenced both from the chardev struct file
+ * and from the struct macvlan_dev using rcu_read_lock.
+ *
+ * We never actually update the contents of a macvtap_queue atomically
+ * with RCU but it is used for race-free destruction of a queue when
+ * either the file or the macvlan_dev goes away. Pointers back to
+ * the dev and the file are implicitly valid as long as the queue
+ * exists.
+ *
+ * The callbacks from macvlan are always done with rcu_read_lock held
+ * already, while in the file_operations, we get it ourselves.
+ *
+ * When destroying a queue, we remove the pointers from the file and
+ * from the dev and then synchronize_rcu to make sure no thread is
+ * still using the queue. There may still be references to the struct
+ * sock inside of the queue from outbound SKBs, but these never
+ * reference back to the file or the dev. The data structure is freed
+ * through __sk_free when both our references and any pending SKBs
+ * are gone.
+ *
+ * macvtap_lock is only used to prevent multiple concurrent open()
+ * calls to assign a new vlan->tap pointer. It could be moved into
+ * the macvlan_dev itself but is extremely rarely used.
+ */
+static DEFINE_SPINLOCK(macvtap_lock);
+
+/*
+ * Choose the next free queue, for now there is only one
+ */
+static int macvtap_set_queue(struct net_device *dev, struct file *file,
+ struct macvtap_queue *q)
+{
+ struct macvlan_dev *vlan = netdev_priv(dev);
+ int err = -EBUSY;
+
+ spin_lock(&macvtap_lock);
+ if (rcu_dereference(vlan->tap))
+ goto out;
+
+ err = 0;
+ q->vlan = vlan;
+ rcu_assign_pointer(vlan->tap, q);
+
+ q->file = file;
+ rcu_assign_pointer(file->private_data, q);
+
+out:
+ spin_unlock(&macvtap_lock);
+ return err;
+}
+
+/*
+ * We must destroy each queue exactly once, when either
+ * the netdev or the file go away.
+ *
+ * Using the spinlock makes sure that we don't get
+ * to the queue again after destroying it.
+ *
+ * synchronize_rcu serializes with the packet flow
+ * that uses rcu_read_lock.
+ */
+static void macvtap_del_queue(struct macvtap_queue **qp)
+{
+ struct macvtap_queue *q;
+
+ spin_lock(&macvtap_lock);
+ q = rcu_dereference(*qp);
+ if (!q) {
+ spin_unlock(&macvtap_lock);
+ return;
+ }
+
+ rcu_assign_pointer(q->vlan->tap, NULL);
+ rcu_assign_pointer(q->file->private_data, NULL);
+ spin_unlock(&macvtap_lock);
+
+ synchronize_rcu();
+ sock_put(&q->sk);
+}
+
+/*
+ * Since we only support one queue, just dereference the pointer.
+ */
+static struct macvtap_queue *macvtap_get_queue(struct net_device *dev,
+ struct sk_buff *skb)
+{
+ struct macvlan_dev *vlan = netdev_priv(dev);
+
+ return rcu_dereference(vlan->tap);
+}
+
+static void macvtap_del_queues(struct net_device *dev)
+{
+ struct macvlan_dev *vlan = netdev_priv(dev);
+ macvtap_del_queue(&vlan->tap);
+}
+
+static inline struct macvtap_queue *macvtap_file_get_queue(struct file *file)
+{
+ rcu_read_lock();
+ return rcu_dereference(file->private_data);
+}
+
+static inline void macvtap_file_put_queue(void)
+{
+ rcu_read_unlock();
+}
+
+/*
+ * Forward happens for data that gets sent from one macvlan
+ * endpoint to another one in bridge mode. We just take
+ * the skb and put it into the receive queue.
+ */
+static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
+{
+ struct macvtap_queue *q = macvtap_get_queue(dev, skb);
+ if (!q)
+ return -ENOLINK;
+
+ skb_queue_tail(&q->sk.sk_receive_queue, skb);
+ wake_up(q->sk.sk_sleep);
+ return 0;
+}
+
+/*
+ * Receive is for data from the external interface (lowerdev),
+ * in case of macvtap, we can treat that the same way as
+ * forward, which macvlan cannot.
+ */
+static int macvtap_receive(struct sk_buff *skb)
+{
+ return macvtap_forward(skb->dev, skb);
+}
+
+static int macvtap_newlink(struct net *src_net,
+ struct net_device *dev,
+ struct nlattr *tb[],
+ struct nlattr *data[])
+{
+ struct device *classdev;
+ dev_t devt;
+ int err;
+
+ err = macvlan_common_newlink(src_net, dev, tb, data,
+ macvtap_receive, macvtap_forward);
+ if (err)
+ goto out;
+
+ devt = MKDEV(MAJOR(macvtap_major), dev->ifindex);
+
+ classdev = device_create(macvtap_class, &dev->dev, devt,
+ dev, "tap%d", dev->ifindex);
+ if (IS_ERR(classdev)) {
+ err = PTR_ERR(classdev);
+ macvtap_del_queues(dev);
+ macvlan_dellink(dev, NULL);
+ }
+
+out:
+ return err;
+}
+
+static void macvtap_dellink(struct net_device *dev,
+ struct list_head *head)
+{
+ device_destroy(macvtap_class,
+ MKDEV(MAJOR(macvtap_major), dev->ifindex));
+
+ macvtap_del_queues(dev);
+ macvlan_dellink(dev, head);
+}
+
+static struct rtnl_link_ops macvtap_link_ops __read_mostly = {
+ .kind = "macvtap",
+ .newlink = macvtap_newlink,
+ .dellink = macvtap_dellink,
+};
+
+
+static int macvtap_open(struct inode *inode, struct file *file)
+{
+ struct net *net = current->nsproxy->net_ns;
+ struct net_device *dev = dev_get_by_index(net, iminor(inode));
+ struct macvtap_queue *q;
+ int err;
+
+ err = -ENODEV;
+ if (!dev)
+ goto out;
+
+ /* check if this is a macvtap device */
+ err = -EINVAL;
+ if (dev->rtnl_link_ops != &macvtap_link_ops)
+ goto out;
+
+ err = -ENOMEM;
+ q = (struct macvtap_queue *)sk_alloc(net, AF_UNSPEC, GFP_KERNEL,
+ &macvtap_proto);
+ if (!q)
+ goto out;
+
+ sock_init_data(NULL, &q->sk);
+ init_waitqueue_head(&q->wait);
+ q->sk.sk_sleep = &q->wait;
+
+ err = macvtap_set_queue(dev, file, q);
+ if (err)
+ sock_put(&q->sk);
+
+out:
+ return err;
+}
+
+static int macvtap_release(struct inode *inode, struct file *file)
+{
+ macvtap_del_queue((struct macvtap_queue **)&file->private_data);
+ return 0;
+}
+
+static unsigned int macvtap_poll(struct file *file, poll_table * wait)
+{
+ struct macvtap_queue *q = macvtap_file_get_queue(file);
+ unsigned int mask = POLLERR;
+
+ if (!q)
+ goto out;
+
+ mask = 0;
+ poll_wait(file, &q->wait, wait);
+
+ if (!skb_queue_empty(&q->sk.sk_receive_queue))
+ mask |= POLLIN | POLLRDNORM;
+
+ if (sock_writeable(&q->sk))
+ mask |= POLLOUT | POLLWRNORM;
+
+out:
+ macvtap_file_put_queue();
+ return mask;
+}
+
+/* Get packet from user space buffer */
+static ssize_t macvtap_get_user(struct macvtap_queue *q,
+ const struct iovec *iv, size_t count,
+ int noblock)
+{
+ struct sk_buff *skb;
+ size_t len = count;
+ int err;
+
+ if (unlikely(len < ETH_HLEN))
+ return -EINVAL;
+
+ skb = sock_alloc_send_skb(&q->sk, NET_IP_ALIGN + len, noblock, &err);
+
+ if (!skb) {
+ macvlan_count_rx(q->vlan, 0, false, false);
+ return err;
+ }
+
+ skb_reserve(skb, NET_IP_ALIGN);
+ skb_put(skb, count);
+
+ if (skb_copy_datagram_from_iovec(skb, 0, iv, 0, len)) {
+ macvlan_count_rx(q->vlan, 0, false, false);
+ kfree_skb(skb);
+ return -EFAULT;
+ }
+
+ skb_set_network_header(skb, ETH_HLEN);
+
+ macvlan_start_xmit(skb, q->vlan->dev);
+
+ return count;
+}
+
+static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)
+{
+ struct file *file = iocb->ki_filp;
+ ssize_t result = -ENOLINK;
+ struct macvtap_queue *q = macvtap_file_get_queue(file);
+
+ if (!q)
+ goto out;
+
+ result = macvtap_get_user(q, iv, iov_length(iv, count),
+ file->f_flags & O_NONBLOCK);
+out:
+ macvtap_file_put_queue();
+ return result;
+}
+
+/* Put packet to the user space buffer */
+static ssize_t macvtap_put_user(struct macvtap_queue *q,
+ const struct sk_buff *skb,
+ const struct iovec *iv, int len)
+{
+ struct macvlan_dev *vlan = q->vlan;
+ int ret;
+
+ len = min_t(int, skb->len, len);
+
+ ret = skb_copy_datagram_const_iovec(skb, 0, iv, 0, len);
+
+ macvlan_count_rx(vlan, len, ret == 0, 0);
+
+ return ret ? ret : len;
+}
+
+static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)
+{
+ struct file *file = iocb->ki_filp;
+ struct macvtap_queue *q = macvtap_file_get_queue(file);
+
+ DECLARE_WAITQUEUE(wait, current);
+ struct sk_buff *skb;
+ ssize_t len, ret = 0;
+
+ if (!q) {
+ ret = -ENOLINK;
+ goto out;
+ }
+
+ len = iov_length(iv, count);
+ if (len < 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ add_wait_queue(q->sk.sk_sleep, &wait);
+ while (len) {
+ current->state = TASK_INTERRUPTIBLE;
+
+ /* Read frames from the queue */
+ skb = skb_dequeue(&q->sk.sk_receive_queue);
+ if (!skb) {
+ if (file->f_flags & O_NONBLOCK) {
+ ret = -EAGAIN;
+ break;
+ }
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
+ /* Nothing to read, let's sleep */
+ schedule();
+ continue;
+ }
+ skb_push(skb, ETH_HLEN);
+ ret = macvtap_put_user(q, skb, iv, len);
+ kfree_skb(skb);
+ break;
+ }
+
+ current->state = TASK_RUNNING;
+ remove_wait_queue(q->sk.sk_sleep, &wait);
+
+out:
+ macvtap_file_put_queue();
+ return ret;
+}
+
+/*
+ * provide compatibility with generic tun/tap interface
+ */
+static long macvtap_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct macvtap_queue *q;
+ void __user *argp = (void __user *)arg;
+ struct ifreq __user *ifr = argp;
+ unsigned int __user *up = argp;
+ unsigned int u;
+ int err;
+
+ switch (cmd) {
+ case TUNSETIFF:
+ /* ignore the name, just look at flags */
+ if (get_user(u, &ifr->ifr_flags))
+ return -EFAULT;
+ if (u != (TUN_TAP_DEV | TUN_NO_PI))
+ return -EINVAL;
+ return 0;
+
+ case TUNGETIFF:
+ q = macvtap_file_get_queue(file);
+ if (!q)
+ return -ENOLINK;
+ err = 0;
+ if (copy_to_user(&ifr->ifr_name, q->vlan->dev->name, IFNAMSIZ) ||
+ put_user((TUN_TAP_DEV | TUN_NO_PI), &ifr->ifr_flags))
+ err = -EFAULT;
+ macvtap_file_put_queue();
+ return err;
+
+ case TUNGETFEATURES:
+ u = TUN_TAP_DEV | TUN_NO_PI;
+ if (put_user((TUN_TAP_DEV | TUN_NO_PI), up))
+ return -EFAULT;
+ return 0;
+
+ case TUNSETSNDBUF:
+ /* ignore */
+ return 0;
+
+ case TUNSETOFFLOAD:
+ if (get_user(u, up))
+ return -EFAULT;
+
+ /* let the user check for future flags */
+ if (u & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
+ TUN_F_TSO_ECN | TUN_F_UFO))
+ return -EINVAL;
+
+ /* TODO: add support for these, so far we don't
+ support any offload */
+ if (u & (TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
+ TUN_F_TSO_ECN | TUN_F_UFO))
+ return -EINVAL;
+
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
+#ifdef CONFIG_COMPAT
+static long macvtap_compat_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return macvtap_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+static const struct file_operations macvtap_fops = {
+ .owner = THIS_MODULE,
+ .open = macvtap_open,
+ .release = macvtap_release,
+ .aio_read = macvtap_aio_read,
+ .aio_write = macvtap_aio_write,
+ .poll = macvtap_poll,
+ .llseek = no_llseek,
+ .unlocked_ioctl = macvtap_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = macvtap_compat_ioctl,
+#endif
+};
+
+static int macvtap_init(void)
+{
+ int err;
+
+ err = alloc_chrdev_region(&macvtap_major, 0,
+ MACVTAP_NUM_DEVS, "macvtap");
+ if (err)
+ goto out1;
+
+ cdev_init(&macvtap_cdev, &macvtap_fops);
+ err = cdev_add(&macvtap_cdev, macvtap_major, MACVTAP_NUM_DEVS);
+ if (err)
+ goto out2;
+
+ macvtap_class = class_create(THIS_MODULE, "macvtap");
+ if (IS_ERR(macvtap_class)) {
+ err = PTR_ERR(macvtap_class);
+ goto out3;
+ }
+
+ err = macvlan_link_register(&macvtap_link_ops);
+ if (err)
+ goto out4;
+
+ return 0;
+
+out4:
+ class_unregister(macvtap_class);
+out3:
+ cdev_del(&macvtap_cdev);
+out2:
+ unregister_chrdev_region(macvtap_major, MACVTAP_NUM_DEVS);
+out1:
+ return err;
+}
+module_init(macvtap_init);
+
+static void macvtap_exit(void)
+{
+ rtnl_link_unregister(&macvtap_link_ops);
+ class_unregister(macvtap_class);
+ cdev_del(&macvtap_cdev);
+ unregister_chrdev_region(macvtap_major, MACVTAP_NUM_DEVS);
+}
+module_exit(macvtap_exit);
+
+MODULE_ALIAS_RTNL_LINK("macvtap");
+MODULE_AUTHOR("Arnd Bergmann <arnd@arndb.de>");
+MODULE_LICENSE("GPL");
--
1.6.3.3
^ permalink raw reply related
* [RFC 0/2] macvtap, second try
From: Arnd Bergmann @ 2009-12-03 17:51 UTC (permalink / raw)
Cc: Edge Virtual Bridging, Herbert Xu, Arnd Bergmann,
Michael S. Tsirkin, Fischer, Anna, netdev, bridge, linux-kernel,
David S . Miller ", Anthony Liguori, Patrick Mullaney,
virtualization, Stephen Hemminger
I did not get this ready for the merge window, but people asked what
the status of this is so I'm posting it now to solicit feedback.
The first patch just adds some hooks into macvlan.c and is less
invasive than the previous version. That part should be fine
and I'd like this to get merged into macvlan for 2.6.33 if people
agree that the approach is right.
The second patch adds the driver using it and is not ready for
integration unfortunately, but maybe it could get into staging
anyway. Please look at that patch carefully and provide feedback.
Arnd <><
---
Arnd Bergmann (1):
macvlan: add tap device backend
Patrick Mullaney (1):
macvlan: Allow plugging in additional backends
drivers/net/Kconfig | 12 +
drivers/net/Makefile | 1 +
drivers/net/macvlan.c | 113 ++++-----
drivers/net/macvtap.c | 560 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/if_macvlan.h | 71 ++++++
5 files changed, 693 insertions(+), 64 deletions(-)
create mode 100644 drivers/net/macvtap.c
^ permalink raw reply
* Re: [PATCH 0/3] Split up pv-ops
From: Avi Kivity @ 2009-12-03 15:07 UTC (permalink / raw)
To: Alexander Graf; +Cc: Nick Piggin, Glauber Costa, kvm list, virtualization
In-Reply-To: <4B17D390.5050602@suse.de>
On 12/03/2009 05:04 PM, Alexander Graf wrote:
>> Don't think so. I suggest you copy lkml and Ingo.
>>
> Sending off the complete set again?
Yes.
> Rebased against what?
>
tip's x86/paravirt seems like a good choice (though only one patch is in
there at present).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 0/3] Split up pv-ops
From: Alexander Graf @ 2009-12-03 15:04 UTC (permalink / raw)
To: Avi Kivity; +Cc: Nick Piggin, Glauber Costa, kvm list, virtualization
In-Reply-To: <4B17D290.9040507@redhat.com>
Avi Kivity wrote:
> On 12/03/2009 04:52 PM, Alexander Graf wrote:
>> Alexander Graf wrote:
>>
>>> Paravirt ops is currently only capable of either replacing a lot of
>>> Linux
>>> internal code or none at all. The are users that don't need all of the
>>> possibilities pv-ops delivers though.
>>>
>>> On KVM for example we're perfectly fine not using the PV MMU, thus not
>>> touching any MMU code. That way we don't have to improve pv-ops to
>>> become
>>> fast, we just don't compile the MMU parts in!
>>>
>>> This patchset splits pv-ops into several smaller config options
>>> split by
>>> feature category and then converts the KVM pv-ops code to use only the
>>> bits that are required, lowering overhead.
>>>
>>>
>> So has this ended up in some tree yet?
>>
>
> Don't think so. I suggest you copy lkml and Ingo.
Sending off the complete set again? Rebased against what?
^ permalink raw reply
* Re: [PATCH 0/3] Split up pv-ops
From: Avi Kivity @ 2009-12-03 15:00 UTC (permalink / raw)
To: Alexander Graf; +Cc: Nick Piggin, Glauber Costa, kvm list, virtualization
In-Reply-To: <4B17D0C7.1020805@suse.de>
On 12/03/2009 04:52 PM, Alexander Graf wrote:
> Alexander Graf wrote:
>
>> Paravirt ops is currently only capable of either replacing a lot of Linux
>> internal code or none at all. The are users that don't need all of the
>> possibilities pv-ops delivers though.
>>
>> On KVM for example we're perfectly fine not using the PV MMU, thus not
>> touching any MMU code. That way we don't have to improve pv-ops to become
>> fast, we just don't compile the MMU parts in!
>>
>> This patchset splits pv-ops into several smaller config options split by
>> feature category and then converts the KVM pv-ops code to use only the
>> bits that are required, lowering overhead.
>>
>>
> So has this ended up in some tree yet?
>
Don't think so. I suggest you copy lkml and Ingo.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 0/3] Split up pv-ops
From: Alexander Graf @ 2009-12-03 14:52 UTC (permalink / raw)
To: kvm list; +Cc: Nick Piggin, Glauber Costa, Avi Kivity, virtualization
In-Reply-To: <1258503192-14246-1-git-send-email-agraf@suse.de>
Alexander Graf wrote:
> Paravirt ops is currently only capable of either replacing a lot of Linux
> internal code or none at all. The are users that don't need all of the
> possibilities pv-ops delivers though.
>
> On KVM for example we're perfectly fine not using the PV MMU, thus not
> touching any MMU code. That way we don't have to improve pv-ops to become
> fast, we just don't compile the MMU parts in!
>
> This patchset splits pv-ops into several smaller config options split by
> feature category and then converts the KVM pv-ops code to use only the
> bits that are required, lowering overhead.
>
So has this ended up in some tree yet?
^ permalink raw reply
* Re: [PATCH] virtio spec: add virtio-blk max sectors feature
From: Avi Kivity @ 2009-12-03 9:58 UTC (permalink / raw)
To: Avishay Traeger1; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <OF38B713F7.02DD66D1-ONC2257681.002F3CAD-C2257681.002FDEA6@il.ibm.com>
On 12/03/2009 10:42 AM, Avishay Traeger1 wrote:
> I previously submitted a patch to have the guest virtio-blk driver get the
> value for the maximum I/O size from the host bdrv, rather than assume that
> there is no limit. Avi requested that I first patch the virtio spec
> (http://ozlabs.org/~rusty/virtio-spec/). Below is that patch.
>
> Please CC me on replies, as I am not subscribed.
>
>
Copying Rusty and virtualization@.
> Thank you,
> Avishay
>
> Signed-off-by: Avishay Traeger<avishay@il.ibm.com>
>
> --- virtio-spec-0.8.2.lyx 2009-11-30 16:34:16.000000000 +0200
> +++ virtio-spec-0.8.2-mod.lyx 2009-12-01 16:04:36.000000000 +0200
> @@ -3999,6 +3999,20 @@
> \end_inset
>
> .
> +\change_inserted 0 1259672174
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 0 1259672252
> +VIRTIO_BLK_F_SECTOR_MAX
> +\begin_inset space ~
> +\end_inset
> +
> +(10) Maximum sectors in an I/O.
> +\change_unchanged
> +
> \end_layout
>
> \end_deeper
> @@ -4068,6 +4082,17 @@
> \begin_layout Plain Layout
>
> u32 blk_size;
> +\change_inserted 0 1259672274
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 0 1259672305
> +
> + u32 sectors_max;
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Plain Layout
> @@ -4107,6 +4132,18 @@
> \begin_layout Enumerate
> If the VIRTIO_BLK_F_RO feature is set by the device, any write requests
> will fail.
> +\change_inserted 0 1259672345
> +
> +\end_layout
> +
> +\begin_layout Enumerate
> +
> +\change_inserted 0 1259672454
> +If the VIRTIO_BLK_F_SECTOR_MAX feature is negotiated, the sectors_max
> field
> + should be read to determine the maximum I/O size for the driver to use.
> + No requests should be submitted which go beyond this limit.
> +\change_unchanged
> +
> \end_layout
>
> \begin_layout Section*
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Amit Shah @ 2009-12-03 3:43 UTC (permalink / raw)
To: Rusty Russell; +Cc: Shirley Ma, virtualization
In-Reply-To: <200912030924.23783.rusty@rustcorp.com.au>
On (Thu) Dec 03 2009 [09:24:23], Rusty Russell wrote:
> On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> > On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > > The console could be flooded with data from the host; handle
> > > > this situation by buffering the data.
> > >
> > > All this complexity makes me really wonder if we should just
> > > have the host say the max # ports it will ever use, and just do this
> > > really dumbly. Yes, it's a limitation, but it'd be much simpler.
> >
> > As in make sure the max nr ports is less than 255 and have per-port vqs?
> > And then the buffering will be done inside the vqs themselves?
>
> Well < 128 (two vqs per port). The config would say (with a feature bit)
> how many vq pairs there are.
Sure. This was how the previous versions behaved as well.
> If we want expect sparse port numbers, it can
> also provide an array mapping the vq pairs to the port number (65535 or
> something meaning unused). In practice I suspect 4 or 8 will be plenty
> for now.
>
> That makes hotplugging ports pretty simple: change the array, ping
> config_changed.
The very early versions of this patch used this approach: a nr_ports u32
and a bitmap...
> Or we could abandon numbers and just use the names; I don't mind (we still
> need a way to figure out what ports are active I guess, maybe a bitmap in
> the config?)
... but we dropped that approach and are now just using the 'names' as
a means of identifying ports. Currently, there's only a 'nr_ports' entry
in the config. For hotplug, the entry is bumped by the host and the
guest adds the new ports.
That's definitely simpler than maintaining the bitmap, and also we don't
associate any special meaning to the port numbers themselves, which is a
big plus. (Hot plug after hot unplug can be made to work with a control
message as well, but that support can come in later.)
Looks like we're just going back to what this patch looked like when it
was sent out first.
> But we get the vq buffering and flow control for free with this approach.
I'll see if I can send out this spin today.
Amit
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Rusty Russell @ 2009-12-02 22:54 UTC (permalink / raw)
To: Amit Shah; +Cc: Shirley Ma, virtualization
In-Reply-To: <20091202092406.GB28265@amit-x200.redhat.com>
On Wed, 2 Dec 2009 07:54:06 pm Amit Shah wrote:
> On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> > On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > > The console could be flooded with data from the host; handle
> > > this situation by buffering the data.
> >
> > All this complexity makes me really wonder if we should just
> > have the host say the max # ports it will ever use, and just do this
> > really dumbly. Yes, it's a limitation, but it'd be much simpler.
>
> As in make sure the max nr ports is less than 255 and have per-port vqs?
> And then the buffering will be done inside the vqs themselves?
Well < 128 (two vqs per port). The config would say (with a feature bit)
how many vq pairs there are. If we want expect sparse port numbers, it can
also provide an array mapping the vq pairs to the port number (65535 or
something meaning unused). In practice I suspect 4 or 8 will be plenty
for now.
That makes hotplugging ports pretty simple: change the array, ping
config_changed.
Or we could abandon numbers and just use the names; I don't mind (we still
need a way to figure out what ports are active I guess, maybe a bitmap in
the config?)
But we get the vq buffering and flow control for free with this approach.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Amit Shah @ 2009-12-02 9:24 UTC (permalink / raw)
To: Rusty Russell; +Cc: Shirley Ma, virtualization
In-Reply-To: <200912021414.20206.rusty@rustcorp.com.au>
On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> > The console could be flooded with data from the host; handle
> > this situation by buffering the data.
>
> All this complexity makes me really wonder if we should just
> have the host say the max # ports it will ever use, and just do this
> really dumbly. Yes, it's a limitation, but it'd be much simpler.
As in make sure the max nr ports is less than 255 and have per-port vqs?
And then the buffering will be done inside the vqs themselves?
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -65,6 +65,23 @@ struct ports_device {
> > * interrupt
> > */
> > struct work_struct rx_work;
> > +
> > + struct list_head unused_read_head;
>
> You should name lists after plurals, rather than using "head" which is
> an implementation detail. eg. "queued_inbufs" and below "used_inbufs".
OK.
> Though Shirly Ma was working on a "destroy_bufs" patch which would avoid
> your need for this list at all, AFAICT.
>
> > + /* Return the number of bytes actually copied */
> > + ret = copy_size;
> > + buf->offset += ret;
> > + out_offset += ret;
> > + out_count -= ret;
>
> We don't actually use ret.
In a later patch, when copy_to_user is added, ret will be used. So I
kept it this way to reduce the noise in the diffs later.
> > + if (buf->len - buf->offset == 0) {
>
> I prefer the simpler "if (buf->offset == buf->len)" myself.
Will update.
> > + spin_lock_irqsave(&port->readbuf_list_lock, flags);
> > + list_del(&buf->list);
> > + spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
> > + kfree(buf->buf);
> > + kfree(buf);
>
> Does it become cleaner later to have this in a separate function? Usually
> I prefer matching alloc and free fns.
Adding a function is easy, sure. I should've done that though; something
that got overlooked.
> > +static struct port_buffer *get_buf(size_t buf_size)
> > +{
> > + struct port_buffer *buf;
> > +
> > + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > + if (!buf)
> > + goto out;
> > + buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > + if (!buf->buf) {
> > + kfree(buf);
> > + goto out;
>
> No, that would return non-NULL. I'd stick with the standard multi-part exit:
>
> if (!buf)
> goto fail;
> buf->buf = kzalloc(buf_size, GFP_KERNEL);
> if (!buf->buf)
> goto fail_free_buf;
> buf->len = buf_size;
> return buf;
>
> fail_free_buf:
> kfree(buf);
> fail:
> return NULL;
Ow, indeed.
Thanks!
Amit
^ permalink raw reply
* Re: [PATCH 13/28] virtio: console: Create a buffer pool for sending data to host
From: Amit Shah @ 2009-12-02 9:19 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization
In-Reply-To: <200912021900.35219.rusty@rustcorp.com.au>
On (Wed) Dec 02 2009 [19:00:35], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:36 pm Amit Shah wrote:
> > The old way of sending data to the host was to populate one buffer and
> > then wait till the host consumed it before sending the next chunk of
> > data.
> >
> > Also, there was no support to send large chunks of data.
> >
> > We now maintain a per-device list of buffers that are ready to be
> > passed on to the host.
> >
> > This patch adds support to send big chunks in multiple buffers of
> > PAGE_SIZE each to the host.
> >
> > When the host consumes the data and signals to us via an interrupt, we
> > add the consumed buffer back to our unconsumed list.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> > drivers/char/virtio_console.c | 159 +++++++++++++++++++++++++++++++++-------
> > 1 files changed, 131 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index e8dabae..3111e4c 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -67,9 +67,13 @@ struct ports_device {
> > struct work_struct rx_work;
> >
> > struct list_head unused_read_head;
> > + struct list_head unused_write_head;
> >
> > /* To protect the list of unused read buffers and the in_vq */
> > spinlock_t read_list_lock;
> > +
> > + /* To protect the list of unused write buffers and the out_vq */
> > + spinlock_t write_list_lock;
> > };
>
> Let's simplify this a little with a single "buffer_lock" or such in the
> previous patch.
You mean a common lock for the in_vq and out_vq?
> > + if (!in_count)
> > return 0;
>
> Not necessary: if it happens all we'll do is gratuitously kick the host.
Will drop.
> > + in_offset = 0; /* offset in the user buffer */
> > + spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
> > + while (in_count - in_offset) {
>
> while (in_offset < in_count) seems clearer to me here.
Sure.
> > + copy_size = min(in_count - in_offset, PAGE_SIZE);
>
> Shouldn't you be using buf->size here?
You mean introduce a new field in the buf struct and put the size there?
Yes, I'll do that.
> > + spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
> > +
> > + /*
> > + * Since we're not sure when the host will actually
> > + * consume the data and tell us about it, we have
> > + * to copy the data here in case the caller
> > + * frees the in_buf
> > + */
> > + memcpy(buf->buf, in_buf + in_offset, copy_size);
> > +
> > + buf->len = copy_size;
> > + sg_init_one(sg, buf->buf, buf->len);
> > +
> > + spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
>
> Dropping the lock here seems gratuitous.
In a later patch when we add support for reading from userspace, we'll add
a copy_from_user here, so dropping the lock is done in this patch to
reduce the noise in the diff for easier reviewing.
> > +/*
> > + * This function is only called from the init routine so the spinlock
> > + * for the unused_write_head list isn't taken
> > + */
> > +static void alloc_write_bufs(struct ports_device *portdev)
> > +{
> > + struct port_buffer *buf;
> > + int i;
> > +
> > + for (i = 0; i < 30; i++) {
>
> 30?
Yeah; 30 is an arbitrary number. Basically just enough to have two
simultaneous 'copy /dev/vport0p2 > blah' run without having to return a
number < requested bytes. (libc uses 32k buffers for transfers.)
> And why aren't we allocating these somehow as they're
> consumed?
Used-up buffers get added to the list again once host has consumed them
in tx_intr.
> > fill_receive_queue(portdev);
> > + alloc_write_bufs(portdev);
>
> What happens if this fails?
Hm, if not a single buf got allocated, any write() requests will not
succeed. dev_warn()?
Amit
^ permalink raw reply
* Re: [PATCH 13/28] virtio: console: Create a buffer pool for sending data to host
From: Rusty Russell @ 2009-12-02 8:30 UTC (permalink / raw)
To: Amit Shah; +Cc: virtualization
In-Reply-To: <1259391051-7752-14-git-send-email-amit.shah@redhat.com>
On Sat, 28 Nov 2009 05:20:36 pm Amit Shah wrote:
> The old way of sending data to the host was to populate one buffer and
> then wait till the host consumed it before sending the next chunk of
> data.
>
> Also, there was no support to send large chunks of data.
>
> We now maintain a per-device list of buffers that are ready to be
> passed on to the host.
>
> This patch adds support to send big chunks in multiple buffers of
> PAGE_SIZE each to the host.
>
> When the host consumes the data and signals to us via an interrupt, we
> add the consumed buffer back to our unconsumed list.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> drivers/char/virtio_console.c | 159 +++++++++++++++++++++++++++++++++-------
> 1 files changed, 131 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e8dabae..3111e4c 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -67,9 +67,13 @@ struct ports_device {
> struct work_struct rx_work;
>
> struct list_head unused_read_head;
> + struct list_head unused_write_head;
>
> /* To protect the list of unused read buffers and the in_vq */
> spinlock_t read_list_lock;
> +
> + /* To protect the list of unused write buffers and the out_vq */
> + spinlock_t write_list_lock;
> };
Let's simplify this a little with a single "buffer_lock" or such in the
previous patch.
> + if (!in_count)
> return 0;
Not necessary: if it happens all we'll do is gratuitously kick the host.
> + in_offset = 0; /* offset in the user buffer */
> + spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
> + while (in_count - in_offset) {
while (in_offset < in_count) seems clearer to me here.
> + copy_size = min(in_count - in_offset, PAGE_SIZE);
Shouldn't you be using buf->size here?
> + spin_unlock_irqrestore(&port->portdev->write_list_lock, irqf);
> +
> + /*
> + * Since we're not sure when the host will actually
> + * consume the data and tell us about it, we have
> + * to copy the data here in case the caller
> + * frees the in_buf
> + */
> + memcpy(buf->buf, in_buf + in_offset, copy_size);
> +
> + buf->len = copy_size;
> + sg_init_one(sg, buf->buf, buf->len);
> +
> + spin_lock_irqsave(&port->portdev->write_list_lock, irqf);
Dropping the lock here seems gratuitous.
> +/*
> + * This function is only called from the init routine so the spinlock
> + * for the unused_write_head list isn't taken
> + */
> +static void alloc_write_bufs(struct ports_device *portdev)
> +{
> + struct port_buffer *buf;
> + int i;
> +
> + for (i = 0; i < 30; i++) {
30? And why aren't we allocating these somehow as they're
consumed?
> fill_receive_queue(portdev);
> + alloc_write_bufs(portdev);
What happens if this fails?
Rusty.
^ permalink raw reply
* Re: [PATCH 12/28] virtio: console: Buffer data that comes in from the host
From: Rusty Russell @ 2009-12-02 3:44 UTC (permalink / raw)
To: Amit Shah; +Cc: Shirley Ma, virtualization
In-Reply-To: <1259391051-7752-13-git-send-email-amit.shah@redhat.com>
On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:
> The console could be flooded with data from the host; handle
> this situation by buffering the data.
All this complexity makes me really wonder if we should just
have the host say the max # ports it will ever use, and just do this
really dumbly. Yes, it's a limitation, but it'd be much simpler.
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -65,6 +65,23 @@ struct ports_device {
> * interrupt
> */
> struct work_struct rx_work;
> +
> + struct list_head unused_read_head;
You should name lists after plurals, rather than using "head" which is
an implementation detail. eg. "queued_inbufs" and below "used_inbufs".
Though Shirly Ma was working on a "destroy_bufs" patch which would avoid
your need for this list at all, AFAICT.
> + /* Return the number of bytes actually copied */
> + ret = copy_size;
> + buf->offset += ret;
> + out_offset += ret;
> + out_count -= ret;
We don't actually use ret.
> + if (buf->len - buf->offset == 0) {
I prefer the simpler "if (buf->offset == buf->len)" myself.
> + spin_lock_irqsave(&port->readbuf_list_lock, flags);
> + list_del(&buf->list);
> + spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
> + kfree(buf->buf);
> + kfree(buf);
Does it become cleaner later to have this in a separate function? Usually
I prefer matching alloc and free fns.
> +static struct port_buffer *get_buf(size_t buf_size)
> +{
> + struct port_buffer *buf;
> +
> + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> + if (!buf)
> + goto out;
> + buf->buf = kzalloc(buf_size, GFP_KERNEL);
> + if (!buf->buf) {
> + kfree(buf);
> + goto out;
No, that would return non-NULL. I'd stick with the standard multi-part exit:
if (!buf)
goto fail;
buf->buf = kzalloc(buf_size, GFP_KERNEL);
if (!buf->buf)
goto fail_free_buf;
buf->len = buf_size;
return buf;
fail_free_buf:
kfree(buf);
fail:
return NULL;
Thanks,
Rusty.
^ permalink raw reply
* Re: virtio: Add memory statistics reporting to the balloon driver (V4)
From: Avi Kivity @ 2009-12-01 15:53 UTC (permalink / raw)
To: Adam Litke; +Cc: linux-kernel, Anthony Liguori, virtualization
In-Reply-To: <1259679905.2816.5.camel@aglitke>
On 12/01/2009 05:05 PM, Adam Litke wrote:
> On Tue, 2009-12-01 at 16:56 +0200, Avi Kivity wrote:
>
>> What about a spec update? Did that happen and I just missed it?
>>
> Yes it did. I forgot to update this patch leader note with the link:
> http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.2.pdf
>
Great, thanks.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: virtio: Add memory statistics reporting to the balloon driver (V4)
From: Adam Litke @ 2009-12-01 15:05 UTC (permalink / raw)
To: Avi Kivity; +Cc: linux-kernel, Anthony Liguori, virtualization
In-Reply-To: <4B152EA4.7080207@redhat.com>
On Tue, 2009-12-01 at 16:56 +0200, Avi Kivity wrote:
> What about a spec update? Did that happen and I just missed it?
Yes it did. I forgot to update this patch leader note with the link:
http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.2.pdf
--
Thanks,
Adam
^ permalink raw reply
* Re: virtio: Add memory statistics reporting to the balloon driver (V4)
From: Avi Kivity @ 2009-12-01 14:56 UTC (permalink / raw)
To: Adam Litke; +Cc: linux-kernel, Anthony Liguori, virtualization
In-Reply-To: <1259597655.30369.2.camel@aglitke>
On 11/30/2009 06:14 PM, Adam Litke wrote:
> Changes since V3:
> - Do not do endian conversions as they will be done in the host
> - Report stats that reference a quantity of memory in bytes
> - Minor coding style updates
>
> Changes since V2:
> - Increase stat field size to 64 bits
> - Report all sizes in kb (not pages)
> - Drop anon_pages stat and fix endianness conversion
>
> Changes since V1:
> - Use a virtqueue instead of the device config space
>
> When using ballooning to manage overcommitted memory on a host, a system for
> guests to communicate their memory usage to the host can provide information
> that will minimize the impact of ballooning on the guests. The current method
> employs a daemon running in each guest that communicates memory statistics to a
> host daemon at a specified time interval. The host daemon aggregates this
> information and inflates and/or deflates balloons according to the level of
> host memory pressure. This approach is effective but overly complex since a
> daemon must be installed inside each guest and coordinated to communicate with
> the host. A simpler approach is to collect memory statistics in the virtio
> balloon driver and communicate them directly to the hypervisor.
>
> This patch enables the guest-side support by adding stats collection and
> reporting to the virtio balloon driver.
>
>
What about a spec update? Did that happen and I just missed it?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: virtio: Add memory statistics reporting to the balloon driver (V4)
From: Adam Litke @ 2009-12-01 14:49 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, Anthony Liguori, virtualization
In-Reply-To: <200912011254.22861.rusty@rustcorp.com.au>
Thanks Rusty.
Acked-by: Adam Litke <agl@us.ibm.com>
On Tue, 2009-12-01 at 12:54 +1030, Rusty Russell wrote:
> On Tue, 1 Dec 2009 02:44:15 am Adam Litke wrote:
> > Changes since V3:
>
> OK, I applied this. And here's the fixes I applied afterwards (to save YA
> round trip: please ack and I'll fold them together)
>
> virtio: balloon fixes
>
> 1) Tag and val args to update_stat() are no longer __le.
> 2) pages_to_bytes() should promote to u64 so we don't truncate.
> 3) Fix two checkpatch.pl warnings.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/virtio/virtio_balloon.c | 8 +++++---
> include/linux/virtio_balloon.h | 3 +--
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -158,14 +158,15 @@ static void leak_balloon(struct virtio_b
> }
>
> static inline void update_stat(struct virtio_balloon *vb, int idx,
> - __le16 tag, __le64 val)
> + u16 tag, u64 val)
> {
> BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> vb->stats[idx].tag = tag;
> vb->stats[idx].val = val;
> }
>
> -#define pages_to_bytes(x) ((x) << PAGE_SHIFT)
> +#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> +
> static void update_balloon_stats(struct virtio_balloon *vb)
> {
> unsigned long events[NR_VM_EVENT_ITEMS];
> @@ -296,7 +297,8 @@ static int virtballoon_probe(struct virt
> * use it to signal us later.
> */
> sg_init_one(&sg, vb->stats, sizeof vb->stats);
> - if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0)
> + if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq,
> + &sg, 1, 0, vb) < 0)
> BUG();
> vb->stats_vq->vq_ops->kick(vb->stats_vq);
> }
> diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> --- a/include/linux/virtio_balloon.h
> +++ b/include/linux/virtio_balloon.h
> @@ -28,8 +28,7 @@ struct virtio_balloon_config
> #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
> #define VIRTIO_BALLOON_S_NR 6
>
> -struct virtio_balloon_stat
> -{
> +struct virtio_balloon_stat {
> u16 tag;
> u64 val;
> } __attribute__((packed));
>
>
--
Thanks,
Adam
^ permalink raw reply
* Re: virtio: Add memory statistics reporting to the balloon driver (V4)
From: Rusty Russell @ 2009-12-01 2:24 UTC (permalink / raw)
To: Adam Litke; +Cc: linux-kernel, Anthony Liguori, virtualization
In-Reply-To: <1259597655.30369.2.camel@aglitke>
On Tue, 1 Dec 2009 02:44:15 am Adam Litke wrote:
> Changes since V3:
OK, I applied this. And here's the fixes I applied afterwards (to save YA
round trip: please ack and I'll fold them together)
virtio: balloon fixes
1) Tag and val args to update_stat() are no longer __le.
2) pages_to_bytes() should promote to u64 so we don't truncate.
3) Fix two checkpatch.pl warnings.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
drivers/virtio/virtio_balloon.c | 8 +++++---
include/linux/virtio_balloon.h | 3 +--
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -158,14 +158,15 @@ static void leak_balloon(struct virtio_b
}
static inline void update_stat(struct virtio_balloon *vb, int idx,
- __le16 tag, __le64 val)
+ u16 tag, u64 val)
{
BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
vb->stats[idx].tag = tag;
vb->stats[idx].val = val;
}
-#define pages_to_bytes(x) ((x) << PAGE_SHIFT)
+#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
+
static void update_balloon_stats(struct virtio_balloon *vb)
{
unsigned long events[NR_VM_EVENT_ITEMS];
@@ -296,7 +297,8 @@ static int virtballoon_probe(struct virt
* use it to signal us later.
*/
sg_init_one(&sg, vb->stats, sizeof vb->stats);
- if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0)
+ if (vb->stats_vq->vq_ops->add_buf(vb->stats_vq,
+ &sg, 1, 0, vb) < 0)
BUG();
vb->stats_vq->vq_ops->kick(vb->stats_vq);
}
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -28,8 +28,7 @@ struct virtio_balloon_config
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_NR 6
-struct virtio_balloon_stat
-{
+struct virtio_balloon_stat {
u16 tag;
u64 val;
} __attribute__((packed));
^ permalink raw reply
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