* [PATCH 4/6] staging: hv: remove commented out code in netvsc_remove()
From: Haiyang Zhang @ 2011-05-23 16:03 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
In-Reply-To: <1306166630-17336-1-git-send-email-haiyangz@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/netvsc_drv.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index ad25433..6a2f17d 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -395,7 +395,6 @@ static int netvsc_remove(struct hv_device *dev)
/* Stop outbound asap */
netif_stop_queue(net);
- /* netif_carrier_off(net); */
unregister_netdev(net);
--
1.6.3.2
^ permalink raw reply related
* [PATCH 3/6] staging: hv: change rndis_filter_device_remove() to void return type
From: Haiyang Zhang @ 2011-05-23 16:03 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
In-Reply-To: <1306166630-17336-1-git-send-email-haiyangz@microsoft.com>
rndis_filter_device_remove() always return 0, so change it to void return
type. Also cleaned up the error checking in the caller.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/hyperv_net.h | 2 +-
drivers/staging/hv/netvsc_drv.c | 9 ++-------
drivers/staging/hv/rndis_filter.c | 4 +---
3 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/hv/hyperv_net.h b/drivers/staging/hv/hyperv_net.h
index 315097d..6226dd3 100644
--- a/drivers/staging/hv/hyperv_net.h
+++ b/drivers/staging/hv/hyperv_net.h
@@ -101,7 +101,7 @@ int rndis_filter_open(struct hv_device *dev);
int rndis_filter_close(struct hv_device *dev);
int rndis_filte_device_add(struct hv_device *dev,
void *additional_info);
-int rndis_filter_device_remove(struct hv_device *dev);
+void rndis_filter_device_remove(struct hv_device *dev);
int rndis_filter_receive(struct hv_device *dev,
struct hv_netvsc_packet *pkt);
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index e716d4d..ad25433 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -387,7 +387,6 @@ static int netvsc_probe(struct hv_device *dev)
static int netvsc_remove(struct hv_device *dev)
{
struct net_device *net = dev_get_drvdata(&dev->device);
- int ret;
if (net == NULL) {
dev_err(&dev->device, "No net device to remove\n");
@@ -404,14 +403,10 @@ static int netvsc_remove(struct hv_device *dev)
* Call to the vsc driver to let it know that the device is being
* removed
*/
- ret = rndis_filter_device_remove(dev);
- if (ret != 0) {
- /* TODO: */
- netdev_err(net, "unable to remove vsc device (ret %d)\n", ret);
- }
+ rndis_filter_device_remove(dev);
free_netdev(net);
- return ret;
+ return 0;
}
/* The one and only one */
diff --git a/drivers/staging/hv/rndis_filter.c b/drivers/staging/hv/rndis_filter.c
index 60ebdb1..572cec6 100644
--- a/drivers/staging/hv/rndis_filter.c
+++ b/drivers/staging/hv/rndis_filter.c
@@ -741,7 +741,7 @@ int rndis_filte_device_add(struct hv_device *dev,
return ret;
}
-int rndis_filter_device_remove(struct hv_device *dev)
+void rndis_filter_device_remove(struct hv_device *dev)
{
struct netvsc_device *net_dev = dev->ext;
struct rndis_device *rndis_dev = net_dev->extension;
@@ -753,8 +753,6 @@ int rndis_filter_device_remove(struct hv_device *dev)
net_dev->extension = NULL;
netvsc_device_remove(dev);
-
- return 0;
}
--
1.6.3.2
^ permalink raw reply related
* [PATCH 2/6] staging: hv: remove commented out code from netvsc_drv.c
From: Haiyang Zhang @ 2011-05-23 16:03 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
In-Reply-To: <1306166630-17336-1-git-send-email-haiyangz@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/netvsc_drv.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index 456d3df..e716d4d 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -156,9 +156,6 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
/* Setup the rndis header */
packet->page_buf_cnt = num_pages;
- /* TODO: Flush all write buffers/ memory fence ??? */
- /* wmb(); */
-
/* Initialize it from the skb */
packet->total_data_buflen = skb->len;
--
1.6.3.2
^ permalink raw reply related
* [PATCH 1/6] staging: hv: remove unnecessary code in netvsc_probe().
From: Haiyang Zhang @ 2011-05-23 16:03 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
netif_carrier_off() was called earlier in this function, and there is
no other thread access this device yet. The status checking code is not
necessary here.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/netvsc_drv.c | 12 +-----------
1 files changed, 1 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index 7b9c229..456d3df 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -364,17 +364,7 @@ static int netvsc_probe(struct hv_device *dev)
return ret;
}
- /*
- * If carrier is still off ie we did not get a link status callback,
- * update it if necessary
- */
- /*
- * FIXME: We should use a atomic or test/set instead to avoid getting
- * out of sync with the device's link status
- */
- if (!netif_carrier_ok(net))
- if (!device_info.link_state)
- netif_carrier_on(net);
+ netif_carrier_on(net);
memcpy(net->dev_addr, device_info.mac_adr, ETH_ALEN);
--
1.6.3.2
^ permalink raw reply related
* RE: vmbus driver
From: Thomas Gleixner @ 2011-05-23 13:51 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, johnstul@us.ibm.com, gregkh@suse.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481FC6DF@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Mon, 23 May 2011, KY Srinivasan wrote:
> I am working on getting Hyper-V drivers (drivers/staging/hv/*) out of staging.
> I would like to request you to look at the Hyper-V timesource driver:
> drivers/staging/hv/hv_timesource.c. The supporting code for this driver
> is already part of the base kernel. Let me know if this driver is ready to exit staging.
Can you please send a patch against drivers/clocksource (the staging
part is uninteresting for review).
Thanks,
tglx
^ permalink raw reply
* RE: vmbus driver
From: KY Srinivasan @ 2011-05-23 12:54 UTC (permalink / raw)
To: Christoph Hellwig, Thomas Gleixner (tglx@linutronix.de),
johnstul@us.ibm.com
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110522105941.GB8346@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Sunday, May 22, 2011 7:00 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: vmbus driver
>
> > I see maintainers for each of the clocksource drivers and I see John Stultz and
> > Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign-
> off
> > on the Hyper-V clocksource.
>
> just send it to both of the with linux-kernel in Cc, and either of them
> will probably put it in.
>
John, Thomas,
I am working on getting Hyper-V drivers (drivers/staging/hv/*) out of staging.
I would like to request you to look at the Hyper-V timesource driver:
drivers/staging/hv/hv_timesource.c. The supporting code for this driver
is already part of the base kernel. Let me know if this driver is ready to exit staging.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-23 11:19 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <87boyutbjg.fsf@rustcorp.com.au>
On Mon, May 23, 2011 at 11:37:15AM +0930, Rusty Russell wrote:
> On Sun, 22 May 2011 15:10:08 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sat, May 21, 2011 at 11:49:59AM +0930, Rusty Russell wrote:
> > > On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > Current code might introduce a lot of latency variation
> > > > if there are many pending bufs at the time we
> > > > attempt to transmit a new one. This is bad for
> > > > real-time applications and can't be good for TCP either.
> > >
> > > Do we have more than speculation to back that up, BTW?
> >
> > Need to dig this up: I thought we saw some reports of this on the list?
>
> I think so too, but a reference needs to be here too.
>
> It helps to have exact benchmarks on what's being tested, otherwise we
> risk unexpected interaction with the other optimization patches.
>
> > > > struct sk_buff *skb;
> > > > unsigned int len;
> > > > -
> > > > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > > > + bool c;
> > > > + int n;
> > > > +
> > > > + /* We try to free up at least 2 skbs per one sent, so that we'll get
> > > > + * all of the memory back if they are used fast enough. */
> > > > + for (n = 0;
> > > > + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) &&
> > > > + ((skb = virtqueue_get_buf(vi->svq, &len)));
> > > > + ++n) {
> > > > pr_debug("Sent skb %p\n", skb);
> > > > vi->dev->stats.tx_bytes += skb->len;
> > > > vi->dev->stats.tx_packets++;
> > > > dev_kfree_skb_any(skb);
> > > > }
> > > > + return !c;
> > >
> > > This is for() abuse :)
> > >
> > > Why is the capacity check in there at all? Surely it's simpler to try
> > > to free 2 skbs each time around?
> >
> > This is in case we can't use indirect: we want to free up
> > enough buffers for the following add_buf to succeed.
>
> Sure, or we could just count the frags of the skb we're taking out,
> which would be accurate for both cases and far more intuitive.
>
> ie. always try to free up twice as much as we're about to put in.
>
> Can we hit problems with OOM? Sure, but no worse than now...
> The problem is that this "virtqueue_get_capacity()" returns the worst
> case, not the normal case. So using it is deceptive.
>
Maybe just document this?
I still believe capacity really needs to be decided
at the virtqueue level, not in the driver.
E.g. with indirect each skb uses a single entry: freeing
1 small skb is always enough to have space for a large one.
I do understand how it seems a waste to leave direct space
in the ring while we might in practice have space
due to indirect. Didn't come up with a nice way to
solve this yet - but 'no worse than now :)'
> > I just wanted to localize the 2+MAX_SKB_FRAGS logic that tries to make
> > sure we have enough space in the buffer. Another way to do
> > that is with a define :).
>
> To do this properly, we should really be using the actual number of sg
> elements needed, but we'd have to do most of xmit_skb beforehand so we
> know how many.
>
> Cheers,
> Rusty.
Maybe I'm confused here. The problem isn't the failing
add_buf for the given skb IIUC. What we are trying to do here is stop
the queue *before xmit_skb fails*. We can't look at the
number of fragments in the current skb - the next one can be
much larger. That's why we check capacity after xmit_skb,
not before it, right?
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Rusty Russell @ 2011-05-23 2:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <20110522121008.GA12155@redhat.com>
On Sun, 22 May 2011 15:10:08 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, May 21, 2011 at 11:49:59AM +0930, Rusty Russell wrote:
> > On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > Current code might introduce a lot of latency variation
> > > if there are many pending bufs at the time we
> > > attempt to transmit a new one. This is bad for
> > > real-time applications and can't be good for TCP either.
> >
> > Do we have more than speculation to back that up, BTW?
>
> Need to dig this up: I thought we saw some reports of this on the list?
I think so too, but a reference needs to be here too.
It helps to have exact benchmarks on what's being tested, otherwise we
risk unexpected interaction with the other optimization patches.
> > > struct sk_buff *skb;
> > > unsigned int len;
> > > -
> > > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > > + bool c;
> > > + int n;
> > > +
> > > + /* We try to free up at least 2 skbs per one sent, so that we'll get
> > > + * all of the memory back if they are used fast enough. */
> > > + for (n = 0;
> > > + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) &&
> > > + ((skb = virtqueue_get_buf(vi->svq, &len)));
> > > + ++n) {
> > > pr_debug("Sent skb %p\n", skb);
> > > vi->dev->stats.tx_bytes += skb->len;
> > > vi->dev->stats.tx_packets++;
> > > dev_kfree_skb_any(skb);
> > > }
> > > + return !c;
> >
> > This is for() abuse :)
> >
> > Why is the capacity check in there at all? Surely it's simpler to try
> > to free 2 skbs each time around?
>
> This is in case we can't use indirect: we want to free up
> enough buffers for the following add_buf to succeed.
Sure, or we could just count the frags of the skb we're taking out,
which would be accurate for both cases and far more intuitive.
ie. always try to free up twice as much as we're about to put in.
Can we hit problems with OOM? Sure, but no worse than now...
The problem is that this "virtqueue_get_capacity()" returns the worst
case, not the normal case. So using it is deceptive.
> I just wanted to localize the 2+MAX_SKB_FRAGS logic that tries to make
> sure we have enough space in the buffer. Another way to do
> that is with a define :).
To do this properly, we should really be using the actual number of sg
elements needed, but we'd have to do most of xmit_skb beforehand so we
know how many.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH RFC] virtio_net: fix patch: virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-22 17:32 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar2, habanero, kvm, steved, netdev, Shirley Ma,
linux-kernel, virtualization, borntraeger, Tom Lendacky, avi
In-Reply-To: <8739kb5era.fsf@rustcorp.com.au>
On Thu, May 19, 2011 at 05:00:17PM +0930, Rusty Russell wrote:
> On Thu, 19 May 2011 01:01:25 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > The patch virtio_net: limit xmit polling
> > got the logic reversed: it polled while we had
> > capacity not while ring was empty.
> >
> > Fix it up and clean up a bit by using a for loop.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > OK, turns out that patch was borken. Here's
> > a fix that survived stress test on my box.
> > Pushed on my branch, I'll send a rebased series
> > with Rusty's comments addressed ASAP.
>
> Normally you would have missed the merge window by now, but I'd really
> like this stuff in, so I'm holding it open for this. I want these patches
> in linux-next for at least a few days before I push them.
>
> If you think we're not close enough, please tell me and I'll push
> the rest of the virtio patches to Linus now.
>
> Thanks,
> Rusty.
I think it makes sense to push just the patches you have
applied by now (event index + delayed callback) - the
rest are close but they are guest only patches so very easy to
experiment with out of tree. OTOH if event index misses the
window it makes testing painful as we have to keep patching
both host and guest.
--
MST
^ permalink raw reply
* RE: vmbus driver
From: KY Srinivasan @ 2011-05-22 17:32 UTC (permalink / raw)
To: Christoph Hellwig
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110522105941.GB8346@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Sunday, May 22, 2011 7:00 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: vmbus driver
>
> > I see maintainers for each of the clocksource drivers and I see John Stultz and
> > Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign-
> off
> > on the Hyper-V clocksource.
>
> just send it to both of the with linux-kernel in Cc, and either of them
> will probably put it in.
>
Will do.
Thanks,
K. Y
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-22 12:10 UTC (permalink / raw)
To: Rusty Russell
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <877h9kvlps.fsf@rustcorp.com.au>
On Sat, May 21, 2011 at 11:49:59AM +0930, Rusty Russell wrote:
> On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Current code might introduce a lot of latency variation
> > if there are many pending bufs at the time we
> > attempt to transmit a new one. This is bad for
> > real-time applications and can't be good for TCP either.
>
> Do we have more than speculation to back that up, BTW?
Need to dig this up: I thought we saw some reports of this on the list?
> This patch is pretty sloppy; the previous ones were better polished.
>
> > -static void free_old_xmit_skbs(struct virtnet_info *vi)
> > +static bool free_old_xmit_skbs(struct virtnet_info *vi, int capacity)
> > {
>
> A comment here indicating it returns true if it frees something?
Agree.
> > struct sk_buff *skb;
> > unsigned int len;
> > -
> > - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> > + bool c;
> > + int n;
> > +
> > + /* We try to free up at least 2 skbs per one sent, so that we'll get
> > + * all of the memory back if they are used fast enough. */
> > + for (n = 0;
> > + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) &&
> > + ((skb = virtqueue_get_buf(vi->svq, &len)));
> > + ++n) {
> > pr_debug("Sent skb %p\n", skb);
> > vi->dev->stats.tx_bytes += skb->len;
> > vi->dev->stats.tx_packets++;
> > dev_kfree_skb_any(skb);
> > }
> > + return !c;
>
> This is for() abuse :)
>
> Why is the capacity check in there at all? Surely it's simpler to try
> to free 2 skbs each time around?
This is in case we can't use indirect: we want to free up
enough buffers for the following add_buf to succeed.
> for (n = 0; n < 2; n++) {
> skb = virtqueue_get_buf(vi->svq, &len);
> if (!skb)
> break;
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> dev_kfree_skb_any(skb);
> }
>
> > static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
> > @@ -574,8 +582,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct virtnet_info *vi = netdev_priv(dev);
> > int capacity;
> >
> > - /* Free up any pending old buffers before queueing new ones. */
> > - free_old_xmit_skbs(vi);
> > + /* Free enough pending old buffers to enable queueing new ones. */
> > + free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS);
> >
> > /* Try to transmit */
> > capacity = xmit_skb(vi, skb);
> > @@ -609,9 +617,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> > netif_stop_queue(dev);
> > if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
> > /* More just got used, free them then recheck. */
> > - free_old_xmit_skbs(vi);
> > - capacity = virtqueue_get_capacity(vi->svq);
> > - if (capacity >= 2+MAX_SKB_FRAGS) {
> > + if (!likely(free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
>
> This extra argument to free_old_xmit_skbs seems odd, unless you have
> future plans?
>
> Thanks,
> Rusty.
I just wanted to localize the 2+MAX_SKB_FRAGS logic that tries to make
sure we have enough space in the buffer. Another way to do
that is with a define :).
--
MST
^ permalink raw reply
* Re: vmbus driver
From: Christoph Hellwig @ 2011-05-22 10:59 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481FC2ED@TK5EX14MBXC122.redmond.corp.microsoft.com>
> I see maintainers for each of the clocksource drivers and I see John Stultz and
> Thomas Gleixner listed as the maintainers for Timekeeping. Who should sign-off
> on the Hyper-V clocksource.
just send it to both of the with linux-kernel in Cc, and either of them
will probably put it in.
^ permalink raw reply
* Re: [PATCHv2 06/14] virtio: add api for delayed callbacks
From: Rusty Russell @ 2011-05-21 2:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <8f343dcaa996f6b10499468c49508ba9d6fb6f5a.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:11:14 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Add an API that tells the other side that callbacks
> should be delayed until a lot of work has been done.
> Implement using the new event_idx feature.
>
> Note: it might seem advantageous to let the drivers
> ask for a callback after a specific capacity has
> been reached. However, as a single head can
> free many entries in the descriptor table,
> we don't really have a clue about capacity
> until get_buf is called. The API is the simplest
> to implement at the moment, we'll see what kind of
> hints drivers can pass when there's more than one
> user of the feature.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Yes, I've applied this (and the next one which uses it in virtio_net),
despite my reservations about the API. But that is fixable...
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 05/14] virtio_test: support event index
From: Rusty Russell @ 2011-05-21 2:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <fbed57582b9e8d97c11f889937ea65f42eb03da2.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:11:05 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Add ability to test the new event idx feature,
> enable by default.
Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 04/14] vhost: support event index
From: Rusty Russell @ 2011-05-21 2:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <b227febf884dcf82dee9233e581c6216d0e9daa5.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:10:54 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Support the new event index feature. When acked,
> utilize it to reduce the # of interrupts sent to the guest.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied, even though it'd normally be in your tree, it's easier for me
to push all together.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 03/14] virtio_ring: support event idx feature
From: Rusty Russell @ 2011-05-21 2:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <960f3e3b260844011b004c81dbda0661c977b79a.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:10:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Support for the new event idx feature:
> 1. When enabling interrupts, publish the current avail index
> value to the host to get interrupts on the next update.
> 2. Use the new avail_event feature to reduce the number
> of exits from the guest.
Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 02/14] virtio ring: inline function to check for events
From: Rusty Russell @ 2011-05-21 2:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <16ce853af7a80d0f7cb0c1118ba8e19adc184ad0.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:10:27 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> With the new used_event and avail_event and features, both
> host and guest need similar logic to check whether events are
> enabled, so it helps to put the common code in the header.
>
> Note that Xen has similar logic for notification hold-off
> in include/xen/interface/io/ring.h with req_event and req_prod
> corresponding to event_idx + 1 and new_idx respectively.
> +1 comes from the fact that req_event and req_prod in Xen start at 1,
> while event index in virtio starts at 0.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 01/14] virtio: event index interface
From: Rusty Russell @ 2011-05-21 2:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <995d19795d84a76f9836f4d9484fb4ef8eae36e9.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:10:17 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Define a new feature bit for the guest and host to utilize
> an event index (like Xen) instead if a flag bit to enable/disable
> interrupts and kicks.
Applied.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 11/14] virtio: don't delay avail index update
From: Rusty Russell @ 2011-05-21 2:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <33d7df119ccbedc9b54fbe4b674076b4f7ba3499.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:12:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Update avail index immediately instead of upon kick:
> for virtio-net RX this helps parallelism with the host.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_ring.c | 28 +++++++++++++++++++---------
> 1 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index eed5f29..8218fe6 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -89,7 +89,7 @@ struct vring_virtqueue
> unsigned int num_free;
> /* Head of free buffer list. */
> unsigned int free_head;
> - /* Number we've added since last sync. */
> + /* Number we've added since last kick. */
> unsigned int num_added;
I always like to see obsolescent nomenclature cleaned up like this.
Thanks.
> /* Last used index we've seen. */
> @@ -174,6 +174,13 @@ int virtqueue_add_buf_gfp(struct virtqueue *_vq,
>
> BUG_ON(data == NULL);
>
> + /* Prevent drivers from adding more than num bufs without a kick. */
> + if (vq->num_added == vq->vring.num) {
> + printk(KERN_ERR "gaaa!!!\n");
> + END_USE(vq);
> + return -ENOSPC;
> + }
> +
I like "gaaa!" but it won't tell us which driver. How about the more
conventional:
if (WARN_ON(vq->num_added >= vq->vring.num)) {
END_USE(vq);
return -ENOSPC;
}
I'd really like to see the results of this patch. It's useless for
outgoing net traffic (we deal with one packet at a time) but perhaps a
flood of incoming packets would show something.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Rusty Russell @ 2011-05-21 2:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <aced8619c07018b5495f9ceb2c02d4fbf897a098.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:11:56 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Current code might introduce a lot of latency variation
> if there are many pending bufs at the time we
> attempt to transmit a new one. This is bad for
> real-time applications and can't be good for TCP either.
Do we have more than speculation to back that up, BTW?
This patch is pretty sloppy; the previous ones were better polished.
> -static void free_old_xmit_skbs(struct virtnet_info *vi)
> +static bool free_old_xmit_skbs(struct virtnet_info *vi, int capacity)
> {
A comment here indicating it returns true if it frees something?
> struct sk_buff *skb;
> unsigned int len;
> -
> - while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
> + bool c;
> + int n;
> +
> + /* We try to free up at least 2 skbs per one sent, so that we'll get
> + * all of the memory back if they are used fast enough. */
> + for (n = 0;
> + ((c = virtqueue_get_capacity(vi->svq) < capacity) || n < 2) &&
> + ((skb = virtqueue_get_buf(vi->svq, &len)));
> + ++n) {
> pr_debug("Sent skb %p\n", skb);
> vi->dev->stats.tx_bytes += skb->len;
> vi->dev->stats.tx_packets++;
> dev_kfree_skb_any(skb);
> }
> + return !c;
This is for() abuse :)
Why is the capacity check in there at all? Surely it's simpler to try
to free 2 skbs each time around?
for (n = 0; n < 2; n++) {
skb = virtqueue_get_buf(vi->svq, &len);
if (!skb)
break;
pr_debug("Sent skb %p\n", skb);
vi->dev->stats.tx_bytes += skb->len;
vi->dev->stats.tx_packets++;
dev_kfree_skb_any(skb);
}
> static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
> @@ -574,8 +582,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> struct virtnet_info *vi = netdev_priv(dev);
> int capacity;
>
> - /* Free up any pending old buffers before queueing new ones. */
> - free_old_xmit_skbs(vi);
> + /* Free enough pending old buffers to enable queueing new ones. */
> + free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS);
>
> /* Try to transmit */
> capacity = xmit_skb(vi, skb);
> @@ -609,9 +617,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> netif_stop_queue(dev);
> if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
> /* More just got used, free them then recheck. */
> - free_old_xmit_skbs(vi);
> - capacity = virtqueue_get_capacity(vi->svq);
> - if (capacity >= 2+MAX_SKB_FRAGS) {
> + if (!likely(free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
This extra argument to free_old_xmit_skbs seems odd, unless you have
future plans?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 09/14] virtio_net: fix TX capacity checks using new API
From: Rusty Russell @ 2011-05-21 2:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <42ca2494c92f572388e3ab4c6f613dd4f038361b.1305846412.git.mst@redhat.com>
On Fri, 20 May 2011 02:11:47 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> virtio net uses the number of sg entries to
> check for TX ring capacity freed. But this
> gives incorrect results when indirect buffers
> are used. Use the new capacity API instead.
OK, but this explanation needs enhancement, such as noting the actual
results of that miscalculation. Something like:
virtio_net uses the number of sg entries in the skb it frees to
calculate how many descriptors in the ring have just been made
available. But this value is an overestimate: with indirect buffers
each skb only uses one descriptor entry, meaning we may wake the queue
only to find we still can't transmit anything.
Using the new virtqueue_get_capacity() call, we can exactly determine
the remaining capacity, so we should use that instead.
But, here's the side effect:
> /* More just got used, free them then recheck. */
> - capacity += free_old_xmit_skbs(vi);
> + free_old_xmit_skbs(vi);
> + capacity = virtqueue_get_capacity(vi->svq);
> if (capacity >= 2+MAX_SKB_FRAGS) {
That capacity >= 2+MAX_SKB_FRAGS is too much for indirect buffers. This
means we waste 20 entries in the ring, but OTOH if we hit OOM we fall
back to direct buffers and we *will* need this.
Which means this comment in the driver is now wrong:
/* This can happen with OOM and indirect buffers. */
if (unlikely(capacity < 0)) {
if (net_ratelimit()) {
if (likely(capacity == -ENOMEM)) {
dev_warn(&dev->dev,
"TX queue failure: out of memory\n");
} else {
dev->stats.tx_fifo_errors++;
dev_warn(&dev->dev,
"Unexpected TX queue failure: %d\n",
capacity);
}
}
dev->stats.tx_dropped++;
kfree_skb(skb);
return NETDEV_TX_OK;
}
virtqueue_kick(vi->svq);
So I'm not applying this patch (nor the virtqueue_get_capacity
predeccessor) for the moment.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Arnd Bergmann @ 2011-05-20 19:59 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, Chris Metcalf
In-Reply-To: <201105201713.25364.arnd@arndb.de>
On Friday 20 May 2011 17:13:25 Arnd Bergmann wrote:
> On Friday 20 May 2011 17:00:47 Chris Metcalf wrote:
> > > Any chance you can still restructure the information? I would recommend
> > > making it a first-class procfs member, since the data is really per-task.
> > >
> > > You can add a conditional entry to tgid_base_stuff[] in fs/proc/base.c
> > > to make it show up for each pid, and then just have the per-task information
> > > in there to do the lookup the other way round:
> > >
> > > # cat /proc/484/hardwall
> > > 2x2 1,1 @2,1
> > >
> > > # cat /proc/479/hardwall
> > > 2x2 1,1 @1,1
> >
> > It's not unreasonable to do what you're suggesting, i.e. "what's this
> > task's hardwall?", but it's not something that we've come up with any kind
> > of use case for in the past, so I'm not currently planning to implement
> > this. If we did, I agree, your solution looks like the right one.
>
> It's fairly easy to aggregate in user space though, we do similar
> things for 'lsof' and 'top', which walk all of procfs in order
> to show the complete picture. This is obviously more overhead than
> walking the lists in the kernel, but still not an expensive
> operation, and it keeps the data format much simpler.
Another problem with the existing interface is that it doesn't currently
support PID name spaces. That could of course be retrofitted, but having
the data split by pid directory would make it work implicitly.
Another approach would be to have a /proc/hardwall/ directory with
one entry per hardwall instance, and symlinks from /proc/<pid>/hardwall
to the respective file.
Arnd
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Arnd Bergmann @ 2011-05-20 15:13 UTC (permalink / raw)
To: Chris Metcalf; +Cc: linux-kernel, virtualization
In-Reply-To: <4DD6821F.6060707@tilera.com>
On Friday 20 May 2011 17:00:47 Chris Metcalf wrote:
> > Any chance you can still restructure the information? I would recommend
> > making it a first-class procfs member, since the data is really per-task.
> >
> > You can add a conditional entry to tgid_base_stuff[] in fs/proc/base.c
> > to make it show up for each pid, and then just have the per-task information
> > in there to do the lookup the other way round:
> >
> > # cat /proc/484/hardwall
> > 2x2 1,1 @2,1
> >
> > # cat /proc/479/hardwall
> > 2x2 1,1 @1,1
>
> It's not unreasonable to do what you're suggesting, i.e. "what's this
> task's hardwall?", but it's not something that we've come up with any kind
> of use case for in the past, so I'm not currently planning to implement
> this. If we did, I agree, your solution looks like the right one.
It's fairly easy to aggregate in user space though, we do similar
things for 'lsof' and 'top', which walk all of procfs in order
to show the complete picture. This is obviously more overhead than
walking the lists in the kernel, but still not an expensive
operation, and it keeps the data format much simpler.
Arnd
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Chris Metcalf @ 2011-05-20 15:00 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel, virtualization
In-Reply-To: <201105201637.20925.arnd@arndb.de>
On 5/20/2011 10:37 AM, Arnd Bergmann wrote:
> On Friday 20 May 2011 16:26:57 Chris Metcalf wrote:
>>>>>> /proc/tile/hardwall
>>>>>> Information on the set of currently active hardwalls (note that
>>>>>> the implementation is already present in arch/tile/kernel/hardwall.c;
>>>>>> this change just enables it)
> Ah, I see. I didn't notice that it was in the other file. You are
> absolutely right, this does not belong into /sys/hypervisor and
> fits well into procfs, we just need to find the right place.
>> Perhaps in this case it would be reasonable to just have the hardwall
>> subsystem put the file in /proc/driver/hardwall, or even /proc/hardwall?
>> Or I could make the /dev/hardwall char device dump out the ASCII text that
>> we currently get from /proc/hardwall if you read from it, which is a little
>> weird but not inconceivable. For example it currently shows things like this:
>>
>> # cat /proc/tile/hardwall
>> 2x2 1,1 pids: 484@2,1 479@1,1
>> 2x2 0,3 pids:
>>
>> In this example "2x2 1,1" is a 2x2 grid of cpus starting at grid (x,y)
>> position (1,1), with task 484 bound to the cpu at (x,y) position (2,1).
> Any chance you can still restructure the information? I would recommend
> making it a first-class procfs member, since the data is really per-task.
>
> You can add a conditional entry to tgid_base_stuff[] in fs/proc/base.c
> to make it show up for each pid, and then just have the per-task information
> in there to do the lookup the other way round:
>
> # cat /proc/484/hardwall
> 2x2 1,1 @2,1
>
> # cat /proc/479/hardwall
> 2x2 1,1 @1,1
It's not unreasonable to do what you're suggesting, i.e. "what's this
task's hardwall?", but it's not something that we've come up with any kind
of use case for in the past, so I'm not currently planning to implement
this. If we did, I agree, your solution looks like the right one.
The proposed /proc/tile/hardwall really is intended as system-wide
information. Each hardwall (one line in the output file example above)
corresponds to a "struct file" that may be shared by multiple processes (or
threads). Processes may pass the "struct file" to other processes via fork
(and maybe exec), or by passing it over Unix sockets. Then those processes
can choose a cpu within a hardwall rectangle, affinitize to that cpu only,
"activate" the hardwall fd with an ioctl(), and then get access from the OS
so they can work together within a hardwall to exchange data across the
Tilera "user dynamic network" (a wormhole routed grid network that moves
data at 32 bits/cycle with almost no latency). Processes can create a new
hardwall as long as it doesn't overlap geometrically with any other
existing hardwall on the system.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Arnd Bergmann @ 2011-05-20 14:37 UTC (permalink / raw)
To: Chris Metcalf; +Cc: linux-kernel, virtualization
In-Reply-To: <4DD67A31.90802@tilera.com>
On Friday 20 May 2011 16:26:57 Chris Metcalf wrote:
> >>>> /proc/tile/hardwall
> >>>> Information on the set of currently active hardwalls (note that
> >>>> the implementation is already present in arch/tile/kernel/hardwall.c;
> >>>> this change just enables it)
> >> This one is not a hypervisor-related file. It just lists information about
> >> the set of Linux hardwalls currently active. Again, it's not primarily
> >> intended for programmatic use, but as a diagnostic tool.
> > same here, I'd still put it into the hypervisor structure.
>
> Since /proc/tile/hardwall has no connection to the hypervisor whatsoever,
> I'm reluctant to put it under /sys/hypervisor.
Ah, I see. I didn't notice that it was in the other file. You are
absolutely right, this does not belong into /sys/hypervisor and
fits well into procfs, we just need to find the right place.
> Perhaps in this case it would be reasonable to just have the hardwall
> subsystem put the file in /proc/driver/hardwall, or even /proc/hardwall?
> Or I could make the /dev/hardwall char device dump out the ASCII text that
> we currently get from /proc/hardwall if you read from it, which is a little
> weird but not inconceivable. For example it currently shows things like this:
>
> # cat /proc/tile/hardwall
> 2x2 1,1 pids: 484@2,1 479@1,1
> 2x2 0,3 pids:
>
> In this example "2x2 1,1" is a 2x2 grid of cpus starting at grid (x,y)
> position (1,1), with task 484 bound to the cpu at (x,y) position (2,1).
Any chance you can still restructure the information? I would recommend
making it a first-class procfs member, since the data is really per-task.
You can add a conditional entry to tgid_base_stuff[] in fs/proc/base.c
to make it show up for each pid, and then just have the per-task information
in there to do the lookup the other way round:
# cat /proc/484/hardwall
2x2 1,1 @2,1
# cat /proc/479/hardwall
2x2 1,1 @1,1
Arnd
^ 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