Re: [PATCHv2 10/14] virtio_net: limit xmit polling
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 m...@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
m...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Sunday, May 22, 2011 7:00 AM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 6/6] staging: hv: removed commented out code from rndis_filter_receive()
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/rndis_filter.c | 18 --
1 files changed, 0 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/hv/rndis_filter.c
b/drivers/staging/hv/rndis_filter.c
index b142aae..5a5bf64 100644
--- a/drivers/staging/hv/rndis_filter.c
+++ b/drivers/staging/hv/rndis_filter.c
@@ -372,24 +372,6 @@ int rndis_filter_receive(struct hv_device *dev,
pkt-page_buf[0].offset);
/* Make sure we got a valid rndis message */
- /*
-* FIXME: There seems to be a bug in set completion msg where its
-* MessageLength is 16 bytes but the ByteCount field in the xfer page
-* range shows 52 bytes
-* */
-#if 0
- if (pkt-total_data_buflen != rndis_hdr-msg_len) {
- kunmap_atomic(rndis_hdr - pkt-page_buf[0].offset,
- KM_IRQ0);
-
- dev_err(dev-device, invalid rndis message? (expected %u
- bytes got %u)...dropping this message!\n,
- rndis_hdr-msg_len,
- pkt-total_data_buflen);
- return -1;
- }
-#endif
-
if ((rndis_hdr-ndis_msg_type != REMOTE_NDIS_PACKET_MSG)
(rndis_hdr-msg_len sizeof(struct rndis_message))) {
dev_err(dev-device, incoming rndis message buffer overflow
--
1.6.3.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 2/6] staging: hv: remove commented out code from netvsc_drv.c
Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 3/6] staging: hv: change rndis_filter_device_remove() to void return type
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 haiya...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 1/6] staging: hv: remove unnecessary code in netvsc_probe().
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 haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 4/6] staging: hv: remove commented out code in netvsc_remove()
Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 5/6] staging: hv: fix typo in name rndis_filte_device_add()
rename rndis_filte_device_add to rndis_filter_device_add
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/hyperv_net.h |2 +-
drivers/staging/hv/netvsc_drv.c |2 +-
drivers/staging/hv/rndis_filter.c |2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/hyperv_net.h b/drivers/staging/hv/hyperv_net.h
index 6226dd3..cf762bd 100644
--- a/drivers/staging/hv/hyperv_net.h
+++ b/drivers/staging/hv/hyperv_net.h
@@ -99,7 +99,7 @@ int netvsc_recv_callback(struct hv_device *device_obj,
int netvsc_initialize(struct hv_driver *drv);
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,
+int rndis_filter_device_add(struct hv_device *dev,
void *additional_info);
void rndis_filter_device_remove(struct hv_device *dev);
int rndis_filter_receive(struct hv_device *dev,
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index 6a2f17d..f510959 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -352,7 +352,7 @@ static int netvsc_probe(struct hv_device *dev)
/* Notify the netvsc driver of the new device */
device_info.ring_size = ring_size;
- ret = rndis_filte_device_add(dev, device_info);
+ ret = rndis_filter_device_add(dev, device_info);
if (ret != 0) {
free_netdev(net);
dev_set_drvdata(dev-device, NULL);
diff --git a/drivers/staging/hv/rndis_filter.c
b/drivers/staging/hv/rndis_filter.c
index 572cec6..b142aae 100644
--- a/drivers/staging/hv/rndis_filter.c
+++ b/drivers/staging/hv/rndis_filter.c
@@ -681,7 +681,7 @@ static int rndis_filter_close_device(struct rndis_device
*dev)
return ret;
}
-int rndis_filte_device_add(struct hv_device *dev,
+int rndis_filter_device_add(struct hv_device *dev,
void *additional_info)
{
int ret;
--
1.6.3.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: vmbus driver
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Monday, May 23, 2011 9:52 AM To: KY Srinivasan Cc: Christoph Hellwig; johns...@us.ibm.com; gre...@suse.de; linux- ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: RE: vmbus driver 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). Will do. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
