* [PATCH 1/1] staging: hv: remove netvsc send buffer and related functions
From: Haiyang Zhang @ 2011-05-25 22:02 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
netvsc send buffer is not used, so remove it.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/hyperv_net.h | 10 ---
drivers/staging/hv/netvsc.c | 161 ---------------------------------------
2 files changed, 0 insertions(+), 171 deletions(-)
diff --git a/drivers/staging/hv/hyperv_net.h b/drivers/staging/hv/hyperv_net.h
index cf762bd..27f987b 100644
--- a/drivers/staging/hv/hyperv_net.h
+++ b/drivers/staging/hv/hyperv_net.h
@@ -355,10 +355,6 @@ struct nvsp_message {
/* #define NVSC_MIN_PROTOCOL_VERSION 1 */
/* #define NVSC_MAX_PROTOCOL_VERSION 1 */
-#define NETVSC_SEND_BUFFER_SIZE (64*1024) /* 64K */
-#define NETVSC_SEND_BUFFER_ID 0xface
-
-
#define NETVSC_RECEIVE_BUFFER_SIZE (1024*1024) /* 1MB */
#define NETVSC_RECEIVE_BUFFER_ID 0xcafe
@@ -383,12 +379,6 @@ struct netvsc_device {
struct list_head recv_pkt_list;
spinlock_t recv_pkt_list_lock;
- /* Send buffer allocated by us but manages by NetVSP */
- void *send_buf;
- u32 send_buf_size;
- u32 send_buf_gpadl_handle;
- u32 send_section_size;
-
/* Receive buffer allocated by us but manages by NetVSP */
void *recv_buf;
u32 recv_buf_size;
diff --git a/drivers/staging/hv/netvsc.c b/drivers/staging/hv/netvsc.c
index 41cbb26..7b5bf0d 100644
--- a/drivers/staging/hv/netvsc.c
+++ b/drivers/staging/hv/netvsc.c
@@ -323,162 +323,6 @@ exit:
return ret;
}
-static int netvsc_destroy_send_buf(struct netvsc_device *net_device)
-{
- struct nvsp_message *revoke_packet;
- int ret = 0;
-
- /*
- * If we got a section count, it means we received a
- * SendReceiveBufferComplete msg (ie sent
- * NvspMessage1TypeSendReceiveBuffer msg) therefore, we need
- * to send a revoke msg here
- */
- if (net_device->send_section_size) {
- /* Send the revoke send buffer */
- revoke_packet = &net_device->revoke_packet;
- memset(revoke_packet, 0, sizeof(struct nvsp_message));
-
- revoke_packet->hdr.msg_type =
- NVSP_MSG1_TYPE_REVOKE_SEND_BUF;
- revoke_packet->msg.v1_msg.
- revoke_send_buf.id = NETVSC_SEND_BUFFER_ID;
-
- ret = vmbus_sendpacket(net_device->dev->channel,
- revoke_packet,
- sizeof(struct nvsp_message),
- (unsigned long)revoke_packet,
- VM_PKT_DATA_INBAND, 0);
- /*
- * If we failed here, we might as well return and have a leak
- * rather than continue and a bugchk
- */
- if (ret != 0) {
- dev_err(&net_device->dev->device, "unable to send "
- "revoke send buffer to netvsp");
- return -1;
- }
- }
-
- /* Teardown the gpadl on the vsp end */
- if (net_device->send_buf_gpadl_handle) {
- ret = vmbus_teardown_gpadl(net_device->dev->channel,
- net_device->send_buf_gpadl_handle);
-
- /*
- * If we failed here, we might as well return and have a leak
- * rather than continue and a bugchk
- */
- if (ret != 0) {
- dev_err(&net_device->dev->device,
- "unable to teardown send buffer's gpadl");
- return -1;
- }
- net_device->send_buf_gpadl_handle = 0;
- }
-
- if (net_device->send_buf) {
- /* Free up the receive buffer */
- free_pages((unsigned long)net_device->send_buf,
- get_order(net_device->send_buf_size));
- net_device->send_buf = NULL;
- }
-
- return ret;
-}
-
-static int netvsc_init_send_buf(struct hv_device *device)
-{
- int ret = 0;
- int t;
- struct netvsc_device *net_device;
- struct nvsp_message *init_packet;
-
- net_device = get_outbound_net_device(device);
- if (!net_device) {
- dev_err(&device->device, "unable to get net device..."
- "device being destroyed?");
- return -1;
- }
- if (net_device->send_buf_size <= 0) {
- ret = -EINVAL;
- goto cleanup;
- }
-
- net_device->send_buf =
- (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
- get_order(net_device->send_buf_size));
- if (!net_device->send_buf) {
- dev_err(&device->device, "unable to allocate send "
- "buffer of size %d", net_device->send_buf_size);
- ret = -1;
- goto cleanup;
- }
-
- /*
- * Establish the gpadl handle for this buffer on this
- * channel. Note: This call uses the vmbus connection rather
- * than the channel to establish the gpadl handle.
- */
- ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
- net_device->send_buf_size,
- &net_device->send_buf_gpadl_handle);
- if (ret != 0) {
- dev_err(&device->device, "unable to establish send buffer's gpadl");
- goto cleanup;
- }
-
- /* Notify the NetVsp of the gpadl handle */
- init_packet = &net_device->channel_init_pkt;
-
- memset(init_packet, 0, sizeof(struct nvsp_message));
-
- init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
- init_packet->msg.v1_msg.send_recv_buf.
- gpadl_handle = net_device->send_buf_gpadl_handle;
- init_packet->msg.v1_msg.send_recv_buf.id =
- NETVSC_SEND_BUFFER_ID;
-
- /* Send the gpadl notification request */
- ret = vmbus_sendpacket(device->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret != 0) {
- dev_err(&device->device,
- "unable to send receive buffer's gpadl to netvsp");
- goto cleanup;
- }
-
- t = wait_for_completion_timeout(&net_device->channel_init_wait, HZ);
-
- BUG_ON(t == 0);
-
- /* Check the response */
- if (init_packet->msg.v1_msg.
- send_send_buf_complete.status != NVSP_STAT_SUCCESS) {
- dev_err(&device->device, "Unable to complete send buffer "
- "initialzation with NetVsp - status %d",
- init_packet->msg.v1_msg.
- send_send_buf_complete.status);
- ret = -1;
- goto cleanup;
- }
-
- net_device->send_section_size = init_packet->
- msg.v1_msg.send_send_buf_complete.section_size;
-
- goto exit;
-
-cleanup:
- netvsc_destroy_send_buf(net_device);
-
-exit:
- put_net_device(device);
- return ret;
-}
-
static int netvsc_connect_vsp(struct hv_device *device)
{
@@ -556,8 +400,6 @@ static int netvsc_connect_vsp(struct hv_device *device)
/* Post the big receive buffer to NetVSP */
ret = netvsc_init_recv_buf(device);
- if (ret == 0)
- ret = netvsc_init_send_buf(device);
cleanup:
put_net_device(device);
@@ -567,7 +409,6 @@ cleanup:
static void netvsc_disconnect_vsp(struct netvsc_device *net_device)
{
netvsc_destroy_recv_buf(net_device);
- netvsc_destroy_send_buf(net_device);
}
/*
@@ -1099,8 +940,6 @@ int netvsc_device_add(struct hv_device *device, void *additional_info)
net_device->recv_buf_size = NETVSC_RECEIVE_BUFFER_SIZE;
spin_lock_init(&net_device->recv_pkt_list_lock);
- net_device->send_buf_size = NETVSC_SEND_BUFFER_SIZE;
-
INIT_LIST_HEAD(&net_device->recv_pkt_list);
for (i = 0; i < NETVSC_RECEIVE_PACKETLIST_COUNT; i++) {
--
1.6.3.2
^ permalink raw reply related
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Arnd Bergmann @ 2011-05-25 20:34 UTC (permalink / raw)
To: Chris Metcalf; +Cc: linux-kernel, virtualization
In-Reply-To: <4DDD6729.7070201@tilera.com>
On Wednesday 25 May 2011 22:31:37 Chris Metcalf wrote:
> On 5/25/2011 4:20 PM, Arnd Bergmann wrote:
> > On Wednesday 25 May 2011 21:18:05 Chris Metcalf wrote:
> >> The contents of the hardwall ID file are then just a cpulist of the cpus
> >> covered by the hardwall, rather than introducing a new convention (as
> >> quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
> >> can be found by reading the "hardwall" files, and we can learn where they
> >> are bound in the hardwall by reading the "stat" file as is normal for
> >> learning process affinity.
> > Be careful with listing PID values in the hardwall files, as the PIDs
> > may not be unique or visible if you combine this with PID name spaces.
> > I guess the right solution would be to only list the tasks that are
> > present in the name space of the thread reading the file.
>
> Sorry not to be clearer -- I am no longer listing any PID values in the
> hardwall files, for that exact reason. You have to look at
> /proc/*/hardwall (or /proc/*/tasks/*/hardwall) to find the files that are
> in a particular hardwall. This pattern is not one that's normally directly
> useful, though, so I'm happy leaving it to userspace if it's desired.
Ok, thanks for the clarification.
Arnd
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Chris Metcalf @ 2011-05-25 20:31 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel, virtualization
In-Reply-To: <201105252220.23948.arnd@arndb.de>
On 5/25/2011 4:20 PM, Arnd Bergmann wrote:
> On Wednesday 25 May 2011 21:18:05 Chris Metcalf wrote:
>> The contents of the hardwall ID file are then just a cpulist of the cpus
>> covered by the hardwall, rather than introducing a new convention (as
>> quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
>> can be found by reading the "hardwall" files, and we can learn where they
>> are bound in the hardwall by reading the "stat" file as is normal for
>> learning process affinity.
> Be careful with listing PID values in the hardwall files, as the PIDs
> may not be unique or visible if you combine this with PID name spaces.
> I guess the right solution would be to only list the tasks that are
> present in the name space of the thread reading the file.
Sorry not to be clearer -- I am no longer listing any PID values in the
hardwall files, for that exact reason. You have to look at
/proc/*/hardwall (or /proc/*/tasks/*/hardwall) to find the files that are
in a particular hardwall. This pattern is not one that's normally directly
useful, though, so I'm happy leaving it to userspace if it's desired.
>>> When you do a /sys/hypervisor/ interface, put everything into a subdirectory
>>> under /sys/hypervisor with the name of your hypervisor, to avoid naming
>>> conflicts, e.g.
>>>
>>> /sys/hypervisor/tilera-hv/board/board_serial
> I just checked for other users. The only one I could find was
> drivers/xen/sys-hypervisor.c, and it also doesn't use a subdirectory to
> identify that hypervisor. It's probably more consistent if you also don't
> do it then.
>
> You can create a directory with multiple files using sysfs_create_group()
> as the xen code does, but not nested directories.
I'll look into sysfs_create_group(), and then send a revised patch with all
the /proc and /sys changes. Thanks!
--
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-25 20:20 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, Chris Metcalf
In-Reply-To: <4DDD55ED.2080607@tilera.com>
On Wednesday 25 May 2011 21:18:05 Chris Metcalf wrote:
> (Resending with no HTML for LKML.)
>
> On 5/20/2011 3:59 PM, Arnd Bergmann 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
> > 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.
>
> I went ahead and implemented this, and will send out a v2 patch shortly. I
> added the "hardwall" entry to both the tgid_base (since everything is
> reflected there) but also to the tid_base_stuff[], since it can be
> different (in principle) for different threads.
Ok, sounds good.
> I played around with using a symlink, but the bottom line seems to be that
> if I make it a symlink (via a SYM() macro in the table) it always has to
> exist -- so what does it point to when there's no hardwall activated? I
> tried making it point to /dev/null, but that just seemed silly. In the end
> I made /proc/PID/hardwall a file, either empty, or else containing the
> hardwall id.
ok. I suppose you could make a non-hardwall file that you can link to,
but an empty file also sounds ok.
> The actual hardwalls are then in /proc/tile/hardwall/NN, where NN is the
> hardwall id. I wrote a very simple hardwall id allocate/free pair; the pid
> allocator seemed too tied to task_structs. We only need at most NR_CPUS
> hardwall ids, so it's pretty simple to just use a cpumask to hold the set
> of allocated hardwall IDs.
ok.
> The contents of the hardwall ID file are then just a cpulist of the cpus
> covered by the hardwall, rather than introducing a new convention (as
> quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
> can be found by reading the "hardwall" files, and we can learn where they
> are bound in the hardwall by reading the "stat" file as is normal for
> learning process affinity.
Be careful with listing PID values in the hardwall files, as the PIDs
may not be unique or visible if you combine this with PID name spaces.
I guess the right solution would be to only list the tasks that are
present in the name space of the thread reading the file.
> > When you do a /sys/hypervisor/ interface, put everything into a subdirectory
> > under /sys/hypervisor with the name of your hypervisor, to avoid naming
> > conflicts, e.g.
> >
> > /sys/hypervisor/tilera-hv/board/board_serial
>
> I don't see an easy way to put a directory in /sys/hypervisor. It seems
> complex to create a kobject and a suitable class, etc., just for a
> subdirectory. Or is there something simple I'm missing? I'll keep looking.
>
> I also suspect just "tile" is an adequate subdirectory name here in the
> context of /sys/hypervisor, e.g. /sys/hypervisor/tile/version.
I just checked for other users. The only one I could find was
drivers/xen/sys-hypervisor.c, and it also doesn't use a subdirectory to
identify that hypervisor. It's probably more consistent if you also don't
do it then.
You can create a directory with multiple files using sysfs_create_group()
as the xen code does, but not nested directories.
Arnd
^ permalink raw reply
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Chris Metcalf @ 2011-05-25 19:18 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel, virtualization
In-Reply-To: <201105202159.50780.arnd@arndb.de>
(Resending with no HTML for LKML.)
On 5/20/2011 3:59 PM, Arnd Bergmann 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
> 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.
I went ahead and implemented this, and will send out a v2 patch shortly. I
added the "hardwall" entry to both the tgid_base (since everything is
reflected there) but also to the tid_base_stuff[], since it can be
different (in principle) for different threads.
I played around with using a symlink, but the bottom line seems to be that
if I make it a symlink (via a SYM() macro in the table) it always has to
exist -- so what does it point to when there's no hardwall activated? I
tried making it point to /dev/null, but that just seemed silly. In the end
I made /proc/PID/hardwall a file, either empty, or else containing the
hardwall id.
The actual hardwalls are then in /proc/tile/hardwall/NN, where NN is the
hardwall id. I wrote a very simple hardwall id allocate/free pair; the pid
allocator seemed too tied to task_structs. We only need at most NR_CPUS
hardwall ids, so it's pretty simple to just use a cpumask to hold the set
of allocated hardwall IDs.
The contents of the hardwall ID file are then just a cpulist of the cpus
covered by the hardwall, rather than introducing a new convention (as
quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
can be found by reading the "hardwall" files, and we can learn where they
are bound in the hardwall by reading the "stat" file as is normal for
learning process affinity.
> When you do a /sys/hypervisor/ interface, put everything into a subdirectory
> under /sys/hypervisor with the name of your hypervisor, to avoid naming
> conflicts, e.g.
>
> /sys/hypervisor/tilera-hv/board/board_serial
I don't see an easy way to put a directory in /sys/hypervisor. It seems
complex to create a kobject and a suitable class, etc., just for a
subdirectory. Or is there something simple I'm missing? I'll keep looking.
I also suspect just "tile" is an adequate subdirectory name here in the
context of /sys/hypervisor, e.g. /sys/hypervisor/tile/version.
--
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: Chris Metcalf @ 2011-05-25 19:17 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel, virtualization
In-Reply-To: <201105202159.50780.arnd@arndb.de>
[-- Attachment #1.1: Type: text/plain, Size: 2936 bytes --]
(Resending after marking LKML in my Thunderbird address book for "no HTML".)
On 5/20/2011 3:59 PM, Arnd Bergmann 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
> 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.
I went ahead and implemented this, and will send out a v2 patch shortly. I
added the "hardwall" entry to both the tgid_base (since everything is
reflected there) but also to the tid_base_stuff[], since it can be
different (in principle) for different threads.
I played around with using a symlink, but the bottom line seems to be that
if I make it a symlink (via a SYM() macro in the table) it always has to
exist -- so what does it point to when there's no hardwall activated? I
tried making it point to /dev/null, but that just seemed silly. In the end
I made /proc/PID/hardwall a file, either empty, or else containing the
hardwall id.
The actual hardwalls are then in /proc/tile/hardwall/NN, where NN is the
hardwall id. I wrote a very simple hardwall id allocate/free pair; the pid
allocator seemed too tied to task_structs. We only need at most NR_CPUS
hardwall ids, so it's pretty simple to just use a cpumask to hold the set
of allocated hardwall IDs.
The contents of the hardwall ID file are then just a cpulist of the cpus
covered by the hardwall, rather than introducing a new convention (as
quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
can be found by reading the "hardwall" files, and we can learn where they
are bound in the hardwall by reading the "stat" file as is normal for
learning process affinity.
> When you do a /sys/hypervisor/ interface, put everything into a subdirectory
> under /sys/hypervisor with the name of your hypervisor, to avoid naming
> conflicts, e.g.
>
> /sys/hypervisor/tilera-hv/board/board_serial
I don't see an easy way to put a directory in /sys/hypervisor. It seems
complex to create a kobject and a suitable class, etc., just for a
subdirectory. Or is there something simple I'm missing? I'll keep looking.
I also suspect just "tile" is an adequate subdirectory name here in the
context of /sys/hypervisor, e.g. /sys/hypervisor/tile/version.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
[-- Attachment #1.2: Type: text/html, Size: 3815 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
* Re: [PATCH] arch/tile: add /proc/tile, /proc/sys/tile, and a sysfs cpu attribute
From: Chris Metcalf @ 2011-05-25 19:09 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel, virtualization
In-Reply-To: <201105202159.50780.arnd@arndb.de>
[-- Attachment #1.1: Type: text/plain, Size: 2858 bytes --]
On 5/20/2011 3:59 PM, Arnd Bergmann 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
> 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.
I went ahead and implemented this, and will send out a v2 patch shortly. I
added the "hardwall" entry to both the tgid_base (since everything is
reflected there) but also to the tid_base_stuff[], since it can be
different (in principle) for different threads.
I played around with using a symlink, but the bottom line seems to be that
if I make it a symlink (via a SYM() macro in the table) it always has to
exist -- so what does it point to when there's no hardwall activated? I
tried making it point to /dev/null, but that just seemed silly. In the end
I made /proc/PID/hardwall a file, either empty, or else containing the
hardwall id.
The actual hardwalls are then in /proc/tile/hardwall/NN, where NN is the
hardwall id. I wrote a very simple hardwall id allocate/free pair; the pid
allocator seemed too tied to task_structs. We only need at most NR_CPUS
hardwall ids, so it's pretty simple to just use a cpumask to hold the set
of allocated hardwall IDs.
The contents of the hardwall ID file are then just a cpulist of the cpus
covered by the hardwall, rather than introducing a new convention (as
quoted above, e.g. "2x2 1,1"). Individual tasks that are in the hardwall
can be found by reading the "hardwall" files, and we can learn where they
are bound in the hardwall by reading the "stat" file as is normal for
learning process affinity.
> When you do a /sys/hypervisor/ interface, put everything into a subdirectory
> under /sys/hypervisor with the name of your hypervisor, to avoid naming
> conflicts, e.g.
>
> /sys/hypervisor/tilera-hv/board/board_serial
I don't see an easy way to put a directory in /sys/hypervisor. It seems
complex to create a kobject and a suitable class, etc., just for a
subdirectory. Or is there something simple I'm missing? I'll keep looking.
I also suspect just "tile" is an adequate subdirectory name here in the
context of /sys/hypervisor, e.g. /sys/hypervisor/tile/version.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
[-- Attachment #1.2: Type: text/html, Size: 3718 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
* Re: 2.6.40 event idx patches
From: Michael S. Tsirkin @ 2011-05-25 6:10 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization
In-Reply-To: <87y61vilig.fsf@rustcorp.com.au>
On Wed, May 25, 2011 at 11:34:07AM +0930, Rusty Russell wrote:
> On Tue, 24 May 2011 12:23:45 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Just checking: were you going to send the following to Linus for 2.6.40?
> > virtio:event_index_interface.patch
> > virtio:ring_inline_function_to_check_for_events.patch
> > virtio:ring_support_event_idx_feature.patch
> > misc:vhost_support_event_index.patch
> > virtio:test_support_event_index.patch
> > virtio:add_api_for_delayed_callbacks.patch
> > virtio:net_delay_tx_callbacks.patch
> >
> > I certainly hope so as these modify both host and guest which makes
> > testing them out of tree hard for people.
> > Once stuff lands on Linus' tree we can patch
> > qemu and get more people to try out the patches.
> >
> > Also, Linus said it's going to be a short window so ...
>
> Yes. They've been in linux-next since the weekend (ie. today will be the
> third linux-next build), and my plan was to push them Friday.
>
> Nothing goes into linux-next which is not for Linus until after Linus
> closes the merge window.
I see, there are for-linus tags in the series file, this is what
got me worried.
> I also want to find a few cycles to adapt lguest; it's *always* good to
> have a second implementation.
>
> Cheers,
> Rusty.
Absolutely.
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-25 6:07 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: <8739k3k1fb.fsf@rustcorp.com.au>
On Wed, May 25, 2011 at 11:05:04AM +0930, Rusty Russell wrote:
> On Mon, 23 May 2011 14:19:00 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 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 :)'
>
> Let's just make it "bool free_old_xmit_skbs(unsigned int max)". max ==
> 2 for the normal xmit path, so we're low latency but we keep ahead on
> average. max == -1 for the "we're out of capacity, we may have to stop
> the queue".
>
> That keeps it simple and probably the right thing...
>
> Thanks,
> Rusty.
Hmm I'm not sure I got it, need to think about this.
I'd like to go back and document how my design was supposed to work.
This really should have been in commit log or even a comment.
I thought we need a min, not a max.
We start with this:
while ((c = (virtqueue_get_capacity(vq) < 2 + MAX_SKB_FRAGS) &&
(skb = get_buf)))
kfree_skb(skb);
return !c;
This is clean and simple, right? And it's exactly asking for what we need.
But this way we always keep a lot of memory in skbs even when rate of
communication is low.
So we add the min parameter:
int n = 0;
while ((((c = (virtqueue_get_capacity(vq) < 2 + MAX_SKB_FRAGS)) ||
n++ < min) && (skb = get_buf)))
kfree_skb(skb);
return !c;
on the normal path min == 2 so we're low latency but we keep ahead on
average. min == 0 for the "we're out of capacity, we may have to stop
the queue".
Does the above make sense at all?
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-25 5:50 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: <8762ozk1qd.fsf@rustcorp.com.au>
On Wed, May 25, 2011 at 10:58:26AM +0930, Rusty Russell wrote:
> On Mon, 23 May 2011 14:19:00 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, May 23, 2011 at 11:37:15AM +0930, Rusty Russell wrote:
> > > 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?
>
> Yes, but also by renaming virtqueue_get_capacity(). Takes it from a 3
> to a 6 on the API hard-to-misuse scale.
>
> How about, virtqueue_min_capacity()? Makes the reader realize something
> weird is going on.
Absolutely. Great idea.
> > 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 :)'
>
> Agreed.
>
> > > > 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?
>
> No, I was confused... More coffee!
>
> Thanks,
> Rusty.
^ permalink raw reply
* Re: 2.6.40 event idx patches
From: Rusty Russell @ 2011-05-25 2:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <20110524092345.GA17087@redhat.com>
On Tue, 24 May 2011 12:23:45 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Just checking: were you going to send the following to Linus for 2.6.40?
> virtio:event_index_interface.patch
> virtio:ring_inline_function_to_check_for_events.patch
> virtio:ring_support_event_idx_feature.patch
> misc:vhost_support_event_index.patch
> virtio:test_support_event_index.patch
> virtio:add_api_for_delayed_callbacks.patch
> virtio:net_delay_tx_callbacks.patch
>
> I certainly hope so as these modify both host and guest which makes
> testing them out of tree hard for people.
> Once stuff lands on Linus' tree we can patch
> qemu and get more people to try out the patches.
>
> Also, Linus said it's going to be a short window so ...
Yes. They've been in linux-next since the weekend (ie. today will be the
third linux-next build), and my plan was to push them Friday.
Nothing goes into linux-next which is not for Linus until after Linus
closes the merge window.
I also want to find a few cycles to adapt lguest; it's *always* good to
have a second implementation.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Rusty Russell @ 2011-05-25 1:35 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: <20110523111900.GB27212@redhat.com>
On Mon, 23 May 2011 14:19:00 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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 :)'
Let's just make it "bool free_old_xmit_skbs(unsigned int max)". max ==
2 for the normal xmit path, so we're low latency but we keep ahead on
average. max == -1 for the "we're out of capacity, we may have to stop
the queue".
That keeps it simple and probably the right thing...
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Rusty Russell @ 2011-05-25 1:28 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: <20110523111900.GB27212@redhat.com>
On Mon, 23 May 2011 14:19:00 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, May 23, 2011 at 11:37:15AM +0930, Rusty Russell wrote:
> > 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?
Yes, but also by renaming virtqueue_get_capacity(). Takes it from a 3
to a 6 on the API hard-to-misuse scale.
How about, virtqueue_min_capacity()? Makes the reader realize something
weird is going on.
> 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 :)'
Agreed.
> > > 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?
No, I was confused... More coffee!
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-24 15:38 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, Chris Metcalf
In-Reply-To: <201105191722.17685.arnd@arndb.de>
On Thursday 19 May 2011, Arnd Bergmann wrote:
> >
> > # cat /proc/tile/board
> > board_part: 402-00002-05
> > board_serial: NBS-5002-00012
> > chip_serial: P62338.01.110
> > chip_revision: A0
> > board_revision: 2.2
> > board_description: Tilera TILExpressPro-64, TILEPro64 processor (866 MHz-capable), 1 10GbE, 6 1GbE
> > # cat /proc/tile/switch
> > control: mdio gbe/0
>
> I think it's ok to have it below /sys/hypervisor, because the information
> is provided through a hypervisor ABI, even though it describes something
> else. This is more like /sys/firmware, but the boundaries between that
> and /sys/hypervisor are not clearly defined when running virtualized anyway.
A minor point that I meant to bring up but had not gotten to:
When you do a /sys/hypervisor/ interface, put everything into a subdirectory
under /sys/hypervisor with the name of your hypervisor, to avoid naming
conflicts, e.g.
/sys/hypervisor/tilera-hv/board/board_serial
Arnd
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-24 13:52 UTC (permalink / raw)
To: Krishna Kumar2
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <OF2D4A7890.FFA91132-ON6525789A.0043E0AC-6525789A.00464690@in.ibm.com>
On Tue, May 24, 2011 at 06:20:35PM +0530, Krishna Kumar2 wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 05/24/2011 04:59:39 PM:
>
> > > > > Maybe Rusty means it is a simpler model to free the amount
> > > > > of space that this xmit needs. We will still fail anyway
> > > > > at some time but it is unlikely, since earlier iteration
> > > > > freed up atleast the space that it was going to use.
> > > >
> > > > Not sure I nderstand. We can't know space is freed in the previous
> > > > iteration as buffers might not have been used by then.
> > >
> > > Yes, the first few iterations may not have freed up space, but
> > > later ones should. The amount of free space should increase
> > > from then on, especially since we try to free double of what
> > > we consume.
> >
> > Hmm. This is only an upper limit on the # of entries in the queue.
> > Assume that vq size is 4 and we transmit 4 enties without
> > getting anything in the used ring. The next transmit will fail.
> >
> > So I don't really see why it's unlikely that we reach the packet
> > drop code with your patch.
>
> I was assuming 256 entries :) I will try to get some
> numbers to see how often it is true tomorrow.
That would depend on how fast the hypervisor is.
Try doing something to make hypervisor slower than the guest. I don't
think we need measurements to realize that with the host being slower
than guest that would happen a lot, though.
> > > I am not sure of why it was changed, since returning TX_BUSY
> > > seems more efficient IMHO.
> > > qdisc_restart() handles requeue'd
> > > packets much better than a stopped queue, as a significant
> > > part of this code is skipped if gso_skb is present
> >
> > I think this is the argument:
> > http://www.mail-archive.com/virtualization@lists.linux-
> > foundation.org/msg06364.html
>
> Thanks for digging up that thread! Yes, that one skb would get
> sent first ahead of possibly higher priority skbs. However,
> from a performance point, TX_BUSY code skips a lot of checks
> and code for all subsequent packets till the device is
> restarted. I can test performance with both cases and report
> what I find (the requeue code has become very simple and clean
> from "horribly complex", thanks to Herbert and Dave).
Cc Herbert, and try to convince him :)
> > > (qdisc
> > > will eventually start dropping packets when tx_queue_len is
> >
> > tx_queue_len is a pretty large buffer so maybe no.
>
> I remember seeing tons of drops (pfifo_fast_enqueue) when
> xmit returns TX_BUSY.
>
> > I think the packet drops from the scheduler queue can also be
> > done intelligently (e.g. with CHOKe) which should
> > work better than dropping a random packet?
>
> I am not sure of that - choke_enqueue checks against a random
> skb to drop current skb, and also during congestion. But for
> my "sample driver xmit", returning TX_BUSY could still allow
> to be used with CHOKe.
>
> thanks,
>
> - KK
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Krishna Kumar2 @ 2011-05-24 12:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <20110524112901.GB17087@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 05/24/2011 04:59:39 PM:
> > > > Maybe Rusty means it is a simpler model to free the amount
> > > > of space that this xmit needs. We will still fail anyway
> > > > at some time but it is unlikely, since earlier iteration
> > > > freed up atleast the space that it was going to use.
> > >
> > > Not sure I nderstand. We can't know space is freed in the previous
> > > iteration as buffers might not have been used by then.
> >
> > Yes, the first few iterations may not have freed up space, but
> > later ones should. The amount of free space should increase
> > from then on, especially since we try to free double of what
> > we consume.
>
> Hmm. This is only an upper limit on the # of entries in the queue.
> Assume that vq size is 4 and we transmit 4 enties without
> getting anything in the used ring. The next transmit will fail.
>
> So I don't really see why it's unlikely that we reach the packet
> drop code with your patch.
I was assuming 256 entries :) I will try to get some
numbers to see how often it is true tomorrow.
> > I am not sure of why it was changed, since returning TX_BUSY
> > seems more efficient IMHO.
> > qdisc_restart() handles requeue'd
> > packets much better than a stopped queue, as a significant
> > part of this code is skipped if gso_skb is present
>
> I think this is the argument:
> http://www.mail-archive.com/virtualization@lists.linux-
> foundation.org/msg06364.html
Thanks for digging up that thread! Yes, that one skb would get
sent first ahead of possibly higher priority skbs. However,
from a performance point, TX_BUSY code skips a lot of checks
and code for all subsequent packets till the device is
restarted. I can test performance with both cases and report
what I find (the requeue code has become very simple and clean
from "horribly complex", thanks to Herbert and Dave).
> > (qdisc
> > will eventually start dropping packets when tx_queue_len is
>
> tx_queue_len is a pretty large buffer so maybe no.
I remember seeing tons of drops (pfifo_fast_enqueue) when
xmit returns TX_BUSY.
> I think the packet drops from the scheduler queue can also be
> done intelligently (e.g. with CHOKe) which should
> work better than dropping a random packet?
I am not sure of that - choke_enqueue checks against a random
skb to drop current skb, and also during congestion. But for
my "sample driver xmit", returning TX_BUSY could still allow
to be used with CHOKe.
thanks,
- KK
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-24 11:29 UTC (permalink / raw)
To: Krishna Kumar2
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <OF69E520FD.340352AC-ON6525789A.003308A2-6525789A.0033F2DF@in.ibm.com>
On Tue, May 24, 2011 at 02:57:43PM +0530, Krishna Kumar2 wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 05/24/2011 02:42:55 PM:
>
> > > > > 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?
> > >
> > > Maybe Rusty means it is a simpler model to free the amount
> > > of space that this xmit needs. We will still fail anyway
> > > at some time but it is unlikely, since earlier iteration
> > > freed up atleast the space that it was going to use.
> >
> > Not sure I nderstand. We can't know space is freed in the previous
> > iteration as buffers might not have been used by then.
>
> Yes, the first few iterations may not have freed up space, but
> later ones should. The amount of free space should increase
> from then on, especially since we try to free double of what
> we consume.
Hmm. This is only an upper limit on the # of entries in the queue.
Assume that vq size is 4 and we transmit 4 enties without
getting anything in the used ring. The next transmit will fail.
So I don't really see why it's unlikely that we reach the packet
drop code with your patch.
> > > The
> > > code could become much simpler:
> > >
> > > start_xmit()
> > > {
> > > {
> > > num_sgs = get num_sgs for this skb;
> > >
> > > /* Free enough pending old buffers to enable queueing this one
> */
> > > free_old_xmit_skbs(vi, num_sgs * 2); /* ?? */
> > >
> > > if (virtqueue_get_capacity() < num_sgs) {
> > > netif_stop_queue(dev);
> > > if (virtqueue_enable_cb_delayed(vi->svq) ||
> > > free_old_xmit_skbs(vi, num_sgs)) {
> > > /* Nothing freed up, or not enough freed up */
> > > kfree_skb(skb);
> > > return NETDEV_TX_OK;
> >
> > This packet drop is what we wanted to avoid.
>
> Please see below on returning NETDEV_TX_BUSY.
>
> >
> > > }
> > > netif_start_queue(dev);
> > > virtqueue_disable_cb(vi->svq);
> > > }
> > >
> > > /* xmit_skb cannot fail now, also pass 'num_sgs' */
> > > xmit_skb(vi, skb, num_sgs);
> > > virtqueue_kick(vi->svq);
> > >
> > > skb_orphan(skb);
> > > nf_reset(skb);
> > >
> > > return NETDEV_TX_OK;
> > > }
> > >
> > > We could even return TX_BUSY since that makes the dequeue
> > > code more efficient. See dev_dequeue_skb() - you can skip a
> > > lot of code (and avoid taking locks) to check if the queue
> > > is already stopped but that code runs only if you return
> > > TX_BUSY in the earlier iteration.
> > >
> > > BTW, shouldn't the check in start_xmit be:
> > > if (likely(!free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
> > > ...
> > > }
> > >
> > > Thanks,
> > >
> > > - KK
> >
> > I thought we used to do basically this but other devices moved to a
> > model where they stop *before* queueing fails, so we did too.
>
> I am not sure of why it was changed, since returning TX_BUSY
> seems more efficient IMHO.
> qdisc_restart() handles requeue'd
> packets much better than a stopped queue, as a significant
> part of this code is skipped if gso_skb is present
I think this is the argument:
http://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg06364.html
> (qdisc
> will eventually start dropping packets when tx_queue_len is
> exceeded anyway).
>
> Thanks,
>
> - KK
tx_queue_len is a pretty large buffer so maybe no.
I think the packet drops from the scheduler queue can also be
done intelligently (e.g. with CHOKe) which should
work better than dropping a random packet?
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Krishna Kumar2 @ 2011-05-24 9:27 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <20110524091255.GB16886@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 05/24/2011 02:42:55 PM:
> > > > 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?
> >
> > Maybe Rusty means it is a simpler model to free the amount
> > of space that this xmit needs. We will still fail anyway
> > at some time but it is unlikely, since earlier iteration
> > freed up atleast the space that it was going to use.
>
> Not sure I nderstand. We can't know space is freed in the previous
> iteration as buffers might not have been used by then.
Yes, the first few iterations may not have freed up space, but
later ones should. The amount of free space should increase
from then on, especially since we try to free double of what
we consume.
> > The
> > code could become much simpler:
> >
> > start_xmit()
> > {
> > {
> > num_sgs = get num_sgs for this skb;
> >
> > /* Free enough pending old buffers to enable queueing this one
*/
> > free_old_xmit_skbs(vi, num_sgs * 2); /* ?? */
> >
> > if (virtqueue_get_capacity() < num_sgs) {
> > netif_stop_queue(dev);
> > if (virtqueue_enable_cb_delayed(vi->svq) ||
> > free_old_xmit_skbs(vi, num_sgs)) {
> > /* Nothing freed up, or not enough freed up */
> > kfree_skb(skb);
> > return NETDEV_TX_OK;
>
> This packet drop is what we wanted to avoid.
Please see below on returning NETDEV_TX_BUSY.
>
> > }
> > netif_start_queue(dev);
> > virtqueue_disable_cb(vi->svq);
> > }
> >
> > /* xmit_skb cannot fail now, also pass 'num_sgs' */
> > xmit_skb(vi, skb, num_sgs);
> > virtqueue_kick(vi->svq);
> >
> > skb_orphan(skb);
> > nf_reset(skb);
> >
> > return NETDEV_TX_OK;
> > }
> >
> > We could even return TX_BUSY since that makes the dequeue
> > code more efficient. See dev_dequeue_skb() - you can skip a
> > lot of code (and avoid taking locks) to check if the queue
> > is already stopped but that code runs only if you return
> > TX_BUSY in the earlier iteration.
> >
> > BTW, shouldn't the check in start_xmit be:
> > if (likely(!free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
> > ...
> > }
> >
> > Thanks,
> >
> > - KK
>
> I thought we used to do basically this but other devices moved to a
> model where they stop *before* queueing fails, so we did too.
I am not sure of why it was changed, since returning TX_BUSY
seems more efficient IMHO. qdisc_restart() handles requeue'd
packets much better than a stopped queue, as a significant
part of this code is skipped if gso_skb is present (qdisc
will eventually start dropping packets when tx_queue_len is
exceeded anyway).
Thanks,
- KK
^ permalink raw reply
* 2.6.40 event idx patches
From: Michael S. Tsirkin @ 2011-05-24 9:23 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization
Just checking: were you going to send the following to Linus for 2.6.40?
virtio:event_index_interface.patch
virtio:ring_inline_function_to_check_for_events.patch
virtio:ring_support_event_idx_feature.patch
misc:vhost_support_event_index.patch
virtio:test_support_event_index.patch
virtio:add_api_for_delayed_callbacks.patch
virtio:net_delay_tx_callbacks.patch
I certainly hope so as these modify both host and guest which makes
testing them out of tree hard for people.
Once stuff lands on Linus' tree we can patch
qemu and get more people to try out the patches.
Also, Linus said it's going to be a short window so ...
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Michael S. Tsirkin @ 2011-05-24 9:12 UTC (permalink / raw)
To: Krishna Kumar2
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <OFEED174CD.D3C9A726-ON6525789A.002661D5-6525789A.002B26ED@in.ibm.com>
On Tue, May 24, 2011 at 01:24:15PM +0530, Krishna Kumar2 wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote on 05/23/2011 04:49:00 PM:
>
> > > 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?
>
> Maybe Rusty means it is a simpler model to free the amount
> of space that this xmit needs. We will still fail anyway
> at some time but it is unlikely, since earlier iteration
> freed up atleast the space that it was going to use.
Not sure I nderstand. We can't know space is freed in the previous
iteration as buffers might not have been used by then.
> The
> code could become much simpler:
>
> start_xmit()
> {
> {
> num_sgs = get num_sgs for this skb;
>
> /* Free enough pending old buffers to enable queueing this one */
> free_old_xmit_skbs(vi, num_sgs * 2); /* ?? */
>
> if (virtqueue_get_capacity() < num_sgs) {
> netif_stop_queue(dev);
> if (virtqueue_enable_cb_delayed(vi->svq) ||
> free_old_xmit_skbs(vi, num_sgs)) {
> /* Nothing freed up, or not enough freed up */
> kfree_skb(skb);
> return NETDEV_TX_OK;
This packet drop is what we wanted to avoid.
> }
> netif_start_queue(dev);
> virtqueue_disable_cb(vi->svq);
> }
>
> /* xmit_skb cannot fail now, also pass 'num_sgs' */
> xmit_skb(vi, skb, num_sgs);
> virtqueue_kick(vi->svq);
>
> skb_orphan(skb);
> nf_reset(skb);
>
> return NETDEV_TX_OK;
> }
>
> We could even return TX_BUSY since that makes the dequeue
> code more efficient. See dev_dequeue_skb() - you can skip a
> lot of code (and avoid taking locks) to check if the queue
> is already stopped but that code runs only if you return
> TX_BUSY in the earlier iteration.
>
> BTW, shouldn't the check in start_xmit be:
> if (likely(!free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
> ...
> }
>
> Thanks,
>
> - KK
I thought we used to do basically this but other devices moved to a
model where they stop *before* queueing fails, so we did too.
--
MST
^ permalink raw reply
* Re: [PATCHv2 10/14] virtio_net: limit xmit polling
From: Krishna Kumar2 @ 2011-05-24 7:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: habanero, lguest, Shirley Ma, kvm, Carsten Otte, linux-s390,
Heiko Carstens, linux-kernel, virtualization, steved,
Christian Borntraeger, Tom Lendacky, netdev, Martin Schwidefsky,
linux390
In-Reply-To: <20110523111900.GB27212@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote on 05/23/2011 04:49:00 PM:
> > 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?
Maybe Rusty means it is a simpler model to free the amount
of space that this xmit needs. We will still fail anyway
at some time but it is unlikely, since earlier iteration
freed up atleast the space that it was going to use. The
code could become much simpler:
start_xmit()
{
{
num_sgs = get num_sgs for this skb;
/* Free enough pending old buffers to enable queueing this one */
free_old_xmit_skbs(vi, num_sgs * 2); /* ?? */
if (virtqueue_get_capacity() < num_sgs) {
netif_stop_queue(dev);
if (virtqueue_enable_cb_delayed(vi->svq) ||
free_old_xmit_skbs(vi, num_sgs)) {
/* Nothing freed up, or not enough freed up */
kfree_skb(skb);
return NETDEV_TX_OK;
}
netif_start_queue(dev);
virtqueue_disable_cb(vi->svq);
}
/* xmit_skb cannot fail now, also pass 'num_sgs' */
xmit_skb(vi, skb, num_sgs);
virtqueue_kick(vi->svq);
skb_orphan(skb);
nf_reset(skb);
return NETDEV_TX_OK;
}
We could even return TX_BUSY since that makes the dequeue
code more efficient. See dev_dequeue_skb() - you can skip a
lot of code (and avoid taking locks) to check if the queue
is already stopped but that code runs only if you return
TX_BUSY in the earlier iteration.
BTW, shouldn't the check in start_xmit be:
if (likely(!free_old_xmit_skbs(vi, 2+MAX_SKB_FRAGS))) {
...
}
Thanks,
- KK
^ permalink raw reply
* RE: vmbus driver
From: KY Srinivasan @ 2011-05-23 16:16 UTC (permalink / raw)
To: Thomas Gleixner
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: <alpine.LFD.2.02.1105231550360.3078@ionos>
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Monday, May 23, 2011 9:52 AM
> 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
> 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
^ permalink raw reply
* [PATCH 6/6] staging: hv: removed commented out code from rndis_filter_receive()
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/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
^ permalink raw reply related
* [PATCH 5/6] staging: hv: fix typo in name rndis_filte_device_add()
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>
rename rndis_filte_device_add to rndis_filter_device_add
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 | 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
^ permalink raw reply related
* [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
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