From: Amit Shah <amit.shah@redhat.com> To: Miche Baker-Harvey <miche@google.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Greg Kroah-Hartman <gregkh@suse.de>, linux-kernel@vger.kernel.org, xen-devel@lists.xensource.com, Anton Blanchard <anton@samba.org>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] VirtioConsole support. Date: Mon, 31 Oct 2011 15:25:44 +0530 [thread overview] Message-ID: <20111031095544.GB3557@amit-x200.redhat.com> (raw) In-Reply-To: <1319740793-2187-1-git-send-email-miche@google.com> On (Thu) 27 Oct 2011 [11:39:53], Miche Baker-Harvey wrote: > Multiple HVC console terminals enabled. > > Serialize device and port open and initialization. Added a mutex > which gates the handling of control messages in virtio_console.c. > This includes adding and removing ports, and making opened ports be > consoles. Extended the use of the prvdata spinlock to cover some missed > modifications to prvdata.next_vtermno. > > I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting > for the driver to be ready, and for the one-time call to hvc_init(). It > had been the case that this was sometimes being called mulitple times, and > partially setup state was being used by the second caller of hvc_alloc(). > > Make separate struct console* for each new port. There was a single static > struct console* hvc_console, to be used for early printk. We aren't doing > early printk, but more importantly, there is no code to multiplex on that > one console. Multiple virtio_console ports were "sharing" this, which was > disasterous since both the index and the flags for the console are stored > there. The console struct is remembered in the hvc_struct, and it is > deallocated when the hvc_struct is deallocated. > > ------------------ > > Konrad, thanks for trying this out on Xen. > This is working in my KVM environment, letting me start multiple > virtio_consoles with getty's on them, but I'm really not sure how > all the console pieces fit together yet. Feedback is welcome. > > Signed-off-by: Miche Baker-Harvey <miche@google.com> > --- > drivers/char/virtio_console.c | 22 +++++++++++++++++++--- > drivers/tty/hvc/hvc_console.c | 39 +++++++++++++++++++++++++++++++++------ > drivers/tty/hvc/hvc_console.h | 1 + > 3 files changed, 53 insertions(+), 9 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index fb68b12..e819d46 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -24,6 +24,7 @@ > #include <linux/fs.h> > #include <linux/init.h> > #include <linux/list.h> > +#include <linux/mutex.h> > #include <linux/poll.h> > #include <linux/sched.h> > #include <linux/slab.h> > @@ -95,6 +96,11 @@ struct console { > u32 vtermno; > }; > > +/* serialize the handling of control messages, which includes > + * the initialization of the virtio_consoles. > + */ Comments in this file are of the type /* * ... */ (others below are fine.) > +static DEFINE_MUTEX(virtio_console_mutex); > + > struct port_buffer { > char *buf; > > @@ -979,8 +985,14 @@ int init_port_console(struct port *port) > * pointers. The final argument is the output buffer size: we > * can do any size, so we put PAGE_SIZE here. > */ > - port->cons.vtermno = pdrvdata.next_vtermno; > + spin_lock_irq(&pdrvdata_lock); > + port->cons.vtermno = pdrvdata.next_vtermno++; > + spin_unlock_irq(&pdrvdata_lock); This needs to be decremented in case of an error below, or you just need the locking around the existing usage. > > + /* > + * xxx Use index 0 for now assuming there is no early HVC, since > + * we don't support it. > + */ correction: x86 doesn't have early HVC console support yet, but s390 (and likely ppc) use this today. I think it's safe to use 0 for the early console, it's not likely we have anything else using hvc consoles till this point. > port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE); > if (IS_ERR(port->cons.hvc)) { > ret = PTR_ERR(port->cons.hvc); > @@ -990,7 +1002,6 @@ int init_port_console(struct port *port) > return ret; > } > spin_lock_irq(&pdrvdata_lock); > - pdrvdata.next_vtermno++; > list_add_tail(&port->cons.list, &pdrvdata.consoles); > spin_unlock_irq(&pdrvdata_lock); > port->guest_connected = true; > @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev, > int err; > > cpkt = (struct virtio_console_control *)(buf->buf + buf->offset); > - unrelated, please drop. > port = find_port_by_id(portdev, cpkt->id); > if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) { > /* No valid header at start of buffer. Drop it. */ > @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev, > return; > } > > + /* > + * These are rare initialization-time events that should be > + * serialized. > + */ > + mutex_lock(&virtio_console_mutex); > switch (cpkt->event) { > case VIRTIO_CONSOLE_PORT_ADD: > if (port) { > @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev, > } > break; > } > + mutex_unlock(&virtio_console_mutex); Not really rare init-time events; ports can be hot-plugged. BTW what does serialising just these add events gain us? I think if this is necessary, the mutex might be necessary for other operations too. I'll leave the review of hvc_console bits to others. Overall, I think you need to split this patch up into 3-4 parts, one fixing the spinlock usage around next_vtermno above, another adding the mutex, and the others dealing with hvc_console.c. Thanks, Amit
WARNING: multiple messages have this Message-ID (diff)
From: Amit Shah <amit.shah@redhat.com> To: Miche Baker-Harvey <miche@google.com> Cc: xen-devel@lists.xensource.com, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Greg Kroah-Hartman <gregkh@suse.de>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Anton Blanchard <anton@samba.org> Subject: Re: [PATCH] VirtioConsole support. Date: Mon, 31 Oct 2011 15:25:44 +0530 [thread overview] Message-ID: <20111031095544.GB3557@amit-x200.redhat.com> (raw) In-Reply-To: <1319740793-2187-1-git-send-email-miche@google.com> On (Thu) 27 Oct 2011 [11:39:53], Miche Baker-Harvey wrote: > Multiple HVC console terminals enabled. > > Serialize device and port open and initialization. Added a mutex > which gates the handling of control messages in virtio_console.c. > This includes adding and removing ports, and making opened ports be > consoles. Extended the use of the prvdata spinlock to cover some missed > modifications to prvdata.next_vtermno. > > I also added a mutex in hvc_console::hvc_alloc() to coordinate waiting > for the driver to be ready, and for the one-time call to hvc_init(). It > had been the case that this was sometimes being called mulitple times, and > partially setup state was being used by the second caller of hvc_alloc(). > > Make separate struct console* for each new port. There was a single static > struct console* hvc_console, to be used for early printk. We aren't doing > early printk, but more importantly, there is no code to multiplex on that > one console. Multiple virtio_console ports were "sharing" this, which was > disasterous since both the index and the flags for the console are stored > there. The console struct is remembered in the hvc_struct, and it is > deallocated when the hvc_struct is deallocated. > > ------------------ > > Konrad, thanks for trying this out on Xen. > This is working in my KVM environment, letting me start multiple > virtio_consoles with getty's on them, but I'm really not sure how > all the console pieces fit together yet. Feedback is welcome. > > Signed-off-by: Miche Baker-Harvey <miche@google.com> > --- > drivers/char/virtio_console.c | 22 +++++++++++++++++++--- > drivers/tty/hvc/hvc_console.c | 39 +++++++++++++++++++++++++++++++++------ > drivers/tty/hvc/hvc_console.h | 1 + > 3 files changed, 53 insertions(+), 9 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index fb68b12..e819d46 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -24,6 +24,7 @@ > #include <linux/fs.h> > #include <linux/init.h> > #include <linux/list.h> > +#include <linux/mutex.h> > #include <linux/poll.h> > #include <linux/sched.h> > #include <linux/slab.h> > @@ -95,6 +96,11 @@ struct console { > u32 vtermno; > }; > > +/* serialize the handling of control messages, which includes > + * the initialization of the virtio_consoles. > + */ Comments in this file are of the type /* * ... */ (others below are fine.) > +static DEFINE_MUTEX(virtio_console_mutex); > + > struct port_buffer { > char *buf; > > @@ -979,8 +985,14 @@ int init_port_console(struct port *port) > * pointers. The final argument is the output buffer size: we > * can do any size, so we put PAGE_SIZE here. > */ > - port->cons.vtermno = pdrvdata.next_vtermno; > + spin_lock_irq(&pdrvdata_lock); > + port->cons.vtermno = pdrvdata.next_vtermno++; > + spin_unlock_irq(&pdrvdata_lock); This needs to be decremented in case of an error below, or you just need the locking around the existing usage. > > + /* > + * xxx Use index 0 for now assuming there is no early HVC, since > + * we don't support it. > + */ correction: x86 doesn't have early HVC console support yet, but s390 (and likely ppc) use this today. I think it's safe to use 0 for the early console, it's not likely we have anything else using hvc consoles till this point. > port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE); > if (IS_ERR(port->cons.hvc)) { > ret = PTR_ERR(port->cons.hvc); > @@ -990,7 +1002,6 @@ int init_port_console(struct port *port) > return ret; > } > spin_lock_irq(&pdrvdata_lock); > - pdrvdata.next_vtermno++; > list_add_tail(&port->cons.list, &pdrvdata.consoles); > spin_unlock_irq(&pdrvdata_lock); > port->guest_connected = true; > @@ -1317,7 +1328,6 @@ static void handle_control_message(struct ports_device *portdev, > int err; > > cpkt = (struct virtio_console_control *)(buf->buf + buf->offset); > - unrelated, please drop. > port = find_port_by_id(portdev, cpkt->id); > if (!port && cpkt->event != VIRTIO_CONSOLE_PORT_ADD) { > /* No valid header at start of buffer. Drop it. */ > @@ -1326,6 +1336,11 @@ static void handle_control_message(struct ports_device *portdev, > return; > } > > + /* > + * These are rare initialization-time events that should be > + * serialized. > + */ > + mutex_lock(&virtio_console_mutex); > switch (cpkt->event) { > case VIRTIO_CONSOLE_PORT_ADD: > if (port) { > @@ -1429,6 +1444,7 @@ static void handle_control_message(struct ports_device *portdev, > } > break; > } > + mutex_unlock(&virtio_console_mutex); Not really rare init-time events; ports can be hot-plugged. BTW what does serialising just these add events gain us? I think if this is necessary, the mutex might be necessary for other operations too. I'll leave the review of hvc_console bits to others. Overall, I think you need to split this patch up into 3-4 parts, one fixing the spinlock usage around next_vtermno above, another adding the mutex, and the others dealing with hvc_console.c. Thanks, Amit
next prev parent reply other threads:[~2011-10-31 9:56 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-10-27 5:30 Regression: patch " hvc_console: display printk messages on console." causing infinite loop with 3.2-rc0 + Xen Konrad Rzeszutek Wilk 2011-10-27 5:34 ` Konrad Rzeszutek Wilk 2011-10-27 5:48 ` Greg KH 2011-10-27 14:45 ` Miche Baker-Harvey 2011-10-27 14:50 ` Fwd: " Miche Baker-Harvey 2011-10-27 15:08 ` Konrad Rzeszutek Wilk 2011-10-27 15:31 ` Konrad Rzeszutek Wilk 2011-10-27 17:22 ` Miche Baker-Harvey 2011-10-27 17:35 ` Konrad Rzeszutek Wilk 2011-10-27 18:39 ` [PATCH] VirtioConsole support Miche Baker-Harvey 2011-10-27 21:15 ` Konrad Rzeszutek Wilk 2011-10-27 21:15 ` Konrad Rzeszutek Wilk 2011-10-31 9:55 ` Amit Shah [this message] 2011-10-31 9:55 ` Amit Shah 2011-10-27 18:39 ` Miche Baker-Harvey 2011-10-27 21:43 ` Miche Baker-Harvey 2011-10-27 21:55 ` Konrad Rzeszutek Wilk 2011-10-27 21:55 ` Konrad Rzeszutek Wilk 2011-10-28 16:27 ` Greg KH 2011-10-28 16:27 ` Greg KH 2011-10-28 2:55 ` Joe Perches 2011-10-28 2:55 ` Joe Perches 2011-10-28 18:19 ` Stephen Boyd 2011-10-28 18:19 ` Stephen Boyd 2011-11-02 22:02 ` Rusty Russell 2011-11-02 22:02 ` Rusty Russell 2011-11-03 12:38 ` Christian Borntraeger 2011-11-03 12:38 ` Christian Borntraeger 2011-11-03 14:15 ` Konrad Rzeszutek Wilk 2011-11-03 14:15 ` Konrad Rzeszutek Wilk 2011-10-27 21:43 ` Miche Baker-Harvey 2011-11-02 1:13 ` Regression: patch " hvc_console: display printk messages on console." causing infinite loop with 3.2-rc0 + Xen Stephen Rothwell 2011-11-02 1:13 ` Stephen Rothwell 2011-11-02 1:13 ` Stephen Rothwell 2011-11-03 1:30 ` Greg KH 2011-11-03 1:30 ` Greg KH 2011-11-03 1:30 ` Greg KH 2011-11-07 6:19 ` Stephen Rothwell 2011-11-07 6:19 ` Stephen Rothwell 2011-11-07 6:19 ` Stephen Rothwell 2011-11-07 20:24 ` Greg KH 2011-11-07 20:24 ` Greg KH 2011-11-07 20:24 ` Greg KH 2011-10-31 7:48 ` Rusty Russell
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20111031095544.GB3557@amit-x200.redhat.com \ --to=amit.shah@redhat.com \ --cc=anton@samba.org \ --cc=benh@kernel.crashing.org \ --cc=gregkh@suse.de \ --cc=konrad.wilk@oracle.com \ --cc=linux-kernel@vger.kernel.org \ --cc=miche@google.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xen-devel@lists.xensource.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.