* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Michael S. Tsirkin @ 2011-08-20 20:00 UTC (permalink / raw)
To: Sasha Levin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1313771587.12243.16.camel@lappy>
On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin 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.
> > >
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > >
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> >
> > The spec seems to be explicit enough:
> > If MSI-X is enabled for the device, two additional fields immediately
> > follow this header.
> >
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
>
> We assumed that PCI config space has a static layout like most other
> devices. Having a behavior of "First bit 20 does something, but after
> enabling MSI-X it does something completely different" sounds strange.
The layout is always virtio header followed by device specific header.
We started with a small header so when more data was added, we could not
extend the header unconditionally.
We can't change that behaviour for MSI-X now, guests and
hosts rely on it.
>
> I'm wondering why offsets of the config structure change during run time
> and are not statically defined when the device is started.
That's because of backwards compatibility with old guests.
When we know the guest is new, we expose new layout,
but old guests must see old layout.
> It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,
Yes it can, e.g. at guest reset. Generally features can be tweaked
any way guest likes until status is set to OK.
> or MSI-X can be simply disabled during run time.
Not sure what you mean by 'run time'. Guest can reset
or disable the device, change any parameters,
then re-enable.
> Maybe this is better solved by copying the way it was done in PCI itself
> with capability linked list?
>
> --
>
> Sasha.
There are any number of ways to lay out the structure. I went for what
seemed a simplest one. For MSI-X the train has left the station. We
can probably still tweak where the high 32 bit features
for 64 bit features are. No idea if it's worth it.
--
MST
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: jamal @ 2011-08-20 14:32 UTC (permalink / raw)
To: San Mehat
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <CAPi7mHp==JPA-0vNZ+xWRzOuJYUxsUL3UY=o=VtMUsB-YcFHwQ@mail.gmail.com>
On Fri, 2011-08-19 at 07:58 -0700, San Mehat wrote:
> Can you explain a good use-case for SOCK_RAW in this type of
> environment? We were noodling it around locally and couldn't come up
> with one that we needed to support.
One that comes to mind is the case of Samir's app: youd need to handle
some of the apps that ride on top of IP typically using SOCK_RAW
eg ping, OSPF essentially anything on IP that doesnt have transport
built into kernel etc;
> > Q: If you want this to be transparent to the apps, who/what is doing
> > the tagging of SOCK_HWASSIST? clearly not the app if you dont want to
> > change it.
>
> The decision of whether to tag a socket or not is made by the 'hardware'
As in some config interface?
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Sasha Levin @ 2011-08-19 16:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20110819152335.GA19489@redhat.com>
On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin 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.
> >
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> >
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
>
> The spec seems to be explicit enough:
> If MSI-X is enabled for the device, two additional fields immediately
> follow this header.
>
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.
We assumed that PCI config space has a static layout like most other
devices. Having a behavior of "First bit 20 does something, but after
enabling MSI-X it does something completely different" sounds strange.
I'm wondering why offsets of the config structure change during run time
and are not statically defined when the device is started.
It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,
or MSI-X can be simply disabled during run time.
Maybe this is better solved by copying the way it was done in PCI itself
with capability linked list?
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Michael S. Tsirkin @ 2011-08-19 15:23 UTC (permalink / raw)
To: Sasha Levin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1313225461-24458-1-git-send-email-levinsasha928@gmail.com>
On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin 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.
>
> Current code in virtnet_probe() used to probe the MAC before checking for
> MSI-X, which means that the read was always made from offset 20 regardless
> of whether MSI-X in enabled or not.
>
> This patch moves the MAC probe to after the detection of whether MSI-X is
> enabled. This way the MAC will be read from offset 24 if the device indeed
> supports MSI-X.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
I am not sure I see a bug in virtio: the config pace layout simply
changes as msix is enabled and disabled (and if you look at the latest
draft, also on whether 64 bit features are enabled).
It doesn't depend on msix capability being present in device.
The spec seems to be explicit enough:
If MSI-X is enabled for the device, two additional fields immediately
follow this header.
So I'm guessing the bug is in kvm tools which assume
same layout for when msix is enabled and disabled.
qemu-kvm seems to do the right thing so the device
seems to get the correct mac.
> ---
> drivers/net/virtio_net.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..55ccf96 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* (!csum && gso) case will be fixed by register_netdev() */
> }
>
> - /* Configuration may specify what MAC to use. Otherwise random. */
> - if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> - vdev->config->get(vdev,
> - offsetof(struct virtio_net_config, mac),
> - dev->dev_addr, dev->addr_len);
> - } else
> - random_ether_addr(dev->dev_addr);
> -
> /* Set up our device-specific information */
> vi = netdev_priv(dev);
> netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
> @@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> dev->features |= NETIF_F_HW_VLAN_FILTER;
> }
>
> + /* Configuration may specify what MAC to use. Otherwise random. */
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> + vdev->config->get(vdev,
> + offsetof(struct virtio_net_config, mac),
> + dev->dev_addr, dev->addr_len);
> + } else
> + random_ether_addr(dev->dev_addr);
> +
> err = register_netdev(dev);
> if (err) {
> pr_debug("virtio_net: registering device failed\n");
> --
> 1.7.6
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: San Mehat @ 2011-08-19 14:58 UTC (permalink / raw)
To: jhs
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <1313758185.16299.43.camel@mojatatu>
On Fri, Aug 19, 2011 at 5:49 AM, jamal <hadi@cyberus.ca> wrote:
>
> On Thu, 2011-08-18 at 15:07 -0700, San Mehat wrote:
> > TL;DR
> > -----
> > In this RFC we propose the introduction of the concept of hardware socket
> > offload to the Linux kernel. Patches will accompany this RFC in a few days,
> > but we felt we had enough on the design to solicit constructive discussion
> > from the community at-large.
> >
>
> [..]
>
> > ALTERNATIVE STRATEGIES
> > ----------------------
> >
> > An alternative strategy for providing similar functionality involves either
> > modifying glibc or using LD_PRELOAD tricks to intercept socket calls. We were
> > forced to rule this out due to the complexity (and fragility) involved with
> > attempting to maintain a general solution compatible across various
> > distributions where platform-libraries differ.
>
> Above should have been in your TL;DR;->
>
> LD_PRELOAD is also horrible because of the granularity of the socket
> calls;
> Having things in the kernel and specifically tagging socket as needing
> this feature is much much more manageable.
>
> Tying things to virtualization may miss the big picture because there
> are many other use cases for intercepting socket calls, example:
> Samir Bellabes <sam@synack.fr> has been trying to get what he refers to
> as "personal firewall" (equivalent to the silly windows firewall) which
> prompts the user "ping from blah, do you want to allow a response?"
> That requires intercepting send/recv calls, prompt the user in possibly
> some pop-up and act based on response. It requires looking at content.
> He is trying to use selinux for that interface,
> but i think this would be the right abstraction.
I agree; there's no reason this needs to be tied to virtualization -
it was just the
driving force behind the design. I will generalize the backend interface types
> I hope you plan to support send/recv.
yes
> I also hope you add support for SOCK_RAW (and maybe SOCK_PACKET).
Can you explain a good use-case for SOCK_RAW in this type of
environment? We were noodling it around locally and couldn't come up
with one that we needed to support.
>
> Q: If you want this to be transparent to the apps, who/what is doing
> the tagging of SOCK_HWASSIST? clearly not the app if you dont want to
> change it.
The decision of whether to tag a socket or not is made by the 'hardware'
>
> I like the uri abstraction if it doesnt come at the expense of the
> app transparency.
>
Thank you
-san
> cheers,
> jamal
>
--
San Mehat | Staff Software Engineer | san@google.com | 415-366-6172
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: San Mehat @ 2011-08-19 14:44 UTC (permalink / raw)
To: jhs
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <1313758185.16299.43.camel@mojatatu>
[-- Attachment #1.1: Type: text/plain, Size: 2605 bytes --]
On Fri, Aug 19, 2011 at 5:49 AM, jamal <hadi@cyberus.ca> wrote:
> On Thu, 2011-08-18 at 15:07 -0700, San Mehat wrote:
> > TL;DR
> > -----
> > In this RFC we propose the introduction of the concept of hardware socket
> > offload to the Linux kernel. Patches will accompany this RFC in a few
> days,
> > but we felt we had enough on the design to solicit constructive
> discussion
> > from the community at-large.
> >
>
> [..]
>
> > ALTERNATIVE STRATEGIES
> > ----------------------
> >
> > An alternative strategy for providing similar functionality involves
> either
> > modifying glibc or using LD_PRELOAD tricks to intercept socket calls. We
> were
> > forced to rule this out due to the complexity (and fragility) involved
> with
> > attempting to maintain a general solution compatible across various
> > distributions where platform-libraries differ.
>
> Above should have been in your TL;DR;->
>
> LD_PRELOAD is also horrible because of the granularity of the socket
> calls;
> Having things in the kernel and specifically tagging socket as needing
> this feature is much much more manageable.
>
> Tying things to virtualization may miss the big picture because there
> are many other use cases for intercepting socket calls, example:
> Samir Bellabes <sam@synack.fr> has been trying to get what he refers to
> as "personal firewall" (equivalent to the silly windows firewall) which
> prompts the user "ping from blah, do you want to allow a response?"
> That requires intercepting send/recv calls, prompt the user in possibly
> some pop-up and act based on response. It requires looking at content.
> He is trying to use selinux for that interface,
> but i think this would be the right abstraction.
>
I agree; there's no reason this needs to be tied to virtualization - it was
just the
driving force behind the design. I will generalize the backend interface
types
> I hope you plan to support send/recv.
>
yes
> I also hope you add support for SOCK_RAW (and maybe SOCK_PACKET).
>
>
Can you explain a good use-case for SOCK_RAW in this type of environment?
We were noodling it around locally and couldn't come up with one that we
needed to support.
Q: If you want this to be transparent to the apps, who/what is doing
> the tagging of SOCK_HWASSIST? clearly not the app if you dont want to
> change it.
>
>
The decision of whether to tag a socket or not is made by the 'hardware'
I like the uri abstraction if it doesnt come at the expense of the
> app transparency.
>
>
Thank you,
-san
> cheers,
> jamal
>
>
--
San Mehat | Staff Software Engineer | san@google.com | 415-366-6172
[-- Attachment #1.2: Type: text/html, Size: 6533 bytes --]
[-- Attachment #2: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 63/75] xen: netfront: convert to SKB paged frag API.
From: Ian Campbell @ 2011-08-19 13:27 UTC (permalink / raw)
To: netdev
Cc: xen-devel, Ian Campbell, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, Jeremy Fitzhardinge
In-Reply-To: <1313760393.5010.356.camel@zakaz.uk.xensource.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/xen-netfront.c | 28 +++++++++++++++++-----------
1 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index d7c8a98..882a957 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -275,7 +275,7 @@ no_skb:
break;
}
- skb_shinfo(skb)->frags[0].page = page;
+ skb_frag_set_page(skb, 0, page);
skb_shinfo(skb)->nr_frags = 1;
__skb_queue_tail(&np->rx_batch, skb);
}
@@ -309,8 +309,8 @@ no_skb:
BUG_ON((signed short)ref < 0);
np->grant_rx_ref[id] = ref;
- pfn = page_to_pfn(skb_shinfo(skb)->frags[0].page);
- vaddr = page_address(skb_shinfo(skb)->frags[0].page);
+ pfn = page_to_pfn(skb_frag_page(&skb_shinfo(skb)->frags[0]));
+ vaddr = page_address(skb_frag_page(&skb_shinfo(skb)->frags[0]));
req = RING_GET_REQUEST(&np->rx, req_prod + i);
gnttab_grant_foreign_access_ref(ref,
@@ -461,7 +461,7 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,
ref = gnttab_claim_grant_reference(&np->gref_tx_head);
BUG_ON((signed short)ref < 0);
- mfn = pfn_to_mfn(page_to_pfn(frag->page));
+ mfn = pfn_to_mfn(page_to_pfn(skb_frag_page(frag)));
gnttab_grant_foreign_access_ref(ref, np->xbdev->otherend_id,
mfn, GNTMAP_readonly);
@@ -768,8 +768,9 @@ static RING_IDX xennet_fill_frags(struct netfront_info *np,
while ((nskb = __skb_dequeue(list))) {
struct xen_netif_rx_response *rx =
RING_GET_RESPONSE(&np->rx, ++cons);
+ skb_frag_t *nfrag = &skb_shinfo(nskb)->frags[0];
- frag->page = skb_shinfo(nskb)->frags[0].page;
+ __skb_frag_set_page(frag, skb_frag_page(nfrag));
frag->page_offset = rx->offset;
frag->size = rx->status;
@@ -873,7 +874,7 @@ static int handle_incoming_queue(struct net_device *dev,
memcpy(skb->data, vaddr + offset,
skb_headlen(skb));
- if (page != skb_shinfo(skb)->frags[0].page)
+ if (page != skb_frag_page(&skb_shinfo(skb)->frags[0]))
__free_page(page);
/* Ethernet work: Delayed to here as it peeks the header. */
@@ -954,7 +955,8 @@ err:
}
}
- NETFRONT_SKB_CB(skb)->page = skb_shinfo(skb)->frags[0].page;
+ NETFRONT_SKB_CB(skb)->page =
+ skb_frag_page(&skb_shinfo(skb)->frags[0]);
NETFRONT_SKB_CB(skb)->offset = rx->offset;
len = rx->status;
@@ -968,7 +970,7 @@ err:
skb_shinfo(skb)->frags[0].size = rx->status - len;
skb->data_len = rx->status - len;
} else {
- skb_shinfo(skb)->frags[0].page = NULL;
+ skb_frag_set_page(skb, 0, NULL);
skb_shinfo(skb)->nr_frags = 0;
}
@@ -1143,7 +1145,8 @@ static void xennet_release_rx_bufs(struct netfront_info *np)
if (!xen_feature(XENFEAT_auto_translated_physmap)) {
/* Remap the page. */
- struct page *page = skb_shinfo(skb)->frags[0].page;
+ const struct page *page =
+ skb_frag_page(&skb_shinfo(skb)->frags[0]);
unsigned long pfn = page_to_pfn(page);
void *vaddr = page_address(page);
@@ -1650,6 +1653,8 @@ static int xennet_connect(struct net_device *dev)
/* Step 2: Rebuild the RX buffer freelist and the RX ring itself. */
for (requeue_idx = 0, i = 0; i < NET_RX_RING_SIZE; i++) {
+ skb_frag_t *frag;
+ const struct page *page;
if (!np->rx_skbs[i])
continue;
@@ -1657,10 +1662,11 @@ static int xennet_connect(struct net_device *dev)
ref = np->grant_rx_ref[requeue_idx] = xennet_get_rx_ref(np, i);
req = RING_GET_REQUEST(&np->rx, requeue_idx);
+ frag = &skb_shinfo(skb)->frags[0];
+ page = skb_frag_page(frag);
gnttab_grant_foreign_access_ref(
ref, np->xbdev->otherend_id,
- pfn_to_mfn(page_to_pfn(skb_shinfo(skb)->
- frags->page)),
+ pfn_to_mfn(page_to_pfn(page)),
0);
req->gref = ref;
req->id = requeue_idx;
--
1.7.2.5
^ permalink raw reply related
* [PATCH 59/75] virtionet: convert to SKB paged frag API.
From: Ian Campbell @ 2011-08-19 13:27 UTC (permalink / raw)
To: netdev; +Cc: Michael S. Tsirkin, linux-kernel, virtualization, Ian Campbell
In-Reply-To: <1313760393.5010.356.camel@zakaz.uk.xensource.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/virtio_net.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..52667a8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -149,7 +149,7 @@ static void set_skb_frag(struct sk_buff *skb, struct page *page,
f = &skb_shinfo(skb)->frags[i];
f->size = min((unsigned)PAGE_SIZE - offset, *len);
f->page_offset = offset;
- f->page = page;
+ __skb_frag_set_page(f, page);
skb->data_len += f->size;
skb->len += f->size;
--
1.7.2.5
^ permalink raw reply related
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: jamal @ 2011-08-19 12:49 UTC (permalink / raw)
To: San Mehat
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <20110818220756.5C93E5C80B@san.sea.corp.google.com>
On Thu, 2011-08-18 at 15:07 -0700, San Mehat wrote:
> TL;DR
> -----
> In this RFC we propose the introduction of the concept of hardware socket
> offload to the Linux kernel. Patches will accompany this RFC in a few days,
> but we felt we had enough on the design to solicit constructive discussion
> from the community at-large.
>
[..]
> ALTERNATIVE STRATEGIES
> ----------------------
>
> An alternative strategy for providing similar functionality involves either
> modifying glibc or using LD_PRELOAD tricks to intercept socket calls. We were
> forced to rule this out due to the complexity (and fragility) involved with
> attempting to maintain a general solution compatible across various
> distributions where platform-libraries differ.
Above should have been in your TL;DR;->
LD_PRELOAD is also horrible because of the granularity of the socket
calls;
Having things in the kernel and specifically tagging socket as needing
this feature is much much more manageable.
Tying things to virtualization may miss the big picture because there
are many other use cases for intercepting socket calls, example:
Samir Bellabes <sam@synack.fr> has been trying to get what he refers to
as "personal firewall" (equivalent to the silly windows firewall) which
prompts the user "ping from blah, do you want to allow a response?"
That requires intercepting send/recv calls, prompt the user in possibly
some pop-up and act based on response. It requires looking at content.
He is trying to use selinux for that interface,
but i think this would be the right abstraction.
I hope you plan to support send/recv.
I also hope you add support for SOCK_RAW (and maybe SOCK_PACKET).
Q: If you want this to be transparent to the apps, who/what is doing
the tagging of SOCK_HWASSIST? clearly not the app if you dont want to
change it.
I like the uri abstraction if it doesnt come at the expense of the
app transparency.
cheers,
jamal
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: Alan Cox @ 2011-08-19 9:28 UTC (permalink / raw)
To: San Mehat
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <CAPi7mHqV6Pgk5tSt+fJn_2aEZKZbO=aO78JXgw7MvDaW0musoQ@mail.gmail.com>
> I have no desire to change the 'genericness' of sockets.. just the
> opposite - i wish to
> introduce the notion that sockets (can be) completely generic (when
> offloaded) as far as
> the guest is concerned.
I suppose my concern is that you don't want to design for a specific
offload device, your offload might change but the view from the
application side should not differ.
> > This guest only view means you can't use the abstraction for local
> > sockets too.
> >
>
> To be honest, the way we're attempting to integrate is in such a way
> that you *could*
> offload AF_LOCAL sockets... but that world gets a bit too much like
> the 'Twilight Zone'
> for my current linkings..
Until you want to be able to have a pair of apps talking that may or may
not be on different systems and may or may not be on a vm host at all, at
which point having the same acceleration between them (a null accelerator
so to speak) would avoid having to add extra paths to the apps.
> > And yes there is still the complicated cases such as 'the routing table
> > has changed from vitual host to via siberia now what' but I don't believe
> > your proposal addresses that either.
>
> Can you be more specific? If you mean solving the 'keeping your tcp connections
> open to non virtual endpoints across a migration (or whatever)' then
> no it doesn't :)
That was my assumption.
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: David Miller @ 2011-08-19 3:39 UTC (permalink / raw)
To: san
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro
In-Reply-To: <20110818220756.5C93E5C80B@san.sea.corp.google.com>
I'm not reading any RFC without any example code, sorry.
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: San Mehat @ 2011-08-18 23:18 UTC (permalink / raw)
To: Alan Cox
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <20110818235719.50365b0b@lxorguk.ukuu.org.uk>
On Thu, Aug 18, 2011 at 3:57 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> The Berkeley sockets coprocessor is a virtual PCI device which has the ability
>> to offload socket activity from an unmodified application at the BSD sockets
>
> Ok I think there is an important question here. Why is this being
> designed for a specific virtual interface. Unix has always had the notion
> that socket operations can be in part generic and that you can pass a
> properly designed program a socket without any notion of what it is for.
Sorry Alan if I wasn't clear, but I'm not quite sure what you're asking...
If you're asking 'why have you only spec'ed out a virtual interface
for this' then
my answer would be 'but of course you could design this in real hardware and
have a proper driver :)'. If you'd prefer that I call that out
specifically I'm happy to do so.
I have no desire to change the 'genericness' of sockets.. just the
opposite - i wish to
introduce the notion that sockets (can be) completely generic (when
offloaded) as far as
the guest is concerned.
>
>> Lastly, pushing socket processing back into the host allows for host-side
>> control of the network protocols used, which limits the potential congestion
>> problems that can arise when various guests are using their own congestion
>> control algorithms.
>
> Does that not depend which side does the congestion and who parcels out
> buffers ?
It does, and it does.
>
>> Since we wish to allow these paravirtualized sockets to coexist peacefully with
>> the existing Linux socket system, we've chosen to introduce the idea that a
>> socket can at some point transition from being managed by the O/S socket system
>> to a more enlightened 'hardware assisted' socket. The transition is managed by
>> a 'socket coprocessor' component which intercepts and gets first right of
>> refusal on handling certain global socket calls (connect, sendto, bind, etc...).
>> In this initial design, the policy on whether to transition a socket or not is
>> made by the virtual hardware, although we understand that further measurement
>> into operation latency is warranted.
>
> Q: whay happens about in process socket syscalls in another thread ?
> Thats always been the ugly in these cases either by intercepting or by
> swapping file operations on an object.
>
>> * SOCK_HWASSIST
>> Indicates socket operations are handled by hardware
>
> This guest only view means you can't use the abstraction for local
> sockets too.
>
To be honest, the way we're attempting to integrate is in such a way
that you *could*
offload AF_LOCAL sockets... but that world gets a bit too much like
the 'Twilight Zone'
for my current linkings..
>> In order to support a variety of socket address families, addresses are
>> converted from their native socket family to an opaque string. Our initial
>> design formats these strings as URIs. The currently supported conversions are:
>
> That makes a lot of sense to me, because its a well understood
> abstraction and you can offload other stuff to this kind of generic
> socket including things like http protocol acceleration, SSL and so on.
>
> Plus its always been annoying that you can't open a socket, but a URI
> interface solves that...
Indeed.
>
>> * We don't handle SOCK_SEQPACKET, SOCK_RAW, SOCK_RDM, or SOCK_PACKET sockets.
>
> But there is no reason SEQPACKET and RDM couldn't be added I assume?
No reason I can think of - we just did not have a specific requirement
for it at the time.
>
> Ok other questions
>
> Suppose instead you just add an abstracted socket interface of
>
> AF_SOMETHING, PF_URI
Mike Waychison and I were saving the 'PF_URI' discussion for a future
date, but indeed
we're on the same wave-length :). Our initial requirements are for an
'extremely minimal
burden of support' on the userspace environments, so we decided to
open up a separate
discussion on PF_URI
>
> it would be easy to convert programs. It would be easier to write
> properly generic programs. It would be easy write some small helpers that
> are a good deal less insane than the existing inet ones. At that point
> you could turn the problem on its head. Instead of 'borrowing' sockets
> for a fairly specific concept of hw assist you ask the reverse question,
> who can accelerate this URI be it some kind of virtual machine interface,
> something funky like raw data over infiniband, or plain old 'use the
> TCP/IP stack'.
Completely agree.
>
> Your decision making code is going to be interesting but it only has to
> make the decision once in simple cases.
Yup.
>
> And yes there is still the complicated cases such as 'the routing table
> has changed from vitual host to via siberia now what' but I don't believe
> your proposal addresses that either.
Can you be more specific? If you mean solving the 'keeping your tcp connections
open to non virtual endpoints across a migration (or whatever)' then
no it doesn't :)
>
> Alan
>
Thanks man,
-san
--
San Mehat | Staff Software Engineer | san@google.com | 415-366-6172
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: Alan Cox @ 2011-08-18 23:03 UTC (permalink / raw)
To: Alan Cox
Cc: mst, netdev, miche, linux-kernel, virtualization, San Mehat,
digitaleric, mikew, maccarro, davem
In-Reply-To: <20110818235719.50365b0b@lxorguk.ukuu.org.uk>
> Q: whay happens about in process socket syscalls in another thread ?
> Thats always been the ugly in these cases either by intercepting or by
> swapping file operations on an object.
Sorry I meant "in progress" 8)
^ permalink raw reply
* Re: [RFC 0/0] Introducing a generic socket offload framework
From: Alan Cox @ 2011-08-18 22:57 UTC (permalink / raw)
To: San Mehat
Cc: mst, netdev, miche, linux-kernel, virtualization, digitaleric,
mikew, maccarro, davem
In-Reply-To: <20110818220756.5C93E5C80B@san.sea.corp.google.com>
> The Berkeley sockets coprocessor is a virtual PCI device which has the ability
> to offload socket activity from an unmodified application at the BSD sockets
Ok I think there is an important question here. Why is this being
designed for a specific virtual interface. Unix has always had the notion
that socket operations can be in part generic and that you can pass a
properly designed program a socket without any notion of what it is for.
> Lastly, pushing socket processing back into the host allows for host-side
> control of the network protocols used, which limits the potential congestion
> problems that can arise when various guests are using their own congestion
> control algorithms.
Does that not depend which side does the congestion and who parcels out
buffers ?
> Since we wish to allow these paravirtualized sockets to coexist peacefully with
> the existing Linux socket system, we've chosen to introduce the idea that a
> socket can at some point transition from being managed by the O/S socket system
> to a more enlightened 'hardware assisted' socket. The transition is managed by
> a 'socket coprocessor' component which intercepts and gets first right of
> refusal on handling certain global socket calls (connect, sendto, bind, etc...).
> In this initial design, the policy on whether to transition a socket or not is
> made by the virtual hardware, although we understand that further measurement
> into operation latency is warranted.
Q: whay happens about in process socket syscalls in another thread ?
Thats always been the ugly in these cases either by intercepting or by
swapping file operations on an object.
> * SOCK_HWASSIST
> Indicates socket operations are handled by hardware
This guest only view means you can't use the abstraction for local
sockets too.
> In order to support a variety of socket address families, addresses are
> converted from their native socket family to an opaque string. Our initial
> design formats these strings as URIs. The currently supported conversions are:
That makes a lot of sense to me, because its a well understood
abstraction and you can offload other stuff to this kind of generic
socket including things like http protocol acceleration, SSL and so on.
Plus its always been annoying that you can't open a socket, but a URI
interface solves that...
> * We don't handle SOCK_SEQPACKET, SOCK_RAW, SOCK_RDM, or SOCK_PACKET sockets.
But there is no reason SEQPACKET and RDM couldn't be added I assume?
Ok other questions
Suppose instead you just add an abstracted socket interface of
AF_SOMETHING, PF_URI
it would be easy to convert programs. It would be easier to write
properly generic programs. It would be easy write some small helpers that
are a good deal less insane than the existing inet ones. At that point
you could turn the problem on its head. Instead of 'borrowing' sockets
for a fairly specific concept of hw assist you ask the reverse question,
who can accelerate this URI be it some kind of virtual machine interface,
something funky like raw data over infiniband, or plain old 'use the
TCP/IP stack'.
Your decision making code is going to be interesting but it only has to
make the decision once in simple cases.
And yes there is still the complicated cases such as 'the routing table
has changed from vitual host to via siberia now what' but I don't believe
your proposal addresses that either.
Alan
^ permalink raw reply
* [RFC 0/0] Introducing a generic socket offload framework
From: San Mehat @ 2011-08-18 22:07 UTC (permalink / raw)
To: davem, mst, rusty
Cc: netdev, miche, linux-kernel, mikew, digitaleric, virtualization,
maccarro
TL;DR
-----
In this RFC we propose the introduction of the concept of hardware socket
offload to the Linux kernel. Patches will accompany this RFC in a few days,
but we felt we had enough on the design to solicit constructive discussion
from the community at-large.
BACKGROUND
----------
Many applications within enterprise organizations suitable for virtualization
neither require nor desire a connection to the full internal Ethernet+IP
network. Rather, some specific socket connections -- for processing HTTP
requests, making database queries, or interacting with storage -- are needed,
and IP networking in the application may typically be discouraged for
applications that do not sit on the edge of the network. Furthermore, removing
the application's need to understand where its inputs come from / go to within
the networking fabric can make save/restore/migration of a virtualized
application substantially easier - especially in large clusters and on fabrics
which can't handle IP re-assignment.
REQUIREMENTS
------------
* Allow VM connectivity to internal resources without requiring additional
network resources (IPs, VLANs, etc).
* Easy authentication of network streams from a trusted domain (vmm).
* Protect host-kernel & network-fabric from direct exposure to untrusted
packet data-structures.
* Support for multiple distributions of Linux.
* Minimal third-party software maintenance burden.
* To be able to co-exist with the existing network stack and ethernet virtual
devices in the event that an applications specific requirements cannot be
met by this design.
DESIGN
------
The Berkeley sockets coprocessor is a virtual PCI device which has the ability
to offload socket activity from an unmodified application at the BSD sockets
layer (Layer 4). Offloaded socket requests bypass the local operating systems
networking stack entirely via the card and are relayed into the VMM
(Virtual Machine Manager) for processing. The VMM then passes the request to a
socket backend for handling. The difference between a socket backend and a
traditional VM ethernet backend is that the socket backend receives layer 4
socket (STREAM/DGRAM) requests instead of a multiplexed stream of layer 2
packets (ethernet) that must be interpreted by the host. This technique also
improves security isolation as the guest is no longer constructing packets which
are evaluated by the host or underlying network fabric; packet construction
happens in the host.
Lastly, pushing socket processing back into the host allows for host-side
control of the network protocols used, which limits the potential congestion
problems that can arise when various guests are using their own congestion
control algorithms.
================================================================================
+-----------------------------------------------------------------+
| |
guest | unmodified application |
userspace +-----------------------------------------------------------------+
| unmodified libc |
+-----------------------------------------------------------------+
| / \
| |
=========================== | ============================ | ===================
| |
\ / |
+------------------------------------------------------+
| socket core |
+----+============+------------------------------------+
| INET | | / \
guest +-----+------+ | |
kernel | TCP | UDP | | |
+-----+------+ | L4 reqs |
| NETDEV | | |
+------------+ | |
| virtio_net | \ / |
+------------+ +------------------+
| / \ | hw_socket |
| | +------------------+
| | | virtio_socket |
| | +------------------+
| | | / \
========================= | == | ====================== | ====== | =============
\ / | \ / |
host +---------------------+ +------------------------+
userspace | virito net device | | virtio socket device |
(vmm) +---------------------+ +------------------------+
| ethernet backend | | socket backend |
+---------------------+ +------------------------+
| / \ | / \
L2 | | | | L4
packets | | \ / | requests
| | +-----------------------+
| | | Socket Handlers |
| | +-----------------------+
| | | / \
======================= | ==== | ===================== | ======= | =============
| | | |
host \ / | \ / |
kernel
================================================================================
One of the most appealing aspects of this design (to application developers) is
that this approach can be completely transparent to the application, provided
we're able to intercept the application's socket requests in such a way that we
do not impact performance in a negative fashion, yet retain the API semantics
the application expects. In the event that this design is not suitable for an
application, the virtual machine may be also fitted with a normal virtual
ethernet device in addition to the co-processor (as shown in the diagram above).
Since we wish to allow these paravirtualized sockets to coexist peacefully with
the existing Linux socket system, we've chosen to introduce the idea that a
socket can at some point transition from being managed by the O/S socket system
to a more enlightened 'hardware assisted' socket. The transition is managed by
a 'socket coprocessor' component which intercepts and gets first right of
refusal on handling certain global socket calls (connect, sendto, bind, etc...).
In this initial design, the policy on whether to transition a socket or not is
made by the virtual hardware, although we understand that further measurement
into operation latency is warranted.
In the event the determination is made to transition a socket to hw-assisted
mode, the socket is marked as being assisted by hardware, and all socket
operations are offloaded to hardware.
The following flag values have been added to struct socket (only visible within
the guest kernel):
* SOCK_HWASSIST
Indicates socket operations are handled by hardware
In order to support a variety of socket address families, addresses are
converted from their native socket family to an opaque string. Our initial
design formats these strings as URIs. The currently supported conversions are:
+-----------------------------------------------------------------------------+
| Domain | Type | URI example conversion |
| AF_INET | SOCK_STREAM | tcp://x.x.x.x:yyyy |
| AF_INET | SOCK_DGRAM | udp://x.x.x.x:yyyy |
| AF_INET6 | SOCK_STREAM | tcp6://aaaa:b:cccc:d:eeee:ffff:gggg:hhhh/ii |
| AF_INET6 | SOCK_DGRAM | udp6://aaaa:b:cccc:d:eeee:ffff:gggg:hhhh/ii |
| AF_IPX | SOCK_DGRAM | ipx://xxxxxxxx.yyyyyyyyyy.zzzz |
+-----------------------------------------------------------------------------+
In order for the socket coprocessor to take control of a socket, hooks must be
added to the socket core. Our initial implementation hooks a number of functions
in the socket-core (too many), and after consideration we feel we can reduce it
down considerably by managing the socket 'ops' pointers.
ALTERNATIVE STRATEGIES
----------------------
An alternative strategy for providing similar functionality involves either
modifying glibc or using LD_PRELOAD tricks to intercept socket calls. We were
forced to rule this out due to the complexity (and fragility) involved with
attempting to maintain a general solution compatible accross various
distributions where platform-libraries differ.
CAVEATS
-------
* We're currently hooked into too many socket calls. We should be able to
reduce the number of hooks to 3 (__sock_create(), sys_connect(), sys_bind()).
* Our 'hw_socket' component should be folded into a netdev so we can leverage
NAPI.
* We don't handle SOCK_SEQPACKET, SOCK_RAW, SOCK_RDM, or SOCK_PACKET sockets.
* We don't currently have support for /proc/net. Our current plan is to
add '/proc/net/hwsock' (filename TBD) and add support for these sockets
to the net-tools packages (netstat & friends), rather than muck around with
plumbing hardware-assisted socket info into '/proc/net/tcp' and
'/proc/net/udp'.
* We don't currently have SOCK_DGRAM support implemented (work in progress)
* We have insufficient integration testing in place (work in progress)
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Avi Kivity @ 2011-08-18 18:30 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Pekka Enberg, Ingo Molnar
In-Reply-To: <1313690353.30799.22.camel@lappy>
On 08/18/2011 10:59 AM, Sasha Levin wrote:
> >
> > This is something that can be used by very few people, but everyone has
> > to carry it. It's more efficient to add statistics support to your
> > automation framework (involving the guest).
> >
>
> That was just one example of many possibilities. However, if you feel
> this can't be used within monitoring or management platforms anyhow then
> I'm ok with dropping it.
>
It really depends on the possibilities.
Adding features is fine, but we need to know they'll be useful to many
people.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Sasha Levin @ 2011-08-18 17:59 UTC (permalink / raw)
To: Avi Kivity
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Pekka Enberg, Ingo Molnar
In-Reply-To: <4E4D4DAC.8080304@redhat.com>
On Thu, 2011-08-18 at 10:36 -0700, Avi Kivity wrote:
> On 08/18/2011 09:29 AM, Sasha Levin wrote:
> > >
> > > What can you do with it?
> > >
> >
> > I was actually planning on submitting another patch that would add
> > something similar into virtio-net. My plan was to enable collecting
> > statistics regarding memory, network and disk usage in a simple manner
> > without accessing guests.
> >
> > How can this be used? my vision behind it was automation of kernel
> > testing and benchmarking.
> >
>
> This is something that can be used by very few people, but everyone has
> to carry it. It's more efficient to add statistics support to your
> automation framework (involving the guest).
>
That was just one example of many possibilities. However, if you feel
this can't be used within monitoring or management platforms anyhow then
I'm ok with dropping it.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Avi Kivity @ 2011-08-18 17:36 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Pekka Enberg, Ingo Molnar
In-Reply-To: <1313684984.30799.20.camel@lappy>
On 08/18/2011 09:29 AM, Sasha Levin wrote:
> >
> > What can you do with it?
> >
>
> I was actually planning on submitting another patch that would add
> something similar into virtio-net. My plan was to enable collecting
> statistics regarding memory, network and disk usage in a simple manner
> without accessing guests.
>
> How can this be used? my vision behind it was automation of kernel
> testing and benchmarking.
>
This is something that can be used by very few people, but everyone has
to carry it. It's more efficient to add statistics support to your
automation framework (involving the guest).
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Sasha Levin @ 2011-08-18 16:29 UTC (permalink / raw)
To: Avi Kivity
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Pekka Enberg, Ingo Molnar
In-Reply-To: <4E4D2B6D.1040908@redhat.com>
On Thu, 2011-08-18 at 08:10 -0700, Avi Kivity wrote:
> On 08/17/2011 09:38 PM, Sasha Levin wrote:
> > On Wed, 2011-08-17 at 16:00 -0700, Avi Kivity wrote:
> > > On 08/16/2011 12:47 PM, Sasha Levin wrote:
> > > > 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.
> > > >
> > > >
> > >
> > > Why can't you get the same info from the host? i.e. read sectors?
> >
> > Some of the stats you can collect from the host, but some you can't.
> >
> > The ones you can't include all the timing statistics and the internal
> > queue statistics (read/write merges).
>
> Surely you can time the actual amount of time the I/O takes? It doesn't
> account for the virtio round-trip, but does it matter?
>
> Why is the merge count important for the host?
>
I assumed that the time the request spends in the virtio layer is
(somewhat) significant, specially since that this is something that adds
up over time.
Merge count can be useful for several testing scenarios (I'll describe
the reasoning behind this patch below).
> >
> > The idea behind providing all of the stats on the stats vq (which is
> > basically what you see in '/dev/block/[device]/stats') is to give a
> > consistent snapshot of the state of the device.
> >
> >
>
> What can you do with it?
>
I was actually planning on submitting another patch that would add
something similar into virtio-net. My plan was to enable collecting
statistics regarding memory, network and disk usage in a simple manner
without accessing guests.
How can this be used? my vision behind it was automation of kernel
testing and benchmarking.
I created a simple set of scripts that does bisection of kernel issues
(which is a combination of 'git bisect run' scripts and a simple
framework that generates /sbin/init used for testing without having to
touch any guests or images.
The scripts builds a simple image which contains only whats required to
test the kernel, and runs the test kernel and test image to determine
whether we hit a 'git bisect good' or 'git bisect bad' automatically.
I was looking to expand it to allow detection of more than just patches
that break things, but also patches that caused a performance hit in a
specific scenario.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Avi Kivity @ 2011-08-18 15:10 UTC (permalink / raw)
To: Sasha Levin; +Cc: virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <1313642283.22532.4.camel@lappy>
On 08/17/2011 09:38 PM, Sasha Levin wrote:
> On Wed, 2011-08-17 at 16:00 -0700, Avi Kivity wrote:
> > On 08/16/2011 12:47 PM, Sasha Levin wrote:
> > > 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.
> > >
> > >
> >
> > Why can't you get the same info from the host? i.e. read sectors?
>
> Some of the stats you can collect from the host, but some you can't.
>
> The ones you can't include all the timing statistics and the internal
> queue statistics (read/write merges).
Surely you can time the actual amount of time the I/O takes? It doesn't
account for the virtio round-trip, but does it matter?
Why is the merge count important for the host?
>
> The idea behind providing all of the stats on the stats vq (which is
> basically what you see in '/dev/block/[device]/stats') is to give a
> consistent snapshot of the state of the device.
>
>
What can you do with it?
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply
* [PATCH] virtio-blk: Update spec with new stats vq
From: Sasha Levin @ 2011-08-18 7:53 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, Avi Kivity
The diff below is an update to virtio-spec with specification of a stats
vq added to virtio-blk.
The idea behind adding a stats vq is to simplify pull of information
which exists only on the guest and can't be collected on the host.
The idea is basically same as the stats vq which exists in
virtio-balloon and reporting host memory statistics.
---
diff --git a/old/virtio-spec.lyx b/new/virtio-spec.lyx
index 448af76..8c3fe7b 100644
--- a/old/virtio-spec.lyx
+++ b/new/virtio-spec.lyx
@@ -58,6 +58,7 @@
\html_be_strict false
\author -608949062 "Rusty Russell,,,"
\author 1 "Michael S. Tsirkin"
+\author 2090695081 "Sasha Levin"
\end_header
\begin_body
@@ -5224,7 +5225,24 @@ ID 2
\end_layout
\begin_layout Description
-Virtqueues 0:requestq.
+Virtqueues 0:requestq
+\change_inserted 2090695081 1313645732
+.
+ 1:statsq.
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313645739
+Only if VIRTIO_BLK_F_STATS_VQ set
+\end_layout
+
+\end_inset
+
+
+\change_unchanged
+.
\end_layout
\begin_layout Description
@@ -5330,6 +5348,20 @@ VIRTIO_BLK_F_SECTOR_MAX
\end_inset
(10) Maximum total sectors in an I/O.
+\change_inserted 2090695081 1313645792
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 2090695081 1313645806
+VIRTIO_BLK_F_STATS_VQ
+\begin_inset space \space{}
+\end_inset
+
+(11) A virtqueue for reporting guest block statistics is present.
+\change_unchanged
+
\end_layout
\end_deeper
@@ -5449,8 +5481,32 @@ If the VIRTIO_BLK_F_RO feature is set by the
device, any write requests
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_inserted 2090695081 1313653667
+
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313653667
+If the VIRTIO_BLK_F_STATS_VQ feature bit is negotiated:
+\end_layout
+
+\begin_deeper
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313653667
+Identify the stats virtqueue.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313653667
+Add one empty buffer to the stats virtqueue and notify the host.
+\change_unchanged
+
\end_layout
+\end_deeper
\begin_layout Section*
Device Operation
\end_layout
@@ -5849,6 +5905,312 @@ residual
status
\emph default
field is a separate read-only buffer of size 1 byte, by itself.
+\change_inserted 2090695081 1313646835
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 2090695081 1313646856
+Block
+\begin_inset space \space{}
+\end_inset
+
+Statistics
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313646867
+The stats virtqueue is atypical because communication is driven by the
device
+ (not the driver).
+ The channel becomes active at driver initialization time when the
driver
+ adds an empty buffer and notifies the device.
+ A request for block statistics proceeds as follows:
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313646835
+The device pushes the buffer onto the used ring and sends an interrupt.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313646835
+The driver pops the used buffer and discards it.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313646878
+The driver collects block statistics and writes them into a new buffer.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313646835
+The driver adds the buffer to the virtqueue and notifies the device.
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 2090695081 1313646835
+The device pops the buffer (retaining it to initiate a subsequent
request)
+ and consumes the statistics.
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 2090695081 1313646925
+Block
+\begin_inset space \space{}
+\end_inset
+
+Statistics
+\begin_inset space \space{}
+\end_inset
+
+Format Each statistic consists of a 16 bit tag and a 64 bit value.
+ Both quantities are represented in the native endian of the guest.
+ All statistics are optional and the driver may choose which ones to
supply.
+ To guarantee backwards compatibility, unsupported statistics should be
+ omitted.
+\end_layout
+
+\begin_deeper
+\begin_layout Standard
+
+\change_inserted 2090695081 1313646835
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313646940
+
+struct virtio_blk_stat {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651887
+
+#define VIRTIO_BLK_S_READ_IO 0
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651942
+
+#define VIRTIO_BLK_S_READ_MERGES 1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651940
+
+#define VIRTIO_BLK_S_READ_SECTORS 2
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651939
+
+#define VIRTIO_BLK_S_READ_TICKS 3
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651938
+
+#define VIRTIO_BLK_S_WRITE_IO 4
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651936
+
+#define VIRTIO_BLK_S_WRITE_MERGES 5
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651935
+
+#define VIRTIO_BLK_S_WRITE_SECTORS 6
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651934
+
+#define VIRTIO_BLK_S_WRITE_TICKS 7
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651932
+
+#define VIRTIO_BLK_S_IN_FLIGHT 8
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651931
+
+#define VIRTIO_BLK_S_IO_TICKS 9
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313651930
+
+#define VIRTIO_BLK_S_TIME_IN_QUEUE 10
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313646835
+
+ u16 tag;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313646835
+
+ u64 val;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 2090695081 1313646835
+
+} __attribute__((packed));
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\end_deeper
+\begin_layout Description
+
+\change_inserted 2090695081 1313651968
+Tags
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652455
+
+\series bold
+VIRTIO_BLK_S_READ_IO
+\series default
+Number of read I/Os processed.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652609
+
+\series bold
+VIRTIO_BLK_S_READ_MERGE
+\series default
+S Number of read I/Os merged with in-queue I/O.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652608
+
+\series bold
+VIRTIO_BLK_S_READ_SECTORS
+\series default
+Number of sectors read.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652607
+
+\series bold
+VIRTIO_BLK_S_READ_TICKS
+\series default
+Total wait time for read requests.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652606
+
+\series bold
+VIRTIO_BLK_S_WRITE_IO
+\series default
+Number of write I/Os processed.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652605
+
+\series bold
+VIRTIO_BLK_S_WRITE_MERGES
+\series default
+Number of write I/Os merged with in-queue I/O.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652604
+
+\series bold
+VIRTIO_BLK_S_WRITE_SECTORS
+\series default
+Number of sectors written.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652604
+
+\series bold
+VIRTIO_BLK_S_WRITE_TICKS
+\series default
+Total wait time for write requests.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652603
+
+\series bold
+VIRTIO_BLK_S_IN_FLIGHT
+\series default
+Number of I/Os currently in flight.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652602
+
+\series bold
+VIRTIO_BLK_S_IO_TICKS
+\series default
+Total time this block device has been active.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 2090695081 1313652601
+
+\series bold
+VIRTIO_BLK_S_TIME_IN_QUEUE
+\series default
+Total wait time for all requests.
\end_layout
\begin_layout Chapter*
--
Sasha.
^ permalink raw reply related
* RE: [PATCH 2/8] Staging: hv: vmbus: Invoke vmbus_on_msg_dpc() directly
From: Sasha Levin @ 2011-08-18 5:50 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E04808194C66@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, 2011-08-17 at 18:43 +0000, KY Srinivasan wrote:
>
> > -----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.
Hm..
Looking at __tasklet_schedule() it should be guaranteed to run on the
same CPU, but as you said - the comment above is actually saying the
opposite.
The relevant documentation in the 'Linux Device Drivers' book also
states that it is guaranteed, so maybe we can get a confirmation from
Greg that this is still true and update the comment instead?
Not using bottom halves and just calling the dpc handler would mean that
you block interrupts for much longer than you need to.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Sasha Levin @ 2011-08-18 4:38 UTC (permalink / raw)
To: Avi Kivity; +Cc: virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <4E4C481A.5080205@redhat.com>
On Wed, 2011-08-17 at 16:00 -0700, Avi Kivity wrote:
> On 08/16/2011 12:47 PM, Sasha Levin wrote:
> > 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.
> >
> >
>
> Why can't you get the same info from the host? i.e. read sectors?
Some of the stats you can collect from the host, but some you can't.
The ones you can't include all the timing statistics and the internal
queue statistics (read/write merges).
The idea behind providing all of the stats on the stats vq (which is
basically what you see in '/dev/block/[device]/stats') is to give a
consistent snapshot of the state of the device.
>
> Patches to virtio drivers should be preceded by specification updates so
> that guest driver authors and alternative userspace developers have a
> solid reference.
>
I'll send an additional update to the spec.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Avi Kivity @ 2011-08-17 23:00 UTC (permalink / raw)
To: Sasha Levin; +Cc: virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <1313524071-27250-1-git-send-email-levinsasha928@gmail.com>
On 08/16/2011 12:47 PM, Sasha Levin wrote:
> 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.
>
>
Why can't you get the same info from the host? i.e. read sectors?
Patches to virtio drivers should be preceded by specification updates so
that guest driver authors and alternative userspace developers have a
solid reference.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ 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