* Re: [net-next RFC PATCH 4/7] tuntap: multiqueue support
From: Jason Wang @ 2011-08-14 6:07 UTC (permalink / raw)
To: paulmck
Cc: krkumar2, kvm, mst, qemu-devel, netdev, linux-kernel,
virtualization, mirq-linux, davem
In-Reply-To: <20110812232131.GU2395@linux.vnet.ibm.com>
----- Original Message -----
> On Fri, Aug 12, 2011 at 09:55:20AM +0800, Jason Wang wrote:
> > With the abstraction that each socket were a backend of a
> > queue for userspace, this patch adds multiqueue support for
> > tap device by allowing multiple sockets to be attached to a
> > tap device. Then we could parallize the transmission by put
> > them into different socket.
> >
> > As queue related information were stored in private_data of
> > file new, we could simply implement the multiqueue support
> > by add an array of pointers to sockets were stored in the
> > tap device. Then ioctls may be added to manipulate those
> > pointers for adding or removing queues.
> >
> > In order to let tx path lockless, NETIF_F_LLTX were used for
> > multiqueue tap device. And RCU is used for doing
> > synchronization between packet handling and system calls
> > such as removing queues.
> >
> > Currently, multiqueue support is limited for tap , but it's
> > easy also enable it for tun if we find it was also helpful.
>
> Question below about calls to tun_get_slot().
>
> Thanx, Paul
>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > drivers/net/tun.c | 376
> > ++++++++++++++++++++++++++++++++++-------------------
> > 1 files changed, 243 insertions(+), 133 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 4cd292a..8bc6dff 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -108,6 +108,8 @@ struct tap_filter {
> > unsigned char addr[FLT_EXACT_COUNT][ETH_ALEN];
> > };
> >
> > +#define MAX_TAP_QUEUES (NR_CPUS < 16 ? NR_CPUS : 16)
> > +
> > struct tun_file {
> > struct sock sk;
> > struct socket socket;
> > @@ -115,7 +117,7 @@ struct tun_file {
> > int vnet_hdr_sz;
> > struct tap_filter txflt;
> > atomic_t count;
> > - struct tun_struct *tun;
> > + struct tun_struct __rcu *tun;
> > struct net *net;
> > struct fasync_struct *fasync;
> > unsigned int flags;
> > @@ -124,7 +126,8 @@ struct tun_file {
> > struct tun_sock;
> >
> > struct tun_struct {
> > - struct tun_file *tfile;
> > + struct tun_file *tfiles[MAX_TAP_QUEUES];
> > + unsigned int numqueues;
> > unsigned int flags;
> > uid_t owner;
> > gid_t group;
> > @@ -139,80 +142,183 @@ struct tun_struct {
> > #endif
> > };
> >
> > -static int tun_attach(struct tun_struct *tun, struct file *file)
> > +static DEFINE_SPINLOCK(tun_lock);
> > +
> > +/*
> > + * get_slot: return a [unused/occupied] slot in tun->tfiles[]:
> > + * - if 'f' is NULL, return the first empty slot;
> > + * - otherwise, return the slot this pointer occupies.
> > + */
> > +static int tun_get_slot(struct tun_struct *tun, struct tun_file
> > *tfile)
> > {
> > - struct tun_file *tfile = file->private_data;
> > - int err;
> > + int i;
> >
> > - ASSERT_RTNL();
> > + for (i = 0; i < MAX_TAP_QUEUES; i++) {
> > + if (rcu_dereference(tun->tfiles[i]) == tfile)
> > + return i;
> > + }
> >
> > - netif_tx_lock_bh(tun->dev);
> > + /* Should never happen */
> > + BUG_ON(1);
> > +}
> >
> > - err = -EINVAL;
> > - if (tfile->tun)
> > - goto out;
> > +/*
> > + * tun_get_queue(): calculate the queue index
> > + * - if skbs comes from mq nics, we can just borrow
> > + * - if not, calculate from the hash
> > + */
> > +static struct tun_file *tun_get_queue(struct net_device *dev,
> > + struct sk_buff *skb)
> > +{
> > + struct tun_struct *tun = netdev_priv(dev);
> > + struct tun_file *tfile = NULL;
> > + int numqueues = tun->numqueues;
> > + __u32 rxq;
> >
> > - err = -EBUSY;
> > - if (tun->tfile)
> > + BUG_ON(!rcu_read_lock_held());
> > +
> > + if (!numqueues)
> > goto out;
> >
> > - err = 0;
> > - tfile->tun = tun;
> > - tun->tfile = tfile;
> > - netif_carrier_on(tun->dev);
> > - dev_hold(tun->dev);
> > - sock_hold(&tfile->sk);
> > - atomic_inc(&tfile->count);
> > + if (likely(skb_rx_queue_recorded(skb))) {
> > + rxq = skb_get_rx_queue(skb);
> > +
> > + while (unlikely(rxq >= numqueues))
> > + rxq -= numqueues;
> > +
> > + tfile = rcu_dereference(tun->tfiles[rxq]);
> > + if (tfile)
> > + goto out;
> > + }
> > +
> > + /* Check if we can use flow to select a queue */
> > + rxq = skb_get_rxhash(skb);
> > + if (rxq) {
> > + tfile = rcu_dereference(tun->tfiles[rxq % numqueues]);
> > + if (tfile)
> > + goto out;
> > + }
> > +
> > + /* Everything failed - find first available queue */
> > + for (rxq = 0; rxq < MAX_TAP_QUEUES; rxq++) {
> > + tfile = rcu_dereference(tun->tfiles[rxq]);
> > + if (tfile)
> > + break;
> > + }
> >
> > out:
> > - netif_tx_unlock_bh(tun->dev);
> > - return err;
> > + return tfile;
> > }
> >
> > -static void __tun_detach(struct tun_struct *tun)
> > +static int tun_detach(struct tun_file *tfile, bool clean)
> > {
> > - struct tun_file *tfile = tun->tfile;
> > - /* Detach from net device */
> > - netif_tx_lock_bh(tun->dev);
> > - netif_carrier_off(tun->dev);
> > - tun->tfile = NULL;
> > - netif_tx_unlock_bh(tun->dev);
> > -
> > - /* Drop read queue */
> > - skb_queue_purge(&tfile->socket.sk->sk_receive_queue);
> > -
> > - /* Drop the extra count on the net device */
> > - dev_put(tun->dev);
> > -}
> > + struct tun_struct *tun;
> > + struct net_device *dev = NULL;
> > + bool destroy = false;
> >
> > -static void tun_detach(struct tun_struct *tun)
> > -{
> > - rtnl_lock();
> > - __tun_detach(tun);
> > - rtnl_unlock();
> > -}
> > + spin_lock(&tun_lock);
> >
> > -static struct tun_struct *__tun_get(struct tun_file *tfile)
> > -{
> > - struct tun_struct *tun = NULL;
> > + tun = rcu_dereference_protected(tfile->tun,
> > + lockdep_is_held(&tun_lock));
> > + if (tun) {
> > + int index = tun_get_slot(tun, tfile);
>
> Don't we need to be in an RCU read-side critical section in order to
> safely call tun_get_slot()?
>
> Or is the fact that we are calling with tun_lock held sufficient?
> If the latter, then the rcu_dereference() in tun_get_slot() should
> use rcu_dereference_protected() rather than rcu_dereference().
>
Nice catch. The latter, tun_lock held is sufficient. Thanks.
^ permalink raw reply
* Re: [net-next RFC PATCH 0/7] multiqueue support for tun/tap
From: Jason Wang @ 2011-08-14 6:19 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: krkumar2, kvm, mst, qemu-devel, netdev, linux-kernel,
virtualization, mirq-linux, davem
In-Reply-To: <1313196390.28682.14.camel@w-sridhar.beaverton.ibm.com>
----- Original Message -----
> On Fri, 2011-08-12 at 09:54 +0800, Jason Wang wrote:
> > As multi-queue nics were commonly used for high-end servers,
> > current single queue based tap can not satisfy the
> > requirement of scaling guest network performance as the
> > numbers of vcpus increase. So the following series
> > implements multiple queue support in tun/tap.
> >
> > In order to take advantages of this, a multi-queue capable
> > driver and qemu were also needed. I just rebase the latest
> > version of Krishna's multi-queue virtio-net driver into this
> > series to simplify the test. And for multiqueue supported
> > qemu, you can refer the patches I post in
> > http://www.spinics.net/lists/kvm/msg52808.html. Vhost is
> > also a must to achieve high performance and its code could
> > be used for multi-queue without modification. Alternatively,
> > this series can be also used for Krishna's M:N
> > implementation of multiqueue but I didn't test it.
> >
> > The idea is simple: each socket were abstracted as a queue
> > for tun/tap, and userspace may open as many files as
> > required and then attach them to the devices. In order to
> > keep the ABI compatibility, device creation were still
> > finished in TUNSETIFF, and two new ioctls TUNATTACHQUEUE and
> > TUNDETACHQUEUE were added for user to manipulate the numbers
> > of queues for the tun/tap.
>
> Is it possible to have tap create these queues automatically when
> TUNSETIFF is called instead of having userspace to do the new
> ioctls. I am just wondering if it is possible to get multi-queue
> to be enabled without any changes to qemu. I guess the number of
> queues
> could be based on the number of vhost threads/guest virtio-net queues.
It's possible but we need at least pass the number of queues
through TUNSETIFF which may break the ABI? And this method
is not flexible as adding new ioctls, consider we may
disable some queues for some reaons such as running a single
queue guest or pxe on an multiple virtio-net backened.
>
> Also, is it possible to enable multi-queue on the host alone without
> any guest virtio-net changes?
If we use current driver without changes, it can run on host
that multiqueu enabled. But it can not make use all of the
queues.
>
> Have you done any multiple TCP_RR/UDP_RR testing with small packet
> sizes? 256byte request/response with 50-100 instances?
Not yet, I would do it after I was back from KVM Forum.
>
> >
> > I've done some basic performance testing of multi queue
> > tap. For tun, I just test it through vpnc.
> >
> > Notes:
> > - Test shows improvement when receving packets from
> > local/external host to guest, and send big packet from guest
> > to local/external host.
> > - Current multiqueue based virtio-net/tap introduce a
> > regression of send small packet (512 byte) from guest to
> > local/external host. I suspect it's the issue of queue
> > selection in both guest driver and tap. Would continue to
> > investigate.
> > - I would post the perforamnce numbers as a reply of this
> > mail.
> >
> > TODO:
> > - solve the issue of packet transmission of small packets.
> > - addressing the comments of virtio-net driver
> > - performance tunning
> >
> > Please review and comment it, Thanks.
> >
> > ---
> >
> > Jason Wang (5):
> > tuntap: move socket/sock related structures to tun_file
> > tuntap: categorize ioctl
> > tuntap: introduce multiqueue related flags
> > tuntap: multiqueue support
> > tuntap: add ioctls to attach or detach a file form tap device
> >
> > Krishna Kumar (2):
> > Change virtqueue structure
> > virtio-net changes
> >
> >
> > drivers/net/tun.c | 738 ++++++++++++++++++++++++++-----------------
> > drivers/net/virtio_net.c | 578 ++++++++++++++++++++++++----------
> > drivers/virtio/virtio_pci.c | 10 -
> > include/linux/if_tun.h | 5
> > include/linux/virtio.h | 1
> > include/linux/virtio_net.h | 3
> > 6 files changed, 867 insertions(+), 468 deletions(-)
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Sasha Levin @ 2011-08-14 13:57 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <878vqw9007.fsf@rustcorp.com.au>
On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
>
> Erk. This means, in general, we have to do virtio_find_single_vq or
> config->find_vqs before we examine any config options.
>
> Look at virtio_blk, which has the same error.
>
> Solutions in order of best to worst:
> (1) Enable MSI-X before calling device probe. This means reserving two
> vectors in virtio_pci_probe to ensure we *can* do this, I think. Michael?
Do you mean reserving the vectors even before we probed the device for
MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).
> (2) Ensure ordering of "find_vqs then access config space" statically. This
> probably means handing the vqs array to virtio_config_val, so noone
> can call it before they have their virtqueues.
Just noticed that only virtio-blk uses virtio_config_val(), while the
others are still doing 'if(virtio_has_feature()) vdev->config->get()',
I'll send patches to fix that regardless of what we end up doing here.
Did you want to pass the vq array to virtio_config_val() just to check
that they were already found?
> (3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done
> find_vqs when they call the config accessors.
>
> If (1) is too invasive for -stable, then we should rearrange the drivers
> in separate patches (and cc: -stable), then fix it properly.
>
> Good catch Sasha!
>
> Cheers,
> Rusty.
--
Sasha.
^ permalink raw reply
* [PATCH 1/3] virtio-console: Use virtio_config_val() for retrieving config
From: Sasha Levin @ 2011-08-14 14:52 UTC (permalink / raw)
To: linux-kernel; +Cc: Amit Shah, virtualization, Sasha Levin, Michael S. Tsirkin
This patch modifies virtio-console to use virtio_config_val() instead
of a 'if(virtio_has_feature()) vdev->config->get()' construct to retrieve
optional values from the config space.
Cc: Amit Shah <amit.shah@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/char/virtio_console.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fb68b12..ed99f3b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1675,13 +1675,11 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
multiport = false;
portdev->config.max_nr_ports = 1;
- if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT)) {
+ if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+ offsetof(struct virtio_console_config,
+ max_nr_ports),
+ &portdev->config.max_nr_ports) == 0)
multiport = true;
- vdev->config->get(vdev, offsetof(struct virtio_console_config,
- max_nr_ports),
- &portdev->config.max_nr_ports,
- sizeof(portdev->config.max_nr_ports));
- }
err = init_vqs(portdev);
if (err < 0) {
--
1.7.6
^ permalink raw reply related
* [PATCH 2/3] virtio_config: Add virtio_config_val_len()
From: Sasha Levin @ 2011-08-14 14:52 UTC (permalink / raw)
To: linux-kernel; +Cc: Amit Shah, virtualization, Sasha Levin, Michael S. Tsirkin
In-Reply-To: <1313333553-23086-1-git-send-email-levinsasha928@gmail.com>
This patch adds virtio_config_val_len() which allows retrieving variable
length data from the virtio config space only if a specific feature is on.
Cc: Amit Shah <amit.shah@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
include/linux/virtio_config.h | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 39c88c5..add4790 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -155,6 +155,9 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
#define virtio_config_val(vdev, fbit, offset, v) \
virtio_config_buf((vdev), (fbit), (offset), (v), sizeof(*v))
+#define virtio_config_val_len(vdev, fbit, offset, v, len) \
+ virtio_config_buf((vdev), (fbit), (offset), (v), (len))
+
static inline int virtio_config_buf(struct virtio_device *vdev,
unsigned int fbit,
unsigned int offset,
--
1.7.6
^ permalink raw reply related
* [PATCH 3/3] virtio-net: Use virtio_config_val() for retrieving config
From: Sasha Levin @ 2011-08-14 14:52 UTC (permalink / raw)
To: linux-kernel; +Cc: Amit Shah, virtualization, Sasha Levin, Michael S. Tsirkin
In-Reply-To: <1313333553-23086-1-git-send-email-levinsasha928@gmail.com>
This patch modifies virtio-net to use virtio_config_val() instead
of a 'if(virtio_has_feature()) vdev->config->get()' construct to retrieve
optional values from the config space.
Cc: Amit Shah <amit.shah@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/net/virtio_net.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..878db50 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -902,12 +902,10 @@ static void virtnet_update_status(struct virtnet_info *vi)
{
u16 v;
- if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS))
- return;
-
- vi->vdev->config->get(vi->vdev,
+ if (virtio_config_val(vi->vdev, VIRTIO_NET_F_STATUS,
offsetof(struct virtio_net_config, status),
- &v, sizeof(v));
+ &v) < 0)
+ return;
/* Ignore unknown (future) status bits */
v &= VIRTIO_NET_S_LINK_UP;
@@ -982,11 +980,9 @@ static int virtnet_probe(struct virtio_device *vdev)
}
/* Configuration may specify what MAC to use. Otherwise random. */
- if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
- vdev->config->get(vdev,
+ if (virtio_config_val_len(vdev, VIRTIO_NET_F_MAC,
offsetof(struct virtio_net_config, mac),
- dev->dev_addr, dev->addr_len);
- } else
+ dev->dev_addr, dev->addr_len) < 0)
random_ether_addr(dev->dev_addr);
/* Set up our device-specific information */
--
1.7.6
^ permalink raw reply related
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Rusty Russell @ 2011-08-15 0:25 UTC (permalink / raw)
To: Sasha Levin; +Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <1313330252.2422.12.camel@sasha>
On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> >
> > Erk. This means, in general, we have to do virtio_find_single_vq or
> > config->find_vqs before we examine any config options.
> >
> > Look at virtio_blk, which has the same error.
> >
> > Solutions in order of best to worst:
> > (1) Enable MSI-X before calling device probe. This means reserving two
> > vectors in virtio_pci_probe to ensure we *can* do this, I think. Michael?
>
> Do you mean reserving the vectors even before we probed the device for
> MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).
We want three, but *need* two: see vp_find_vqs(). Also, the generic
code doesn't know how many virtqueues we have on the device.
> > (2) Ensure ordering of "find_vqs then access config space" statically. This
> > probably means handing the vqs array to virtio_config_val, so noone
> > can call it before they have their virtqueues.
>
> Just noticed that only virtio-blk uses virtio_config_val(), while the
> others are still doing 'if(virtio_has_feature()) vdev->config->get()',
> I'll send patches to fix that regardless of what we end up doing here.
Thanks.
> Did you want to pass the vq array to virtio_config_val() just to check
> that they were already found?
Not if we fix is using method #1...
Thanks,
Rusty.
^ permalink raw reply
* [PULL] lguest fix, virtio documentation
From: Rusty Russell @ 2011-08-15 0:55 UTC (permalink / raw)
To: Linus Torvalds; +Cc: lkml - Kernel Mailing List, virtualization
[ This adds a text copy of the virtio draft spec to the kernel source,
which should make it easier for people to find and read. The master
is still the lyx document on my server, so I'll keep it in sync. ]
The following changes since commit 91d85ea6786107aa2837bef3e957165ad7c8b823:
Merge branch 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/staging (2011-08-13 18:37:28 -0700)
are available in the git repository at:
ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master
Rusty Russell (2):
virtio: Add text copy of spec to Documentation/virtual.
lguest: allow booting guest with CONFIG_RELOCATABLE=y
Documentation/virtual/00-INDEX | 3 +
Documentation/virtual/lguest/lguest.c | 3 +
Documentation/virtual/virtio-spec.txt | 2200 +++++++++++++++++++++++++++++++++
3 files changed, 2206 insertions(+), 0 deletions(-)
create mode 100644 Documentation/virtual/virtio-spec.txt
^ permalink raw reply
* Re: [PATCH 3/3] virtio-net: Use virtio_config_val() for retrieving config
From: Rusty Russell @ 2011-08-15 0:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Amit Shah, virtualization, Sasha Levin, Michael S. Tsirkin
In-Reply-To: <1313333553-23086-3-git-send-email-levinsasha928@gmail.com>
On Sun, 14 Aug 2011 17:52:33 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> This patch modifies virtio-net to use virtio_config_val() instead
> of a 'if(virtio_has_feature()) vdev->config->get()' construct to retrieve
> optional values from the config space.
Thanks, all applied.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PULL] lguest fix, virtio documentation
From: Sasha Levin @ 2011-08-15 14:48 UTC (permalink / raw)
To: Rusty Russell; +Cc: Linus Torvalds, lkml - Kernel Mailing List, virtualization
In-Reply-To: <87sjp37au3.fsf@rustcorp.com.au>
On Mon, 2011-08-15 at 10:25 +0930, Rusty Russell wrote:
> [ This adds a text copy of the virtio draft spec to the kernel source,
> which should make it easier for people to find and read. The master
> is still the lyx document on my server, so I'll keep it in sync. ]
>
> The following changes since commit 91d85ea6786107aa2837bef3e957165ad7c8b823:
>
> Merge branch 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/staging (2011-08-13 18:37:28 -0700)
>
> are available in the git repository at:
>
> ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master
>
> Rusty Russell (2):
> virtio: Add text copy of spec to Documentation/virtual.
The "Device Status" paragraph says FAILED is 8, while it should be 0x80.
--
Sasha.
^ permalink raw reply
* [PATCH 0/8] Staging: hv: vmbus: Driver cleanup
From: K. Y. Srinivasan @ 2011-08-15 22:10 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: K. Y. Srinivasan
Further cleanup of the vmbus driver:
1) Cleanup the interrupt handler by inlining some code.
2) Ensure message handling is performed on the same CPU that
takes the vmbus interrupt.
3) Check for events before messages (from the host).
4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
interrupt anyway.
5) Some general cleanup.
Regards,
K. Y
^ permalink raw reply
* [PATCH 1/8] Staging: hv: vmbus: Get rid of vmbus_on_isr() by inlining the code
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446210-10611-1-git-send-email-kys@microsoft.com>
Get rid of vmbus_on_isr() by inlining this code in the
real interrupt handler.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 41 +++++++++++----------------------------
1 files changed, 12 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index b382829..bb25c5b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -411,53 +411,36 @@ static void vmbus_on_msg_dpc(unsigned long data)
}
}
-/*
- * vmbus_on_isr - ISR routine
- */
-static int vmbus_on_isr(void)
+static irqreturn_t vmbus_isr(int irq, void *dev_id)
{
- int ret = 0;
int cpu = smp_processor_id();
void *page_addr;
struct hv_message *msg;
union hv_synic_event_flags *event;
+ bool handled = false;
page_addr = hv_context.synic_message_page[cpu];
msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
/* Check if there are actual msgs to be process */
- if (msg->header.message_type != HVMSG_NONE)
- ret |= 0x1;
+ if (msg->header.message_type != HVMSG_NONE) {
+ handled = true;
+ tasklet_schedule(&msg_dpc);
+ }
page_addr = hv_context.synic_event_page[cpu];
event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
/* Since we are a child, we only need to check bit 0 */
- if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0]))
- ret |= 0x2;
-
- return ret;
-}
-
-
-static irqreturn_t vmbus_isr(int irq, void *dev_id)
-{
- int ret;
-
- ret = vmbus_on_isr();
-
- /* Schedules a dpc if necessary */
- if (ret > 0) {
- if (test_bit(0, (unsigned long *)&ret))
- tasklet_schedule(&msg_dpc);
-
- if (test_bit(1, (unsigned long *)&ret))
- tasklet_schedule(&event_dpc);
+ if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0])) {
+ handled = true;
+ tasklet_schedule(&event_dpc);
+ }
+ if (handled)
return IRQ_HANDLED;
- } else {
+ else
return IRQ_NONE;
- }
}
/*
--
1.7.4.1
^ permalink raw reply related
* [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
The message processing function needs to execute on the same CPU where
the interrupt was taken. tasklets cannot gurantee this; so, invoke the
function directly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index bb25c5b..1d4e878 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -39,7 +39,6 @@
static struct acpi_device *hv_acpi_dev;
-static struct tasklet_struct msg_dpc;
static struct tasklet_struct event_dpc;
unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
@@ -425,7 +424,7 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
/* Check if there are actual msgs to be process */
if (msg->header.message_type != HVMSG_NONE) {
handled = true;
- tasklet_schedule(&msg_dpc);
+ vmbus_on_msg_dpc(0);
}
page_addr = hv_context.synic_event_page[cpu];
@@ -465,7 +464,6 @@ static int vmbus_bus_init(int irq)
}
/* Initialize the bus context */
- tasklet_init(&msg_dpc, vmbus_on_msg_dpc, 0);
tasklet_init(&event_dpc, vmbus_on_event, 0);
/* Now, register the bus with LDM */
--
1.7.4.1
^ permalink raw reply related
* [PATCH 3/8] Staging: hv: vmbus: Check for events before messages
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
Hyper-V requires that events be processed before processing messages. Make the
necessary code adjustments.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 1d4e878..536466b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -418,15 +418,6 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
union hv_synic_event_flags *event;
bool handled = false;
- page_addr = hv_context.synic_message_page[cpu];
- msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
-
- /* Check if there are actual msgs to be process */
- if (msg->header.message_type != HVMSG_NONE) {
- handled = true;
- vmbus_on_msg_dpc(0);
- }
-
page_addr = hv_context.synic_event_page[cpu];
event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
@@ -436,6 +427,15 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
tasklet_schedule(&event_dpc);
}
+ page_addr = hv_context.synic_message_page[cpu];
+ msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
+
+ /* Check if there are actual msgs to be process */
+ if (msg->header.message_type != HVMSG_NONE) {
+ handled = true;
+ vmbus_on_msg_dpc(0);
+ }
+
if (handled)
return IRQ_HANDLED;
else
--
1.7.4.1
^ permalink raw reply related
* [PATCH 4/8] Staging: hv: vmbus: Do not enable auto eoi
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
The standard interrupt handling in Linux issues an eoi when the
interrupt handling is done. So, setup the vmbus interrupt without
enabling auto eoi.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
index 14e6315..3b0f9f4 100644
--- a/drivers/staging/hv/hv.c
+++ b/drivers/staging/hv/hv.c
@@ -370,7 +370,7 @@ void hv_synic_init(void *irqarg)
shared_sint.as_uint64 = 0;
shared_sint.vector = irq_vector; /* HV_SHARED_SINT_IDT_VECTOR + 0x20; */
shared_sint.masked = false;
- shared_sint.auto_eoi = true;
+ shared_sint.auto_eoi = false;
wrmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 5/8] Staging: hv: vmbus: Fixup indentation in vmbus_acpi_add()
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
Fixup some indentation issues in vmbus_acpi_add().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 536466b..275c4ef 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -654,9 +654,8 @@ static int vmbus_acpi_add(struct acpi_device *device)
hv_acpi_dev = device;
- result =
- acpi_walk_resources(device->handle, METHOD_NAME__CRS,
- vmbus_walk_resources, &irq);
+ result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+ vmbus_walk_resources, &irq);
if (ACPI_FAILURE(result)) {
complete(&probe_event);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 6/8] Staging: hv: vmbus: Get rid of some dated/redundant comments
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
Get rid of some dated/redundant comments.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 12 +-----------
1 files changed, 1 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 275c4ef..f20ab9a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -364,9 +364,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
kfree(ctx);
}
-/*
- * vmbus_on_msg_dpc - DPC routine to handle messages from the hypervisior
- */
static void vmbus_on_msg_dpc(unsigned long data)
{
int cpu = smp_processor_id();
@@ -463,15 +460,12 @@ static int vmbus_bus_init(int irq)
return ret;
}
- /* Initialize the bus context */
tasklet_init(&event_dpc, vmbus_on_event, 0);
- /* Now, register the bus with LDM */
ret = bus_register(&hv_bus);
if (ret)
return ret;
- /* Get the interrupt resource */
ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
driver_name, hv_acpi_dev);
@@ -522,7 +516,6 @@ int vmbus_child_driver_register(struct hv_driver *hv_drv)
pr_info("child driver registering - name %s\n", drv->name);
- /* The child driver on this vmbus */
drv->bus = &hv_bus;
ret = driver_register(drv);
@@ -563,7 +556,6 @@ struct hv_device *vmbus_child_device_create(uuid_le *type,
{
struct hv_device *child_device_obj;
- /* Allocate the new child device */
child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
if (!child_device_obj) {
pr_err("Unable to allocate device object for child device\n");
@@ -589,12 +581,10 @@ int vmbus_child_device_register(struct hv_device *child_device_obj)
static atomic_t device_num = ATOMIC_INIT(0);
- /* Set the device name. Otherwise, device_register() will fail. */
dev_set_name(&child_device_obj->device, "vmbus_0_%d",
atomic_inc_return(&device_num));
- /* The new device belongs to this bus */
- child_device_obj->device.bus = &hv_bus; /* device->dev.bus; */
+ child_device_obj->device.bus = &hv_bus;
child_device_obj->device.parent = &hv_acpi_dev->dev;
child_device_obj->device.release = vmbus_device_release;
--
1.7.4.1
^ permalink raw reply related
* [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
Fix a bug in error handling in vmbus_bus_init().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index f20ab9a..0b0bff3 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
tasklet_init(&event_dpc, vmbus_on_event, 0);
ret = bus_register(&hv_bus);
- if (ret)
+ if (ret) {
+ hv_cleanup();
return ret;
+ }
ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
driver_name, hv_acpi_dev);
@@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
pr_err("Unable to request IRQ %d\n",
irq);
+ hv_cleanup();
bus_unregister(&hv_bus);
return ret;
@@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
on_each_cpu(hv_synic_init, (void *)&vector, 1);
ret = vmbus_connect();
if (ret) {
+ hv_cleanup();
free_irq(irq, hv_acpi_dev);
bus_unregister(&hv_bus);
return ret;
--
1.7.4.1
^ permalink raw reply related
* [PATCH 8/8] Staging: hv: vmbus: Get rid of an unnecessary check in vmbus_connect()
From: K. Y. Srinivasan @ 2011-08-15 22:12 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang
In-Reply-To: <1313446327-10650-1-git-send-email-kys@microsoft.com>
Get rid of an unnecessary check in vmbus_connect(). Since vmbus_connect is
only called once, the check being removed is redundant.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/connection.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/connection.c b/drivers/staging/hv/connection.c
index 66b7c4b..9e99c04 100644
--- a/drivers/staging/hv/connection.c
+++ b/drivers/staging/hv/connection.c
@@ -50,10 +50,6 @@ int vmbus_connect(void)
struct vmbus_channel_initiate_contact *msg;
unsigned long flags;
- /* Make sure we are not connecting or connected */
- if (vmbus_connection.conn_state != DISCONNECTED)
- return -EISCONN;
-
/* Initialize the vmbus connection */
vmbus_connection.conn_state = CONNECTING;
vmbus_connection.work_queue = create_workqueue("hv_vmbus_con");
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Sasha Levin @ 2011-08-15 22:17 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <87vctz7c7d.fsf@rustcorp.com.au>
On Mon, 2011-08-15 at 09:55 +0930, Rusty Russell wrote:
> On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > The MAC of a virtio-net device is located at the first field of the device
> > > > specific header. This header is located at offset 20 if the device doesn't
> > > > support MSI-X or offset 24 if it does.
> > >
> > > Erk. This means, in general, we have to do virtio_find_single_vq or
> > > config->find_vqs before we examine any config options.
> > >
> > > Look at virtio_blk, which has the same error.
> > >
> > > Solutions in order of best to worst:
> > > (1) Enable MSI-X before calling device probe. This means reserving two
> > > vectors in virtio_pci_probe to ensure we *can* do this, I think. Michael?
> >
> > Do you mean reserving the vectors even before we probed the device for
> > MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).
>
> We want three, but *need* two: see vp_find_vqs(). Also, the generic
> code doesn't know how many virtqueues we have on the device.
We can just pci_enable_msix() and see if we can get 2 vectors, if we can
- we assume the device has msix on, right?
--
Sasha.
^ permalink raw reply
* [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Sasha Levin @ 2011-08-16 19:47 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization, Sasha Levin, kvm, Michael S. Tsirkin
This patch adds support for an optional stats vq that works similary to the
stats vq provided by virtio-balloon.
The purpose of this change is to allow collection of statistics about working
virtio-blk devices to easily analyze performance without having to tap into
the guest.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/block/virtio_blk.c | 110 +++++++++++++++++++++++++++++++++++++++++---
include/linux/virtio_blk.h | 20 ++++++++
2 files changed, 123 insertions(+), 7 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..9c196ea 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -19,7 +19,7 @@ struct virtio_blk
spinlock_t lock;
struct virtio_device *vdev;
- struct virtqueue *vq;
+ struct virtqueue *vq, *stats_vq;
/* The disk structure for the kernel. */
struct gendisk *disk;
@@ -35,6 +35,10 @@ struct virtio_blk
/* What host tells us, plus 2 for header & tailer. */
unsigned int sg_elems;
+ /* Block statistics */
+ int need_stats_update;
+ struct virtio_blk_stat stats[VIRTIO_BLK_S_NR];
+
/* Scatterlist: can be too big for stack. */
struct scatterlist sg[/*sg_elems*/];
};
@@ -48,6 +52,75 @@ struct virtblk_req
u8 status;
};
+static inline void update_stat(struct virtio_blk *vb, int idx,
+ u16 tag, u64 val)
+{
+ BUG_ON(idx >= VIRTIO_BLK_S_NR);
+ vb->stats[idx].tag = tag;
+ vb->stats[idx].val = val;
+}
+
+static void update_blk_stats(struct virtio_blk *vb)
+{
+ struct hd_struct *p = disk_get_part(vb->disk, 0);
+ int cpu;
+ int idx = 0;
+
+ cpu = part_stat_lock();
+ part_round_stats(cpu, p);
+ part_stat_unlock();
+
+ update_stat(vb, idx++, VIRTIO_BLK_S_READ_IO,
+ part_stat_read(p, ios[READ]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_READ_MERGES,
+ part_stat_read(p, merges[READ]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_READ_SECTORS,
+ part_stat_read(p, sectors[READ]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_READ_TICKS,
+ jiffies_to_msecs(part_stat_read(p, ticks[READ])));
+ update_stat(vb, idx++, VIRTIO_BLK_S_WRITE_IO,
+ part_stat_read(p, ios[WRITE]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_WRITE_MERGES,
+ part_stat_read(p, merges[WRITE]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_WRITE_SECTORS,
+ part_stat_read(p, sectors[WRITE]));
+ update_stat(vb, idx++, VIRTIO_BLK_S_WRITE_TICKS,
+ jiffies_to_msecs(part_stat_read(p, ticks[WRITE])));
+ update_stat(vb, idx++, VIRTIO_BLK_S_IN_FLIGHT,
+ part_in_flight(p));
+ update_stat(vb, idx++, VIRTIO_BLK_S_IO_TICKS,
+ jiffies_to_msecs(part_stat_read(p, io_ticks)));
+ update_stat(vb, idx++, VIRTIO_BLK_S_TIME_IN_QUEUE,
+ jiffies_to_msecs(part_stat_read(p, time_in_queue)));
+}
+
+static void stats_request(struct virtqueue *vq)
+{
+ struct virtio_blk *vb;
+ unsigned int len;
+
+ vb = virtqueue_get_buf(vq, &len);
+ if (!vb)
+ return;
+ vb->need_stats_update = 1;
+ queue_work(virtblk_wq, &vb->config_work);
+}
+
+static void stats_handle_request(struct virtio_blk *vb)
+{
+ struct virtqueue *vq;
+ struct scatterlist sg;
+
+ vb->need_stats_update = 0;
+ update_blk_stats(vb);
+
+ vq = vb->stats_vq;
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0)
+ BUG();
+ virtqueue_kick(vq);
+}
+
static void blk_done(struct virtqueue *vq)
{
struct virtio_blk *vblk = vq->vdev->priv;
@@ -306,6 +379,11 @@ static void virtblk_config_changed_work(struct work_struct *work)
char cap_str_2[10], cap_str_10[10];
u64 capacity, size;
+ if (vblk->need_stats_update) {
+ stats_handle_request(vblk);
+ return;
+ }
+
/* Host must always specify the capacity. */
vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
&capacity, sizeof(capacity));
@@ -341,7 +419,10 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
{
struct virtio_blk *vblk;
struct request_queue *q;
- int err;
+ vq_callback_t *callbacks[] = { blk_done, stats_request};
+ const char *names[] = { "requests", "stats" };
+ struct virtqueue *vqs[2];
+ int err, nvqs;
u64 cap;
u32 v, blk_size, sg_elems, opt_io_size;
u16 min_io_size;
@@ -375,11 +456,26 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
sg_init_table(vblk->sg, vblk->sg_elems);
INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
- /* We expect one virtqueue, for output. */
- vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
- if (IS_ERR(vblk->vq)) {
- err = PTR_ERR(vblk->vq);
+ /* We expect one virtqueue for output, and optionally a stats vq. */
+ nvqs = virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_STATS_VQ) ? 2 : 1;
+ err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+ if (err)
goto out_free_vblk;
+
+ vblk->vq = vqs[0];
+
+ if (nvqs == 2) {
+ struct scatterlist sg;
+ vblk->stats_vq = vqs[1];
+
+ /*
+ * Prime this virtqueue with one buffer so the hypervisor can
+ * use it to signal us later.
+ */
+ sg_init_one(&sg, vblk->stats, sizeof vblk->stats);
+ if (virtqueue_add_buf(vblk->stats_vq, &sg, 1, 0, vblk) < 0)
+ BUG();
+ virtqueue_kick(vblk->stats_vq);
}
vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
@@ -548,7 +644,7 @@ static const struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
- VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY
+ VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_STATS_VQ
};
/*
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
index e0edb40..6e87c2e 100644
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -39,6 +39,7 @@
#define VIRTIO_BLK_F_SCSI 7 /* Supports scsi command passthru */
#define VIRTIO_BLK_F_FLUSH 9 /* Cache flush command support */
#define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
+#define VIRTIO_BLK_F_STATS_VQ 11 /* Optional stats vq is available */
#define VIRTIO_BLK_ID_BYTES 20 /* ID string length */
@@ -119,4 +120,23 @@ struct virtio_scsi_inhdr {
#define VIRTIO_BLK_S_OK 0
#define VIRTIO_BLK_S_IOERR 1
#define VIRTIO_BLK_S_UNSUPP 2
+
+#define VIRTIO_BLK_S_READ_IO 0
+#define VIRTIO_BLK_S_READ_MERGES 1
+#define VIRTIO_BLK_S_READ_SECTORS 2
+#define VIRTIO_BLK_S_READ_TICKS 3
+#define VIRTIO_BLK_S_WRITE_IO 4
+#define VIRTIO_BLK_S_WRITE_MERGES 5
+#define VIRTIO_BLK_S_WRITE_SECTORS 6
+#define VIRTIO_BLK_S_WRITE_TICKS 7
+#define VIRTIO_BLK_S_IN_FLIGHT 8
+#define VIRTIO_BLK_S_IO_TICKS 9
+#define VIRTIO_BLK_S_TIME_IN_QUEUE 10
+#define VIRTIO_BLK_S_NR 11
+
+struct virtio_blk_stat {
+ u16 tag;
+ u64 val;
+} __attribute__((packed));
+
#endif /* _LINUX_VIRTIO_BLK_H */
--
1.7.6
^ permalink raw reply related
* Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly
From: Sasha Levin @ 2011-08-17 12:47 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1313446327-10650-2-git-send-email-kys@microsoft.com>
On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> The message processing function needs to execute on the same CPU where
> the interrupt was taken. tasklets cannot gurantee this; so, invoke the
> function directly.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
tasklets are guaranteed to run on the same CPU as the function that
scheduled them.
Unless I'm missing something?
> drivers/staging/hv/vmbus_drv.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index bb25c5b..1d4e878 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -39,7 +39,6 @@
>
> static struct acpi_device *hv_acpi_dev;
>
> -static struct tasklet_struct msg_dpc;
> static struct tasklet_struct event_dpc;
>
> unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
> @@ -425,7 +424,7 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> /* Check if there are actual msgs to be process */
> if (msg->header.message_type != HVMSG_NONE) {
> handled = true;
> - tasklet_schedule(&msg_dpc);
> + vmbus_on_msg_dpc(0);
> }
>
> page_addr = hv_context.synic_event_page[cpu];
> @@ -465,7 +464,6 @@ static int vmbus_bus_init(int irq)
> }
>
> /* Initialize the bus context */
> - tasklet_init(&msg_dpc, vmbus_on_msg_dpc, 0);
> tasklet_init(&event_dpc, vmbus_on_event, 0);
>
> /* Now, register the bus with LDM */
--
Sasha.
^ permalink raw reply
* Re: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
From: Sasha Levin @ 2011-08-17 13:26 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1313446327-10650-7-git-send-email-kys@microsoft.com>
On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> Fix a bug in error handling in vmbus_bus_init().
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
vmbus_bus_init() has a 'cleanup' label which is used just for returning,
while the cleanup logic is replicated all over the code.
Can't we just move all of it to the cleanup label?
> drivers/staging/hv/vmbus_drv.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index f20ab9a..0b0bff3 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
> tasklet_init(&event_dpc, vmbus_on_event, 0);
>
> ret = bus_register(&hv_bus);
> - if (ret)
> + if (ret) {
> + hv_cleanup();
> return ret;
> + }
>
> ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> driver_name, hv_acpi_dev);
> @@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
> pr_err("Unable to request IRQ %d\n",
> irq);
>
> + hv_cleanup();
> bus_unregister(&hv_bus);
>
> return ret;
> @@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
> on_each_cpu(hv_synic_init, (void *)&vector, 1);
> ret = vmbus_connect();
> if (ret) {
> + hv_cleanup();
> free_irq(irq, hv_acpi_dev);
> bus_unregister(&hv_bus);
> return ret;
--
Sasha.
^ permalink raw reply
* RE: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly
From: KY Srinivasan @ 2011-08-17 18:43 UTC (permalink / raw)
To: Sasha Levin
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <1313585267.15004.3.camel@lappy>
> -----Original Message-----
> From: Sasha Levin [mailto:levinsasha928@gmail.com]
> Sent: Wednesday, August 17, 2011 8:48 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc()
> directly
>
> On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> > The message processing function needs to execute on the same CPU where
> > the interrupt was taken. tasklets cannot gurantee this; so, invoke the
> > function directly.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
>
> tasklets are guaranteed to run on the same CPU as the function that
> scheduled them.
>
> Unless I'm missing something?
I too was under this impression until I stumbled upon this comment in
include/Linux/interrupt.h where I see that there is no guarantee that
tasklet would run on the same CPU that it was scheduled on
(look at the first listed property):
/* Tasklets --- multithreaded analogue of BHs.
Main feature differing them of generic softirqs: tasklet
is running only on one CPU simultaneously.
Main feature differing them of BHs: different tasklets
may be run simultaneously on different CPUs.
Properties:
* If tasklet_schedule() is called, then tasklet is guaranteed
to be executed on some cpu at least once after this.
.
.
*/
Given this comment here, I felt that safest thing to do would be to just
not use the tasklet in this scenario.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in vmbus_bus_init()
From: KY Srinivasan @ 2011-08-17 19:10 UTC (permalink / raw)
To: Sasha Levin
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <1313587570.18295.2.camel@lappy>
> -----Original Message-----
> From: Sasha Levin [mailto:levinsasha928@gmail.com]
> Sent: Wednesday, August 17, 2011 9:26 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 7/8] Staging: hv: vmbus: Fix a bug in error handling in
> vmbus_bus_init()
>
> On Mon, 2011-08-15 at 15:12 -0700, K. Y. Srinivasan wrote:
> > Fix a bug in error handling in vmbus_bus_init().
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
>
> vmbus_bus_init() has a 'cleanup' label which is used just for returning,
> while the cleanup logic is replicated all over the code.
>
> Can't we just move all of it to the cleanup label?
These are in two different functions - vmbus_bus_init(), hv_acpi_init().
hv_cleanup() rolls back state established in hv_init(). I could look at
cleaning up the error handling code.
Regards,
K. Y
>
> > drivers/staging/hv/vmbus_drv.c | 6 +++++-
> > 1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > index f20ab9a..0b0bff3 100644
> > --- a/drivers/staging/hv/vmbus_drv.c
> > +++ b/drivers/staging/hv/vmbus_drv.c
> > @@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
> > tasklet_init(&event_dpc, vmbus_on_event, 0);
> >
> > ret = bus_register(&hv_bus);
> > - if (ret)
> > + if (ret) {
> > + hv_cleanup();
> > return ret;
> > + }
> >
> > ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > driver_name, hv_acpi_dev);
> > @@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
> > pr_err("Unable to request IRQ %d\n",
> > irq);
> >
> > + hv_cleanup();
> > bus_unregister(&hv_bus);
> >
> > return ret;
> > @@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
> > on_each_cpu(hv_synic_init, (void *)&vector, 1);
> > ret = vmbus_connect();
> > if (ret) {
> > + hv_cleanup();
> > free_irq(irq, hv_acpi_dev);
> > bus_unregister(&hv_bus);
> > return ret;
>
> --
>
> Sasha.
>
^ 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