Re: [PATCH 5 of 5] virtio: expose added descriptors immediately
On Mon, Nov 21, 2011 at 12:18:45PM +1030, Rusty Russell wrote:
> On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin"
> wrote:
> > My unlocked kick patches will trip this warning: they make
> > virtio-net do add + get without kick.
>
> Heh, it's a good sign if they do, since that means you're running really
> well :)
They don't in fact, in my testing :(. But I think they can with luck.
> > I think block with unlocked kick can trip it too:
> > add, lock is dropped and then an interrupt can get.
> >
> > We also don't need a kick each num - each 2^15 is enough.
> > Why don't we do this at start of add_buf:
> > if (vq->num_added >= 0x7fff)
> > return -ENOSPC;
>
> The warning was there in case a driver is never doing a kick, and
> getting away with it (mostly) because the device is polling. Let's not
> penalize good drivers to catch bad ones.
>
> How about we do this properly, like so:
Absolutely. But I think we also need to handle num_added
overflow of a 15 bit counter, no? Otherwise the
vring_need_event logic might give us false negatives
I'm guessing we can just assume we need a kick in that case.
> From: Rusty Russell
> Subject: virtio: add debugging if driver doesn't kick.
>
> Under the existing #ifdef DEBUG, check that they don't have more than
> 1/10 of a second between an add_buf() and a
> virtqueue_notify()/virtqueue_kick_prepare() call.
>
> We could get false positives on a really busy system, but good for
> development.
>
> Signed-off-by: Rusty Russell
> ---
> drivers/virtio/virtio_ring.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> /* virtio guest is communicating with a virtual "device" that actually runs
> on
> * a host processor. Memory barriers are used to control SMP effects. */
> @@ -102,6 +103,10 @@ struct vring_virtqueue
> #ifdef DEBUG
> /* They're supposed to lock for us. */
> unsigned int in_use;
> +
> + /* Figure out if their kicks are too delayed. */
> + bool last_add_time_valid;
> + ktime_t last_add_time;
> #endif
>
> /* Tokens for callbacks. */
> @@ -192,6 +197,19 @@ int virtqueue_add_buf(struct virtqueue *
>
> BUG_ON(data == NULL);
>
> +#ifdef DEBUG
> + {
> + ktime_t now = ktime_get();
> +
> + /* No kick or get, with .1 second between? Warn. */
> + if (vq->last_add_time_valid)
> + WARN_ON(ktime_to_ms(ktime_sub(now, vq->last_add_time))
> + > 100);
> + vq->last_add_time = now;
> + vq->last_add_time_valid = true;
> + }
> +#endif
> +
> /* If the host supports indirect descriptor tables, and we have multiple
>* buffers, then go indirect. FIXME: tune this threshold */
> if (vq->indirect && (out + in) > 1 && vq->num_free) {
> @@ -291,6 +309,14 @@ bool virtqueue_kick_prepare(struct virtq
> new = vq->vring.avail->idx;
> vq->num_added = 0;
>
> +#ifdef DEBUG
> + if (vq->last_add_time_valid) {
> + WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
> + vq->last_add_time)) > 100);
> + }
> + vq->last_add_time_valid = false;
> +#endif
> +
> if (vq->event) {
> needs_kick = vring_need_event(vring_avail_event(&vq->vring),
> new, old);
> @@ -428,6 +454,10 @@ void *virtqueue_get_buf(struct virtqueue
> virtio_mb();
> }
>
> +#ifdef DEBUG
> + vq->last_add_time_valid = false;
> +#endif
> +
> END_USE(vq);
> return ret;
> }
> @@ -611,6 +641,7 @@ struct virtqueue *vring_new_virtqueue(un
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> vq->in_use = false;
> + vq->last_add_time_valid = false;
> #endif
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 2011-11-21 at 03:32 +, Rusty Russell wrote: > No it's not; I didn't bother when I converted things across, and it > shows. 546970bc (Rusty Russell 2010-08-11 23:04:20 -0600 133) #define module_param_cb Now I understand... :-) > But we expect virtio hackers to be smarter than average :) Hope I'll stand up to the challenge ;-) > > there would be a clash. So I wanted to add a "first_id" parameter, but <...> > Well, tell them to get the cmdline order right, or allow an explicit > device id in the commandline. Yeah, I've came up with the same idea last night: [KMG]@:[:] , if specified, sets the id for the current device and a base for the next one. > Since I hope we're going to be coding together more often, I've written > this how I would have done it (based loosely on your version) to > compare. Thanks, your efforts are truly appreciated! > Main changes other than the obvious: > 1) Documentation in kernel-parameters.txt I was considering this previously but for some reason I thought kernel-parameters.txt wasn't a place for module parameters at all. Now I had another look and see I was wrong. > 2) Doesn't leak mem on error paths. Em, I don't think my code was leaking memory in any error case? (no offence meant or taken ;-) > 3) Handles error from platform_device_register_resndata(). As my code was ignoring wrong descriptions (all the continues) I ignored this result as well (the implementation complains on its own anyway), but I get your point. > 4) Uses shorter names for static functions/variables. Ah, I get the hint :-) I'm trying to keep the naming convention in static symbols as well, as it makes cscope/ctags/grep usage easier... I'll just use the abbreviated "vm_" prefixes then. > See what you think... Funnily enough when I proposed some string parser few years ago (totally different story) I was flamed for using strchr() instead of strsep() ;-) But ok, I don't mind getting back to basics. > +static int set_cmdline_device(const char *device, const struct kernel_param > *kp) [...] > + delim = strchr(device, '@'); [...] > + *delim = '\0'; Ah. I forgot that strchr() takes const char * but returns "non-const" char *... Cheating, that's what it is ;-), but will work. Probably what we really want is something like kstrtoull_delim(device, 0, &val, "@\0") I'll have a look and may try to propose something of that sort, but that's another story. Maybe I should just use simple_strtol() for the time being? I'll post v3 tonight or tomorrow. Cheers! Paweł ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 2011-11-21 at 14:44 +, Pawel Moll wrote:
> I'll post v3 tonight or tomorrow.
Ok, it won't be v3 then...
I tried again, using your suggestions - see below (just the crucial bit,
however I have the kernel-parameters.txt bits ready as well). Parsing
works like charm, however when it gets to device_register() and
platform_device_register_resndata() I hit the same problem as
previously. Both are using slab allocator...
device_register()->device_add()->device_private_init()->kzalloc()
and
platform_device_register_resndata()->platform_device_register_full()->
platform_device_alloc()->kzalloc().
Essentially it seems that the parameter callbacks are just executed
waaay to early to do more than just set few integers here and there...
Any ideas?
Cheers!
Paweł
8<---
@@ -443,6 +489,145 @@ static int __devexit virtio_mmio_remove(struct
platform_device *pdev)
+/* Devices list parameter */
+
+#if defined(CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES)
+
+static struct device vm_cmdline_parent = {
+ .init_name = "virtio-mmio-cmdline",
+};
+
+static int vm_cmdline_parent_registered;
+static int vm_cmdline_id;
+
+static int vm_cmdline_set(const char *str,
+ const struct kernel_param *kp)
+{
+ int err;
+ struct resource resources[2];
+ unsigned long long val;
+ char *delim;
+ struct platform_device *pdev;
+
+ /* Size */
+ resources[0].flags = IORESOURCE_MEM;
+ resources[0].end = memparse(str, &delim) - 1;
+ if (*delim != '@')
+ return -EINVAL;
+ str = delim + 1;
+
+ /* Base address */
+ delim = strchr(str, ':');
+ if (!delim)
+ return -EINVAL;
+ /* kstrtoull is strict, so we have to temporarily truncate */
+ *delim = '\0';
+ err = kstrtoull(str, 0, &val);
+ *delim = ':';
+ if (err)
+ return err;
+ resources[0].start = val;
+ resources[0].end += val;
+ str = delim + 1;
+
+ /* Interrupt */
+ delim = strchr(str, ':');
+ if (delim) /* Optional device id delimiter */
+ *delim = '\0';
+ err = kstrtoull(str, 0, &val);
+ if (delim)
+ *delim = ':';
+ resources[1].flags = IORESOURCE_IRQ;
+ resources[1].start = resources[1].end = val;
+ str = delim + 1;
+
+ /* Optional device id */
+ if (delim) {
+ err = kstrtoull(str, 0, &val);
+ if (err)
+ return err;
+ vm_cmdline_id = val;
+ }
+
+ if (!vm_cmdline_parent_registered) {
+ err = device_register(&vm_cmdline_parent);
+ if (err) {
+ pr_err("Failed to register parent device!\n");
+ return err;
+ }
+ vm_cmdline_parent_registered = 1;
+ }
+
+ pr_info("Registering device virtio-mmio.%d at 0x%llx-0x%llx, IRQ %d.\n",
+ vm_cmdline_id++,
+ (unsigned long long)resources[0].start,
+ (unsigned long long)resources[0].end,
+ (int)resources[1].start);
+
+ pdev = platform_device_register_resndata(&vm_cmdline_parent,
+ "virtio-mmio", vm_cmdline_id++,
+ resources, ARRAY_SIZE(resources), NULL, 0);
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
+
+ return 0;
+}
+
+static int vm_cmdline_get_device(struct device *dev, void *data)
+{
+ char *buffer = data;
+ unsigned int len = strlen(buffer);
+ struct platform_device *pdev = to_platform_device(dev);
+
+ snprintf(buffer + len, PAGE_SIZE - len, "0x%llx@0x%llx:%llu:%d\n",
+ pdev->resource[0].end - pdev->resource[0].start + 1ULL,
+ (unsigned long long)pdev->resource[0].start,
+ (unsigned long long)pdev->resource[1].start,
+ pdev->id);
+ return 0;
+}
+
+static int vm_cmdline_get(char *buffer, const struct kernel_param *kp)
+{
+ buffer[0] = '\0';
+ device_for_each_child(&vm_cmdline_parent, buffer,
+ vm_cmdline_get_device);
+ return strlen(buffer) + 1;
+}
+
+static struct kernel_param_ops vm_cmdline_param_ops = {
+ .set = vm_cmdline_set,
+ .get = vm_cmdline_get,
+};
+
+module_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
+
+static int vm_unregister_cmdline_device(struct device *dev,
+ void *data)
+{
+ platform_device_unregister(to_platform_device(dev));
+
+ return 0;
+}
+
+static void vm_unregister_cmdline_devices(void)
+{
+ if (vm_cmdline_parent_registered) {
+ device_for_each_child(&vm_cmdline_parent, NULL,
+ vm_unregister_cmdline_device);
+ device_unregister(&vm_cmdline_parent);
+ vm_cmdline
Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.
Rusty, Michael, Stephen, et al, Thanks for your comments on these patches. For what I'm trying to do, all three patches are necessary, but maybe I'm going about it the wrong way. Your input would be appreciated. I'm in no way claiming that these patches are "right", just that it's working for me, and that what's in the current pool is not. What I'm trying to do is: On X86, under KVM, start a virtio console device, with multiple ports on the device, at least one of which is also a console (as well as ttyS0). (Eventually, we want to be able to add virtio console ports on the fly, and to have multiple virtio console ports be consoles.) When all three of the patches are in place, this works great. (By great, I mean that getty's start up on all of ttyS0, hvc0 and hvc1, and console output goes to ttyS0 and to hvc0. "who" shows three users: ttyS0, hvc0, and hvc1. "cat /proc/consoles" shows both ttyS0 and hvc0. I can use all three getty's, and console output really does appear on both the consoles. Based on Rusty's comments, I tried removing each of the patches individually. Here's what happens today. I've seen other failure modes depending on what precisely I'm passing the guest. There's three patches: 1/3 "fix locking of vtermno" 2/3 "enforce one-time initialization with hvc_init "3/3 "use separate struct console * for each console" If I remove the "fix locking of vtermno", I only get one virtio console terminal. "who" shows the ttyS0 and the hvc0, and I can log into the gettys on both. I don't get the second virtio console getty. Interestingly, hvc0 shows up in /proc/consoles twice, and in fact the console output is dumped twice to hvc0 (as you'd expect from looking at printk.c, each line appears twice, followed by the next line.) If I remove the "enforce one-time initialization with hvc_init" patch, which makes sure only a single thread is doing the hvc_init, and gates anyone from continuing until it has completed, I get different failures, including hangs, and dereferences of NULL pointers. If I remove the "use separate struct console * for each console"patch, what I'm seeing now is that while all of ttyS0, hvc0, and hvc1 are present with gettys running on them, of the three, only ttyS0 is a console. I also re-tried each patch alone: For either the "fix locking of vtermno" or "use separate struct console * for each console" patches (in other words, not the "enforce one-time initialization with hvc_init" patch), I panic during boot with a null dereference. For just the "enforce one-time initialization with hvc_init" patch, I see all of hvc0, hvc1, and ttyS0 in a "who" listing, but only one getty is available with an hvc. Also, an echo to *either* "hvc0" or"hvc1" appears on the single hvc getty. Also, no virtio console appears in the /proc/consoles list. Michael, I agree with you about the comment and naming of the mutex around hvc_init. Stephen, the duplicate messages are not something I'm seeing. It's probably the case that there are two "consoles" (registered in printk) that have the same tty as their target. I've added a call to register_console in hvc_alloc, and I'm guessing that something in your system is making your tty register as a console in hvc_instantiate, and then it's re-registered in hvc_alloc, but I really am not sure. We don't have earlyprintk support, so the register_console in hvc_instantiate is never called. Miche ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] [ver3 PATCH 3/6] virtio_net: virtio_net driver changes
On Fri, 2011-11-11 at 18:34 +0530, Krishna Kumar wrote:
> Changes for multiqueue virtio_net driver.
[...]
> @@ -677,25 +730,35 @@ static struct rtnl_link_stats64 *virtnet
> {
> struct virtnet_info *vi = netdev_priv(dev);
> int cpu;
> - unsigned int start;
>
> for_each_possible_cpu(cpu) {
> - struct virtnet_stats __percpu *stats
> - = per_cpu_ptr(vi->stats, cpu);
> - u64 tpackets, tbytes, rpackets, rbytes;
> -
> - do {
> - start = u64_stats_fetch_begin(&stats->syncp);
> - tpackets = stats->tx_packets;
> - tbytes = stats->tx_bytes;
> - rpackets = stats->rx_packets;
> - rbytes = stats->rx_bytes;
> - } while (u64_stats_fetch_retry(&stats->syncp, start));
> -
> - tot->rx_packets += rpackets;
> - tot->tx_packets += tpackets;
> - tot->rx_bytes += rbytes;
> - tot->tx_bytes += tbytes;
> + int qpair;
> +
> + for (qpair = 0; qpair < vi->num_queue_pairs; qpair++) {
> + struct virtnet_send_stats __percpu *tx_stat;
> + struct virtnet_recv_stats __percpu *rx_stat;
While you're at it, you can drop the per-CPU stats and make them only
per-queue. There is unlikely to be any benefit in maintaining them
per-CPU while receive and transmit processing is serialised per-queue.
[...]
> +static int invoke_find_vqs(struct virtnet_info *vi)
> +{
> + vq_callback_t **callbacks;
> + struct virtqueue **vqs;
> + int ret = -ENOMEM;
> + int i, total_vqs;
> + char **names;
> +
> + /*
> + * We expect 1 RX virtqueue followed by 1 TX virtqueue, followed
> + * by the same 'vi->num_queue_pairs-1' more times, and optionally
> + * one control virtqueue.
> + */
> + total_vqs = vi->num_queue_pairs * 2 +
> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ);
> +
> + /* Allocate space for find_vqs parameters */
> + vqs = kmalloc(total_vqs * sizeof(*vqs), GFP_KERNEL);
> + callbacks = kmalloc(total_vqs * sizeof(*callbacks), GFP_KERNEL);
> + names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
> + if (!vqs || !callbacks || !names)
> + goto err;
> +
> + /* Allocate/initialize parameters for recv virtqueues */
> + for (i = 0; i < vi->num_queue_pairs * 2; i += 2) {
> + callbacks[i] = skb_recv_done;
> + names[i] = kasprintf(GFP_KERNEL, "input.%d", i / 2);
> + if (!names[i])
> + goto err;
> + }
> +
> + /* Allocate/initialize parameters for send virtqueues */
> + for (i = 1; i < vi->num_queue_pairs * 2; i += 2) {
> + callbacks[i] = skb_xmit_done;
> + names[i] = kasprintf(GFP_KERNEL, "output.%d", i / 2);
> + if (!names[i])
> + goto err;
> + }
[...]
The RX and TX interrupt names for a multiqueue device should follow the
formats "-rx-" and "-tx-".
Ben.
--
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: [RFC] [ver3 PATCH 3/6] virtio_net: virtio_net driver changes
On Fri, 2011-11-18 at 08:24 +0200, Sasha Levin wrote:
> On Fri, 2011-11-18 at 01:08 +, Ben Hutchings wrote:
> > On Fri, 2011-11-11 at 18:34 +0530, Krishna Kumar wrote:
> > > Changes for multiqueue virtio_net driver.
> > [...]
> > > @@ -677,25 +730,35 @@ static struct rtnl_link_stats64 *virtnet
> > > {
> > > struct virtnet_info *vi = netdev_priv(dev);
> > > int cpu;
> > > - unsigned int start;
> > >
> > > for_each_possible_cpu(cpu) {
> > > - struct virtnet_stats __percpu *stats
> > > - = per_cpu_ptr(vi->stats, cpu);
> > > - u64 tpackets, tbytes, rpackets, rbytes;
> > > -
> > > - do {
> > > - start = u64_stats_fetch_begin(&stats->syncp);
> > > - tpackets = stats->tx_packets;
> > > - tbytes = stats->tx_bytes;
> > > - rpackets = stats->rx_packets;
> > > - rbytes = stats->rx_bytes;
> > > - } while (u64_stats_fetch_retry(&stats->syncp, start));
> > > -
> > > - tot->rx_packets += rpackets;
> > > - tot->tx_packets += tpackets;
> > > - tot->rx_bytes += rbytes;
> > > - tot->tx_bytes += tbytes;
> > > + int qpair;
> > > +
> > > + for (qpair = 0; qpair < vi->num_queue_pairs; qpair++) {
> > > + struct virtnet_send_stats __percpu *tx_stat;
> > > + struct virtnet_recv_stats __percpu *rx_stat;
> >
> > While you're at it, you can drop the per-CPU stats and make them only
> > per-queue. There is unlikely to be any benefit in maintaining them
> > per-CPU while receive and transmit processing is serialised per-queue.
>
> It allows you to update stats without a lock.
But you'll already be holding a lock related to the queue.
> Whats the benefit of having them per queue?
It should save some memory (and a little time when summing stats, though
that's unlikely to matter much).
The important thing is that splitting up stats per-CPU *and* per-queue
is a waste.
Ben.
--
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: [RFC] [ver3 PATCH 3/6] virtio_net: virtio_net driver changes
On Fri, 2011-11-18 at 18:18 +0200, Sasha Levin wrote:
> On Fri, 2011-11-18 at 15:40 +, Ben Hutchings wrote:
> > On Fri, 2011-11-18 at 08:24 +0200, Sasha Levin wrote:
> > > On Fri, 2011-11-18 at 01:08 +, Ben Hutchings wrote:
> > > > On Fri, 2011-11-11 at 18:34 +0530, Krishna Kumar wrote:
> > > > > Changes for multiqueue virtio_net driver.
> > > > [...]
> > > > > @@ -677,25 +730,35 @@ static struct rtnl_link_stats64 *virtnet
> > > > > {
> > > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > > int cpu;
> > > > > - unsigned int start;
> > > > >
> > > > > for_each_possible_cpu(cpu) {
> > > > > - struct virtnet_stats __percpu *stats
> > > > > - = per_cpu_ptr(vi->stats, cpu);
> > > > > - u64 tpackets, tbytes, rpackets, rbytes;
> > > > > -
> > > > > - do {
> > > > > - start = u64_stats_fetch_begin(&stats->syncp);
> > > > > - tpackets = stats->tx_packets;
> > > > > - tbytes = stats->tx_bytes;
> > > > > - rpackets = stats->rx_packets;
> > > > > - rbytes = stats->rx_bytes;
> > > > > - } while (u64_stats_fetch_retry(&stats->syncp, start));
> > > > > -
> > > > > - tot->rx_packets += rpackets;
> > > > > - tot->tx_packets += tpackets;
> > > > > - tot->rx_bytes += rbytes;
> > > > > - tot->tx_bytes += tbytes;
> > > > > + int qpair;
> > > > > +
> > > > > + for (qpair = 0; qpair < vi->num_queue_pairs; qpair++) {
> > > > > + struct virtnet_send_stats __percpu *tx_stat;
> > > > > + struct virtnet_recv_stats __percpu *rx_stat;
> > > >
> > > > While you're at it, you can drop the per-CPU stats and make them only
> > > > per-queue. There is unlikely to be any benefit in maintaining them
> > > > per-CPU while receive and transmit processing is serialised per-queue.
> > >
> > > It allows you to update stats without a lock.
> >
> > But you'll already be holding a lock related to the queue.
>
> Right, but now you're holding a queue lock just when playing with the
> queue, we don't hold it when we process the data - which is when we
> usually need to update stats.
[...]
The *stack* is holding the appropriate lock when calling the NAPI poll
function or ndo_start_xmit function.
Ben.
--
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 v3 2/3] hvc_init(): Enforce one-time initialization.
Thanks, Rusty. I'm not using QEMU though, just KVM. I create the device, wait for the message from the guest that the device is ready, and then add ports. Miche On Sun, Nov 20, 2011 at 9:01 PM, Rusty Russell wrote: > On Thu, 17 Nov 2011 10:57:37 -0800, Miche Baker-Harvey > wrote: >> Rusty, Michael, Stephen, et al, >> >> Thanks for your comments on these patches. >> >> For what I'm trying to do, all three patches are necessary, but maybe >> I'm going about it the wrong way. Your input would be appreciated. >> I'm in no way claiming that these patches are "right", just that it's >> working for me, and that what's in the current pool is not. > > We have to *understand* the code. If we don't, we need to rewrite the > code so we *do* understand it, or make way for someone who does. > > I'm looking at the kvm man page to try to figure out how to have virtio > console, and it's deeply unclear. What kvm commandline are you using? > I'll try to debug it here, and see what I learn about hvc_console. > > Cheers, > Rusty. > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5 of 5] virtio: expose added descriptors immediately
On Mon, 21 Nov 2011 13:57:04 +0200, "Michael S. Tsirkin"
wrote:
> On Mon, Nov 21, 2011 at 12:18:45PM +1030, Rusty Russell wrote:
> > On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin"
> > wrote:
> > > My unlocked kick patches will trip this warning: they make
> > > virtio-net do add + get without kick.
> >
> > Heh, it's a good sign if they do, since that means you're running really
> > well :)
>
> They don't in fact, in my testing :(. But I think they can with luck.
>
> > > I think block with unlocked kick can trip it too:
> > > add, lock is dropped and then an interrupt can get.
> > >
> > > We also don't need a kick each num - each 2^15 is enough.
> > > Why don't we do this at start of add_buf:
> > > if (vq->num_added >= 0x7fff)
> > > return -ENOSPC;
> >
> > The warning was there in case a driver is never doing a kick, and
> > getting away with it (mostly) because the device is polling. Let's not
> > penalize good drivers to catch bad ones.
> >
> > How about we do this properly, like so:
>
> Absolutely. But I think we also need to handle num_added
> overflow of a 15 bit counter, no? Otherwise the
> vring_need_event logic might give us false negatives
> I'm guessing we can just assume we need a kick in that case.
You're right. Thankyou. My immediate reaction of "make it an unsigned
long" doesn't work.
Here's the diff to what I posted before:
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -254,9 +254,10 @@ add_head:
vq->vring.avail->idx++;
vq->num_added++;
- /* If you haven't kicked in this long, you're probably doing something
-* wrong. */
- WARN_ON(vq->num_added > vq->vring.num);
+ /* This is very unlikely, but theoretically possible. Kick
+* just in case. */
+ if (unlikely(vq->num_added == 65535))
+ virtqueue_kick(_vq);
pr_debug("Added buffer head %i to %p\n", head, vq);
END_USE(vq);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.
On Mon, 21 Nov 2011 14:16:38 -0800, Miche Baker-Harvey wrote: > Thanks, Rusty. I'm not using QEMU though, just KVM. I create the device, > wait > for the message from the guest that the device is ready, and then add ports. > > Miche OK, since Amit was the one who implemented multi-port console, I'm going to hand this to him. I'm sure he's been looking for excuses to dive back into the console code! Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 21 Nov 2011 14:44:48 +, Pawel Moll wrote: > On Mon, 2011-11-21 at 03:32 +, Rusty Russell wrote: > > 2) Doesn't leak mem on error paths. > > Em, I don't think my code was leaking memory in any error case? (no > offence meant or taken ;-) You're right, I'm wrong. In your original version, your innovative use of free() then strdup() worked as intended. > > 3) Handles error from platform_device_register_resndata(). > > As my code was ignoring wrong descriptions (all the continues) I ignored > this result as well (the implementation complains on its own anyway), > but I get your point. I was referring to this: return platform_device_register_resndata(&virtio_mmio_cmdline_parent, "virtio-mmio", virtio_mmio_cmdline_id++, resources, ARRAY_SIZE(resources), NULL, 0) != NULL; The set function returns 0 or a -ve errno, not true/false. > > 4) Uses shorter names for static functions/variables. > > Ah, I get the hint :-) I'm trying to keep the naming convention in > static symbols as well, as it makes cscope/ctags/grep usage easier... > I'll just use the abbreviated "vm_" prefixes then. > > > See what you think... > > Funnily enough when I proposed some string parser few years ago (totally > different story) I was flamed for using strchr() instead of strsep() ;-) > But ok, I don't mind getting back to basics. > > > +static int set_cmdline_device(const char *device, const struct > > kernel_param *kp) > [...] > > + delim = strchr(device, '@'); > [...] > > + *delim = '\0'; > > Ah. I forgot that strchr() takes const char * but returns "non-const" > char *... Cheating, that's what it is ;-), but will work. Probably what > we really want is something like > kstrtoull_delim(device, 0, &val, "@\0") > I'll have a look and may try to propose something of that sort, but > that's another story. Maybe I should just use simple_strtol() for the > time being? Or would it be simpler to enhance sscanf() with some weird format option for suffixing? I haven't looked for similar cases, but I'd suspect a big win in general. This would be neater than anything else we've got: if (sscanf(device, "%llu@%llu[KMG]:%u", ...) != 3 && sscanf(device, "%llu@%llu[KMG]:%u:%u", ...) != 4) return -EINVAL; Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-mmio: Devices parameter parsing
On Mon, 21 Nov 2011 17:56:50 +, Pawel Moll wrote: > On Mon, 2011-11-21 at 14:44 +, Pawel Moll wrote: > > I'll post v3 tonight or tomorrow. > > Ok, it won't be v3 then... > > I tried again, using your suggestions - see below (just the crucial bit, > however I have the kernel-parameters.txt bits ready as well). Parsing > works like charm, however when it gets to device_register() and > platform_device_register_resndata() I hit the same problem as > previously. Both are using slab allocator... > > device_register()->device_add()->device_private_init()->kzalloc() > > and > > platform_device_register_resndata()->platform_device_register_full()-> > platform_device_alloc()->kzalloc(). > > Essentially it seems that the parameter callbacks are just executed > waaay to early to do more than just set few integers here and there... > > Any ideas? Rewrite the kernel and every arch so allocators work as soon as we hit start_kernel() and we can get rid of hundreds of ugly workarounds? Perhaps not today. So, we will need a linked list of devices and resources. Yeah, that means allocating, which means YA slab_is_available()/alloc_bootmem() hack. Think of yourself as a pioneer... Thanks :) Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5 of 5] virtio: expose added descriptors immediately
On Tue, Nov 22, 2011 at 11:03:04AM +1030, Rusty Russell wrote:
> On Mon, 21 Nov 2011 13:57:04 +0200, "Michael S. Tsirkin"
> wrote:
> > On Mon, Nov 21, 2011 at 12:18:45PM +1030, Rusty Russell wrote:
> > > On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin"
> > > wrote:
> > > > My unlocked kick patches will trip this warning: they make
> > > > virtio-net do add + get without kick.
> > >
> > > Heh, it's a good sign if they do, since that means you're running really
> > > well :)
> >
> > They don't in fact, in my testing :(. But I think they can with luck.
> >
> > > > I think block with unlocked kick can trip it too:
> > > > add, lock is dropped and then an interrupt can get.
> > > >
> > > > We also don't need a kick each num - each 2^15 is enough.
> > > > Why don't we do this at start of add_buf:
> > > > if (vq->num_added >= 0x7fff)
> > > > return -ENOSPC;
> > >
> > > The warning was there in case a driver is never doing a kick, and
> > > getting away with it (mostly) because the device is polling. Let's not
> > > penalize good drivers to catch bad ones.
> > >
> > > How about we do this properly, like so:
> >
> > Absolutely. But I think we also need to handle num_added
> > overflow of a 15 bit counter, no? Otherwise the
> > vring_need_event logic might give us false negatives
> > I'm guessing we can just assume we need a kick in that case.
>
> You're right. Thankyou. My immediate reaction of "make it an unsigned
> long" doesn't work.
>
> Here's the diff to what I posted before:
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -254,9 +254,10 @@ add_head:
> vq->vring.avail->idx++;
> vq->num_added++;
>
> - /* If you haven't kicked in this long, you're probably doing something
> - * wrong. */
> - WARN_ON(vq->num_added > vq->vring.num);
> + /* This is very unlikely, but theoretically possible. Kick
> + * just in case. */
> + if (unlikely(vq->num_added == 65535))
This is 0x but why use the decimal notation?
> + virtqueue_kick(_vq);
>
> pr_debug("Added buffer head %i to %p\n", head, vq);
> END_USE(vq);
We also still need to reset vq->num_added, right?
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
