Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close
On (Thu) 08 Nov 2012 [10:28:53], Rusty Russell wrote: sjur.brandel...@stericsson.com writes: From: Sjur Brændeland sjur.brandel...@stericsson.com Free pending output buffers from the virtio out-queue when host has acknowledged port_close. Also removed WARN_ON() in remove_port_data(). Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com --- Resending, this time including a proper Subject... -- Hi Amit, Note: This patch is compile tested only. I have done the removal of buffers from out-queue in handle_control_message() when host has acked the close request. This seems less racy than doing it in the release function. This confuses me... why are we doing this in case VIRTIO_CONSOLE_PORT_OPEN:? We can't pull unconsumed buffers out of the ring when the other side may still access it, and this seems to be doing that. Yes -- and it's my fault; I asked Sjur to do that in the close fops function. We should only do this in the port remove case (unplug or device remove) -- so the original patch, with just the WARN_ON removed is the right way. I'll send the revised 3/3 patch for you. Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv8 0/3]virtio_console: Add rproc_serial driver
On (Tue) 30 Oct 2012 [09:51:50], Sjur Brændeland wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com This patch-set introduces a new virtio type rproc_serial for communicating with remote processors over shared memory. The driver depends on the the remoteproc framework. As preparation for introducing rproc_serial I've done a refactoring of the transmit buffer handling. Thanks, Sjur. Please pick the virtio spec from https://github.com/rustyrussell/virtio-spec and update the spec with info for remote-proc. Thanks, Amit ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[[PATCH v9 3/3] 1/1] virtio_console: Remove buffers from out_vq at port removal
From: Sjur Brændeland sjur.brandel...@stericsson.com Remove buffers from the out-queue when a port is removed. Rproc_serial communicates with remote processors that may crash and leave buffers in the out-queue. The virtio serial ports may have buffers in the out-queue as well, e.g. for non-blocking ports and the host didn't consume them yet. [Amit: Remove WARN_ON for generic ports case.] Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com Signed-off-by: Amit Shah amit.s...@redhat.com --- drivers/char/virtio_console.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 9ebadcb..5ff3b3e 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1521,6 +1521,10 @@ static void remove_port_data(struct port *port) /* Remove buffers we queued up for the Host to send us data in. */ while ((buf = virtqueue_detach_unused_buf(port-in_vq))) free_buf(buf, true); + + /* Remove buffers we queued up for the Host to consume */ + while ((buf = virtqueue_detach_unused_buf(port-out_vq))) + free_buf(buf, true); } /* -- 1.7.7.6 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close
Note: This patch is compile tested only. I have done the removal of buffers from out-queue in handle_control_message() when host has acked the close request. This seems less racy than doing it in the release function. This confuses me... why are we doing this in case VIRTIO_CONSOLE_PORT_OPEN:? We can't pull unconsumed buffers out of the ring when the other side may still access it, and this seems to be doing that. Yes -- and it's my fault; I asked Sjur to do that in the close fops function. Thanks Amit :-), but this was really my bad. We should only do this in the port remove case (unplug or device remove) -- so the original patch, with just the WARN_ON removed is the right way. I'll send the revised 3/3 patch for you. Thank you. Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter
Resorted to poaching now have we Pawel? I hope you were joking! Yes, of course. I thought that was clearly indicated by the jovial winking smiley. :) I realise it wasn't obvious soley by this exchange, but Pawel and I are actually ol' friends. Doing your work for you isn't poaching. This isn't related to my work, as I have no professional interest in virtio. In this particular instance I was trying to help out by fixing bugs uncovered by the use of randconfig, which I'm doing in my own time, as a hobby. I guess the use my work address clouds this fact, so I apologise for that. Sorry for any confusion or offence caused though Rusty. :) Kind regards, Lee ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio: Don't access index after unregister.
Virtio wants to release used indices after the corresponding
virtio device has been unregistered. However, virtio does not
hold an extra reference, giving up its last reference with
device_unregister(), making accessing dev-index afterwards
invalid.
I actually saw problems when testing my (not-yet-merged)
virtio-ccw code:
- device_add virtio-net,id=xxx
- creates device virtion with n0
- device_del xxx
- deletes virtion, but calls ida_simple_remove with an
index of 0
- device_add virtio-net,id=xxx
- tries to add virtio0, which is still in use...
So let's save the index we want to release before calling
device_unregister().
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/virtio/virtio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 1e8659c..809b0de 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
+ int index = dev-index; /* save for after device release */
+
device_unregister(dev-dev);
- ida_simple_remove(virtio_index_ida, dev-index);
+ ida_simple_remove(virtio_index_ida, index);
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
--
1.7.12.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access index after unregister.
On Thu, Nov 8, 2012 at 11:43 AM, Cornelia Huck cornelia.h...@de.ibm.com wrote:
Virtio wants to release used indices after the corresponding
virtio device has been unregistered. However, virtio does not
hold an extra reference, giving up its last reference with
device_unregister(), making accessing dev-index afterwards
invalid.
I actually saw problems when testing my (not-yet-merged)
virtio-ccw code:
- device_add virtio-net,id=xxx
- creates device virtion with n0
- device_del xxx
- deletes virtion, but calls ida_simple_remove with an
index of 0
- device_add virtio-net,id=xxx
- tries to add virtio0, which is still in use...
So let's save the index we want to release before calling
device_unregister().
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/virtio/virtio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 1e8659c..809b0de 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
+ int index = dev-index; /* save for after device release */
+
device_unregister(dev-dev);
- ida_simple_remove(virtio_index_ida, dev-index);
+ ida_simple_remove(virtio_index_ida, index);
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
Acked-by: Sjur Brændeland sjur.brandel...@stericsson.com
Great minds think alike! I discovered issues with this implementation
a while back and Michael suggested an identical patch:
https://lkml.org/lkml/2012/9/4/173
https://lkml.org/lkml/2012/9/7/105
The issue I ran into was that when virtio devices are created by remoteproc
the device memory might be freed when calling device_unregister(), and
the value of dev-index is then undefined. So this bug bites when
unregistering a Virtio devices from remoteproc with CONFIG_DEBUG_SLAB
enabled. However this bug is not triggered by virtio_pci as it implements a
non-standard device release-function that does not free the device memory.
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Fix irq parsing in command line parameter
Lee Jones lee.jo...@linaro.org writes: Resorted to poaching now have we Pawel? I hope you were joking! Yes, of course. I thought that was clearly indicated by the jovial winking smiley. :) I realise it wasn't obvious soley by this exchange, but Pawel and I are actually ol' friends. Doing your work for you isn't poaching. This isn't related to my work, as I have no professional interest in virtio. In this particular instance I was trying to help out by fixing bugs uncovered by the use of randconfig, which I'm doing in my own time, as a hobby. I guess the use my work address clouds this fact, so I apologise for that. Sorry for any confusion or offence caused though Rusty. :) Kind regards, Lee No offence for me. I just don't want bystanders to believe that there is some etiquette other than best patch wins. Thanks for the clarification! Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 0/6] VSOCK for Linux upstreaming
Hi Gerd, Also, there was some interest from RedHat into using vSockets as a unified interface, routed over a hypervisor-specific transport (virtio or otherwise, although for now VMCI is the only one implemented). Can you outline how this can be done? From a quick look over the code it seems like vsock has a hard dependency on vmci, is that correct? That's correct, VMCI is wired into vSockets and we don't currently provide any way to insert another transport. When making vsock a generic, reusable kernel service it should be the other way around: vsock should provide the core implementation and an interface where hypervisor-specific transports (vmci, virtio, xenbus, ...) can register themself. Sorry, that was a bad explanation on my part. You're absolutely correct as to how it _should_ work. But it's up to Red Hat or others to get the ball rolling and motivate the necessary work on vSockets to make this happen. As Greg says, everyone is lazy and just wants their code accepted ;) Thanks! - Andy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3] virtio-mmio: Fix irq parsing in command line parameter
When the resource_size_t is 64-bit long, the sscanf() on
the virtio device command line paramter string may return
wrong value because its format was defined as %u. Fixed
by using an intermediate local value of a known length.
Also added cleaned up the resource creation and added extra
comments to make the parameters parsing easier to follow.
Reported-by: Lee Jones lee.jo...@linaro.org
Signed-off-by: Pawel Moll pawel.m...@arm.com
---
drivers/virtio/virtio_mmio.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..9df4578 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -521,25 +521,33 @@ static int vm_cmdline_set(const char *device,
int err;
struct resource resources[2] = {};
char *str;
- long long int base;
+ long long int base, size;
+ unsigned int irq;
int processed, consumed = 0;
struct platform_device *pdev;
- resources[0].flags = IORESOURCE_MEM;
- resources[1].flags = IORESOURCE_IRQ;
-
- resources[0].end = memparse(device, str) - 1;
+ /* Consume size part of the command line parameter */
+ size = memparse(device, str);
+ /* Get @base:irq[:id] chunks */
processed = sscanf(str, @%lli:%u%n:%d%n,
- base, resources[1].start, consumed,
+ base, irq, consumed,
vm_cmdline_id, consumed);
- if (processed 2 || processed 3 || str[consumed])
+ /*
+* sscanf() must processes at least 2 chunks; also there
+* must be no extra characters after the last chunk, so
+* str[consumed] must be '\0'
+*/
+ if (processed 2 || str[consumed])
return -EINVAL;
+ resources[0].flags = IORESOURCE_MEM;
resources[0].start = base;
- resources[0].end += base;
- resources[1].end = resources[1].start;
+ resources[0].end = base + size - 1;
+
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = irq;
if (!vm_cmdline_parent_registered) {
err = device_register(vm_cmdline_parent);
--
1.7.10.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [rfc net-next v6 3/3] virtio-net: change the number of queues through ethtool
On Tue, 2012-10-30 at 18:03 +0800, Jason Wang wrote:
This patch implement the {set|get}_channels method of ethool to allow user to
change the number of queues dymaically when the device is running. This would
let the user to tune the device for specific applications.
Signed-off-by: Jason Wang jasow...@redhat.com
---
drivers/net/virtio_net.c | 43 +++
1 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8cc43e5..66fc129 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1616,10 +1616,53 @@ static struct virtio_driver virtio_net_driver = {
#endif
};
+/* TODO: Eliminate OOO packets during switching */
+static int virtnet_set_channels(struct net_device *dev,
+ struct ethtool_channels *channels)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ u16 queue_pairs = channels-combined_count;
+
+ /* Only two modes were support currently */
+ if (queue_pairs == 0)
+ return -EINVAL;
+ if (queue_pairs != vi-total_queue_pairs - 1 queue_pairs != 1)
+ return -EINVAL;
You should also check that all of the other counts are == 0.
+ vi-num_queue_pairs = queue_pairs 1 ? queue_pairs + 1 : 1;
This doesn't make sense. You're setting vi-num_queue_pairs to be
combined_count + 1...
+ BUG_ON(virtnet_set_queues(vi));
+
+ netif_set_real_num_tx_queues(dev, vi-num_queue_pairs);
+ netif_set_real_num_rx_queues(dev, vi-num_queue_pairs);
+
+ return 0;
+}
+
+static void virtnet_get_channels(struct net_device *dev,
+ struct ethtool_channels *channels)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+
+ if (vi-total_queue_pairs != 1) {
+ channels-combined_count = vi-num_queue_pairs;
but here you report combined_count as being vi-num_queue_pairs. Do you
need to subtract 1 here?
Ben.
+ channels-max_combined = vi-total_queue_pairs - 1;
+ } else {
+ channels-combined_count = 1;
+ channels-max_combined = 1;
+ }
+
+ channels-max_other = 0;
+ channels-rx_count = 0;
+ channels-tx_count = 0;
+ channels-other_count = 0;
+}
+
static const struct ethtool_ops virtnet_ethtool_ops = {
.get_drvinfo = virtnet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_ringparam = virtnet_get_ringparam,
+ .set_channels = virtnet_set_channels,
+ .get_channels = virtnet_get_channels,
};
static int __init init(void)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access index after unregister.
Cornelia Huck cornelia.h...@de.ibm.com writes: Virtio wants to release used indices after the corresponding virtio device has been unregistered. However, virtio does not hold an extra reference, giving up its last reference with device_unregister(), making accessing dev-index afterwards invalid. I actually saw problems when testing my (not-yet-merged) virtio-ccw code: - device_add virtio-net,id=xxx - creates device virtion with n0 - device_del xxx - deletes virtion, but calls ida_simple_remove with an index of 0 - device_add virtio-net,id=xxx - tries to add virtio0, which is still in use... So let's save the index we want to release before calling device_unregister(). Great catch! I've add a CC:stable. Applied, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] virtio-mmio: Fix irq parsing in command line parameter
Pawel Moll pawel.m...@arm.com writes: When the resource_size_t is 64-bit long, the sscanf() on the virtio device command line paramter string may return wrong value because its format was defined as %u. Fixed by using an intermediate local value of a known length. Also added cleaned up the resource creation and added extra comments to make the parameters parsing easier to follow. Applied. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access index after unregister.
On Thu, Nov 08, 2012 at 11:43:47AM +0100, Cornelia Huck wrote:
Virtio wants to release used indices after the corresponding
virtio device has been unregistered. However, virtio does not
hold an extra reference, giving up its last reference with
device_unregister(), making accessing dev-index afterwards
invalid.
I actually saw problems when testing my (not-yet-merged)
virtio-ccw code:
- device_add virtio-net,id=xxx
- creates device virtion with n0
- device_del xxx
- deletes virtion, but calls ida_simple_remove with an
index of 0
- device_add virtio-net,id=xxx
- tries to add virtio0, which is still in use...
So let's save the index we want to release before calling
device_unregister().
Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
---
drivers/virtio/virtio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 1e8659c..809b0de 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
void unregister_virtio_device(struct virtio_device *dev)
{
+ int index = dev-index; /* save for after device release */
It's obvious from code that we safe for after release,
I think a better comment would explain *why* we do this.
Something like
/*
device_unregister drops reference to device so put_device could
invoke release callback. In case that callback will free the device,
make sure we don't access device after this call.
*/
int index = dev-index;
?
+
device_unregister(dev-dev);
- ida_simple_remove(virtio_index_ida, dev-index);
+ ida_simple_remove(virtio_index_ida, index);
}
EXPORT_SYMBOL_GPL(unregister_virtio_device);
--
1.7.12.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH -next] tcm_vhost: remove unused variable in vhost_scsi_allocate_cmd()
Hello Wei,
On Wed, 2012-11-07 at 20:53 +0800, Wei Yongjun wrote:
From: Wei Yongjun yongjun_...@trendmicro.com.cn
The variable se_sess is initialized but never used
otherwise, so remove the unused variable.
dpatch engine is used to auto generate this patch.
(https://github.com/weiyj/dpatch)
Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn
---
drivers/vhost/tcm_vhost.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 23c138f..551fff0 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -415,14 +415,12 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
{
struct tcm_vhost_cmd *tv_cmd;
struct tcm_vhost_nexus *tv_nexus;
- struct se_session *se_sess;
tv_nexus = tv_tpg-tpg_nexus;
if (!tv_nexus) {
pr_err(Unable to locate active struct tcm_vhost_nexus\n);
return ERR_PTR(-EIO);
}
- se_sess = tv_nexus-tvn_se_sess;
tv_cmd = kzalloc(sizeof(struct tcm_vhost_cmd), GFP_ATOMIC);
if (!tv_cmd) {
--
Looks fine to me.
Reviewed-by + Acked-by: Nicholas Bellinger n...@linux-iscsi.org
Thanks!
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
