[PATCH 1/3] serial: 8250: Remove trailing space in 8250 driver

[PATCH 1/3] serial: 8250: Remove trailing space in 8250 driver

[Simon Glass]

Trivial change to remove a trailing space.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/tty/serial/8250.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 9f50c4e..54f8920 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -865,7 +865,7 @@ static int broken_efr(struct uart_8250_port *up)
 	/*
 	 * Exar ST16C2550 "A2" devices incorrectly detect as
 	 * having an EFR, and report an ID of 0x0201.  See
-	 * http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-11/4812.html 
+	 * http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-11/4812.html
 	 */
 	if (autoconfig_read_divisor_id(up) == 0x0201 && size_fifo(up) == 16)
 		return 1;
-- 
1.7.7.3

[PATCH 2/3] serial: Make wakeup_capable a flag to reduce boot time

[Simon Glass]

The synchronize_rcu() call resulting from making every serial driver
wake-up capable (commit b3b708fa) slows boot down by 600ms on my Tegra2x
system (with CONFIG_PREEMPT disabled).

Waking up the machine from a serial port seems to be an unlikely
requirement, so make serial_core default to not using the behaviour.

Before:
[    0.338809] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.951861] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
[    1.666042] console [ttyS0] enabled

After:
[    0.338452] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.339016] serial8250.0: ttyS0 at MMIO 0x70006040 (irq = 69) is a Tegra
[    1.041860] console [ttyS0] enabled

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/tty/serial/serial_core.c |    6 ++++--
 include/linux/serial_core.h      |    3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c7bf31a..0129551 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2348,8 +2348,10 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 */
 	tty_dev = tty_register_device(drv->tty_driver, uport->line, uport->dev);
 	if (likely(!IS_ERR(tty_dev))) {
-		device_init_wakeup(tty_dev, 1);
-		device_set_wakeup_enable(tty_dev, 0);
+		if (uport->wakeup_capable) {
+			device_init_wakeup(tty_dev, 1);
+			device_set_wakeup_enable(tty_dev, 0);
+		}
 	} else
 		printk(KERN_ERR "Cannot register tty device on line %d\n",
 		       uport->line);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c91ace7..9a87975 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -377,7 +377,8 @@ struct uart_port {
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		suspended;
 	unsigned char		irq_wake;
-	unsigned char		unused[2];
+	unsigned char		wakeup_capable;	/* 1 if wake-up capable */
+	unsigned char		unused;
 	void			*private_data;		/* generic platform data pointer */
 };
 
-- 
1.7.7.3

Re: [PATCH 2/3] serial: Make wakeup_capable a flag to reduce boot time

[Alan Cox]

On Tue, 17 Jan 2012 10:56:02 -0800
Simon Glass <sjg@chromium.org> wrote:

> The synchronize_rcu() call resulting from making every serial driver
> wake-up capable (commit b3b708fa) slows boot down by 600ms on my Tegra2x
> system (with CONFIG_PREEMPT disabled).
> 
> Waking up the machine from a serial port seems to be an unlikely
> requirement, so make serial_core default to not using the behaviour.

Its quite normal on x86, and very very common on embedded. Sorry someone
needs to fix the root cause not hack the driver.

The other question would be whether in PC space you can deduce the wake
up situation from ACPI data ? One for Len ?

Alan

[PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Since serial_core now does not make serial ports wake-up capable by
default, add a parameter to support this feature in the 8250 UART.
This is the only UART where I think this feature is useful.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/tty/serial/8250.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 54f8920..78feee4 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -65,6 +65,7 @@ static int serial_index(struct uart_port *port)
 }
 
 static unsigned int skip_txen_test; /* force skip of txen test at init time */
+static unsigned int wakeup_capable; /* device can wake up system */
 
 /*
  * Debugging.
@@ -2786,6 +2787,7 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 
 		if (up->port.flags & UPF_FIXED_TYPE)
 			serial8250_init_fixed_type_port(up, up->port.type);
+		up->port.wakeup_capable = wakeup_capable;
 
 		uart_add_one_port(drv, &up->port);
 	}
@@ -3219,6 +3221,7 @@ int serial8250_register_port(struct uart_port *port)
 			uart->port.set_termios = port->set_termios;
 		if (port->pm)
 			uart->port.pm = port->pm;
+		uart->port.wakeup_capable = wakeup_capable;
 
 		if (serial8250_isa_config != NULL)
 			serial8250_isa_config(0, &uart->port,
@@ -3252,6 +3255,7 @@ void serial8250_unregister_port(int line)
 		uart->port.type = PORT_UNKNOWN;
 		uart->port.dev = &serial8250_isa_devs->dev;
 		uart->capabilities = uart_config[uart->port.type].flags;
+		uart->port.wakeup_capable = wakeup_capable;
 		uart_add_one_port(&serial8250_reg, &uart->port);
 	} else {
 		uart->port.dev = NULL;
@@ -3355,3 +3359,6 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
 #endif
 MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
+
+module_param(wakeup_capable, uint, 0644);
+MODULE_PARM_DESC(wakeup_capable, "Allow driver to wake up the system");
-- 
1.7.7.3

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Alan Cox]

On Tue, 17 Jan 2012 10:56:03 -0800
Simon Glass <sjg@chromium.org> wrote:

> Since serial_core now does not make serial ports wake-up capable by
> default, add a parameter to support this feature in the 8250 UART.
> This is the only UART where I think this feature is useful.

NAK

Things should just work for users. Magic parameters is not an
improvement. If its a performance problem someone needs to fix the rcu
sync overhead or stop using rcu on that path.

Alan

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> On Tue, 17 Jan 2012 10:56:03 -0800
> Simon Glass <sjg@chromium.org> wrote:
> 
> > Since serial_core now does not make serial ports wake-up capable by
> > default, add a parameter to support this feature in the 8250 UART.
> > This is the only UART where I think this feature is useful.
> 
> NAK
> 
> Things should just work for users. Magic parameters is not an
> improvement. If its a performance problem someone needs to fix the rcu
> sync overhead or stop using rcu on that path.

I must say that I lack context here, even after looking at the patch,
but the synchronize_rcu_expedited() primitives can be used if the latency
of synchronize_rcu() is too large.

							Thanx, Paul

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

[+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]

Hi Alan, Paul,

On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> On Tue, 17 Jan 2012 10:56:03 -0800
>> Simon Glass <sjg@chromium.org> wrote:
>>
>> > Since serial_core now does not make serial ports wake-up capable by
>> > default, add a parameter to support this feature in the 8250 UART.
>> > This is the only UART where I think this feature is useful.
>>
>> NAK
>>
>> Things should just work for users. Magic parameters is not an
>> improvement. If its a performance problem someone needs to fix the rcu
>> sync overhead or stop using rcu on that path.

OK fair enough, I agree. Every level I move down the source tree
affects more people though.

>
> I must say that I lack context here, even after looking at the patch,
> but the synchronize_rcu_expedited() primitives can be used if the latency
> of synchronize_rcu() is too large.
>

Let me provide a bit of context. The serial_core code seems to be the
only place in the kernel that does this:

		device_init_wakeup(tty_dev, 1);
		device_set_wakeup_enable(tty_dev, 0);

The first call makes the device wakeup capable and enables wakeup, The
second call disabled wakeup.

The code that removes the wakeup source looks like this:

void wakeup_source_remove(struct wakeup_source *ws)
{
	if (WARN_ON(!ws))
		return;

	spin_lock_irq(&events_lock);
	list_del_rcu(&ws->entry);
	spin_unlock_irq(&events_lock);
	synchronize_rcu();
}

The sync is there because we are about to destroy the actual ws
structure (in wakeup_source_destroy()). I wonder if it should be in
wakeup_source_destroy() but that wouldn't help me anyway.

synchronize_rcu_expedited() is a bit faster but not really fast
enough. Anyway surely people will complain if I put this in the wakeup
code - it will affect all wakeup users. It seems to me that the right
solution is to avoid enabling and then immediately disabling wakeup.

I assume we can't and shouldn't change device_init_wakeup() . We could
add a call like device_init_wakeup_disabled() which makes the device
wakeup capable but does not actually enable it. Does that work?

Regards,
Simon

>                                                        Thanx, Paul
>

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> 
> Hi Alan, Paul,
> 
> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> Simon Glass <sjg@chromium.org> wrote:
> >>
> >> > Since serial_core now does not make serial ports wake-up capable by
> >> > default, add a parameter to support this feature in the 8250 UART.
> >> > This is the only UART where I think this feature is useful.
> >>
> >> NAK
> >>
> >> Things should just work for users. Magic parameters is not an
> >> improvement. If its a performance problem someone needs to fix the rcu
> >> sync overhead or stop using rcu on that path.
> 
> OK fair enough, I agree. Every level I move down the source tree
> affects more people though.
> 
> >
> > I must say that I lack context here, even after looking at the patch,
> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > of synchronize_rcu() is too large.
> >
> 
> Let me provide a bit of context. The serial_core code seems to be the
> only place in the kernel that does this:
> 
> 		device_init_wakeup(tty_dev, 1);
> 		device_set_wakeup_enable(tty_dev, 0);
> 
> The first call makes the device wakeup capable and enables wakeup, The
> second call disabled wakeup.
> 
> The code that removes the wakeup source looks like this:
> 
> void wakeup_source_remove(struct wakeup_source *ws)
> {
> 	if (WARN_ON(!ws))
> 		return;
> 
> 	spin_lock_irq(&events_lock);
> 	list_del_rcu(&ws->entry);
> 	spin_unlock_irq(&events_lock);
> 	synchronize_rcu();
> }
> 
> The sync is there because we are about to destroy the actual ws
> structure (in wakeup_source_destroy()). I wonder if it should be in
> wakeup_source_destroy() but that wouldn't help me anyway.
> 
> synchronize_rcu_expedited() is a bit faster but not really fast
> enough. Anyway surely people will complain if I put this in the wakeup
> code - it will affect all wakeup users. It seems to me that the right
> solution is to avoid enabling and then immediately disabling wakeup.

Hmmm...  What hardware are you running this one?  Normally,
synchronize_rcu_expedited() will be a couple of orders of magnitude
faster than synchronize_rcu().

> I assume we can't and shouldn't change device_init_wakeup() . We could
> add a call like device_init_wakeup_disabled() which makes the device
> wakeup capable but does not actually enable it. Does that work?

If the only reason for the synchronize_rcu() is to defer the pair of
kfree()s in wakeup_source_destroy(), then another possible approach
would be to remove the synchronize_rcu() from wakeup_source_remove()
and then use call_rcu() to defer the two kfree()s.

If this is a reasonable change to make, the approach is as follows:

1.	Add a struct rcu_head to wakeup_source, call it "rcu".
	Or adjust the following to suit your choice of name.

2.	Replace the pair of kfree()s with:

		call_rcu(&ws->rcu, wakeup_source_destroy_rcu);

3.	Create the wakeup_source_destroy_rcu() as follows:

	static void wakeup_source_destroy_rcu(struct rcu_head *head)
	{
		struct wakeup_source *ws =
			container_of(head, struct wakeup_source, rcu);

		kfree(ws->name);
		kfree(ws);
	}

Of course, this assumes that it is OK for wakeup_source_unregister()
to return before the memory is freed up.  This often is OK, but there
are some cases where the caller requires that there be no further
RCU readers with access to the old data.  In these cases, you really
do need the wait.

							Thanx, Paul

> Regards,
> Simon
> 
> >                                                        Thanx, Paul
> >
> 

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Hi Paul,

On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
>> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>>
>> Hi Alan, Paul,
>>
>> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> >> Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >> > Since serial_core now does not make serial ports wake-up capable by
>> >> > default, add a parameter to support this feature in the 8250 UART.
>> >> > This is the only UART where I think this feature is useful.
>> >>
>> >> NAK
>> >>
>> >> Things should just work for users. Magic parameters is not an
>> >> improvement. If its a performance problem someone needs to fix the rcu
>> >> sync overhead or stop using rcu on that path.
>>
>> OK fair enough, I agree. Every level I move down the source tree
>> affects more people though.
>>
>> >
>> > I must say that I lack context here, even after looking at the patch,
>> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> > of synchronize_rcu() is too large.
>> >
>>
>> Let me provide a bit of context. The serial_core code seems to be the
>> only place in the kernel that does this:
>>
>>               device_init_wakeup(tty_dev, 1);
>>               device_set_wakeup_enable(tty_dev, 0);
>>
>> The first call makes the device wakeup capable and enables wakeup, The
>> second call disabled wakeup.
>>
>> The code that removes the wakeup source looks like this:
>>
>> void wakeup_source_remove(struct wakeup_source *ws)
>> {
>>       if (WARN_ON(!ws))
>>               return;
>>
>>       spin_lock_irq(&events_lock);
>>       list_del_rcu(&ws->entry);
>>       spin_unlock_irq(&events_lock);
>>       synchronize_rcu();
>> }
>>
>> The sync is there because we are about to destroy the actual ws
>> structure (in wakeup_source_destroy()). I wonder if it should be in
>> wakeup_source_destroy() but that wouldn't help me anyway.
>>
>> synchronize_rcu_expedited() is a bit faster but not really fast
>> enough. Anyway surely people will complain if I put this in the wakeup
>> code - it will affect all wakeup users. It seems to me that the right
>> solution is to avoid enabling and then immediately disabling wakeup.
>
> Hmmm...  What hardware are you running this one?  Normally,
> synchronize_rcu_expedited() will be a couple of orders of magnitude
> faster than synchronize_rcu().
>
>> I assume we can't and shouldn't change device_init_wakeup() . We could
>> add a call like device_init_wakeup_disabled() which makes the device
>> wakeup capable but does not actually enable it. Does that work?
>
> If the only reason for the synchronize_rcu() is to defer the pair of
> kfree()s in wakeup_source_destroy(), then another possible approach
> would be to remove the synchronize_rcu() from wakeup_source_remove()
> and then use call_rcu() to defer the two kfree()s.
>
> If this is a reasonable change to make, the approach is as follows:
>
> 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
>        Or adjust the following to suit your choice of name.
>
> 2.      Replace the pair of kfree()s with:
>
>                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
>
> 3.      Create the wakeup_source_destroy_rcu() as follows:
>
>        static void wakeup_source_destroy_rcu(struct rcu_head *head)
>        {
>                struct wakeup_source *ws =
>                        container_of(head, struct wakeup_source, rcu);
>
>                kfree(ws->name);
>                kfree(ws);
>        }
>
> Of course, this assumes that it is OK for wakeup_source_unregister()
> to return before the memory is freed up.  This often is OK, but there
> are some cases where the caller requires that there be no further
> RCU readers with access to the old data.  In these cases, you really
> do need the wait.

Thanks very much for that. I'm not sure if it is a reasonable change,
but it does bug me that we add it to a data structure knowing that we
will immediately remove it!

>From what I can see, making a device wakeup-enabled mostly happens on
init or in response to a request to the driver (presumably from user
space). In the latter case I suspect the synchronise_rcu() is fine. In
the former it feels like we should make up our minds which of the
three options is required (incapable, capable but not enabled, capable
and enabled).

I will try a patch first based on splitting the two options (capable
and enable) and see if that get a NAK.

Then I will come back to your solution - it seems fine to me and not a
lot of code. Do we have to worry about someone enabling, disabled,
enabling and then disabling wakeup quickly? Will this method break in
that case if the second call to call_rcu() uses the same wc->rcu?

Regards,
Simon
>
>                                                        Thanx, Paul
>

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> Hi Paul,
> 
> On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> >>
> >> Hi Alan, Paul,
> >>
> >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> >> Simon Glass <sjg@chromium.org> wrote:
> >> >>
> >> >> > Since serial_core now does not make serial ports wake-up capable by
> >> >> > default, add a parameter to support this feature in the 8250 UART.
> >> >> > This is the only UART where I think this feature is useful.
> >> >>
> >> >> NAK
> >> >>
> >> >> Things should just work for users. Magic parameters is not an
> >> >> improvement. If its a performance problem someone needs to fix the rcu
> >> >> sync overhead or stop using rcu on that path.
> >>
> >> OK fair enough, I agree. Every level I move down the source tree
> >> affects more people though.
> >>
> >> >
> >> > I must say that I lack context here, even after looking at the patch,
> >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> >> > of synchronize_rcu() is too large.
> >> >
> >>
> >> Let me provide a bit of context. The serial_core code seems to be the
> >> only place in the kernel that does this:
> >>
> >>               device_init_wakeup(tty_dev, 1);
> >>               device_set_wakeup_enable(tty_dev, 0);
> >>
> >> The first call makes the device wakeup capable and enables wakeup, The
> >> second call disabled wakeup.
> >>
> >> The code that removes the wakeup source looks like this:
> >>
> >> void wakeup_source_remove(struct wakeup_source *ws)
> >> {
> >>       if (WARN_ON(!ws))
> >>               return;
> >>
> >>       spin_lock_irq(&events_lock);
> >>       list_del_rcu(&ws->entry);
> >>       spin_unlock_irq(&events_lock);
> >>       synchronize_rcu();
> >> }
> >>
> >> The sync is there because we are about to destroy the actual ws
> >> structure (in wakeup_source_destroy()). I wonder if it should be in
> >> wakeup_source_destroy() but that wouldn't help me anyway.
> >>
> >> synchronize_rcu_expedited() is a bit faster but not really fast
> >> enough. Anyway surely people will complain if I put this in the wakeup
> >> code - it will affect all wakeup users. It seems to me that the right
> >> solution is to avoid enabling and then immediately disabling wakeup.
> >
> > Hmmm...  What hardware are you running this one?  Normally,
> > synchronize_rcu_expedited() will be a couple of orders of magnitude
> > faster than synchronize_rcu().
> >
> >> I assume we can't and shouldn't change device_init_wakeup() . We could
> >> add a call like device_init_wakeup_disabled() which makes the device
> >> wakeup capable but does not actually enable it. Does that work?
> >
> > If the only reason for the synchronize_rcu() is to defer the pair of
> > kfree()s in wakeup_source_destroy(), then another possible approach
> > would be to remove the synchronize_rcu() from wakeup_source_remove()
> > and then use call_rcu() to defer the two kfree()s.
> >
> > If this is a reasonable change to make, the approach is as follows:
> >
> > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
> >        Or adjust the following to suit your choice of name.
> >
> > 2.      Replace the pair of kfree()s with:
> >
> >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> >
> > 3.      Create the wakeup_source_destroy_rcu() as follows:
> >
> >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
> >        {
> >                struct wakeup_source *ws =
> >                        container_of(head, struct wakeup_source, rcu);
> >
> >                kfree(ws->name);
> >                kfree(ws);
> >        }
> >
> > Of course, this assumes that it is OK for wakeup_source_unregister()
> > to return before the memory is freed up.  This often is OK, but there
> > are some cases where the caller requires that there be no further
> > RCU readers with access to the old data.  In these cases, you really
> > do need the wait.
> 
> Thanks very much for that. I'm not sure if it is a reasonable change,
> but it does bug me that we add it to a data structure knowing that we
> will immediately remove it!
> 
> >From what I can see, making a device wakeup-enabled mostly happens on
> init or in response to a request to the driver (presumably from user
> space). In the latter case I suspect the synchronise_rcu() is fine. In
> the former it feels like we should make up our minds which of the
> three options is required (incapable, capable but not enabled, capable
> and enabled).
> 
> I will try a patch first based on splitting the two options (capable
> and enable) and see if that get a NAK.
> 
> Then I will come back to your solution - it seems fine to me and not a
> lot of code. Do we have to worry about someone enabling, disabled,
> enabling and then disabling wakeup quickly? Will this method break in
> that case if the second call to call_rcu() uses the same wc->rcu?

There are a couple of questions here, let me take them one at a time:

1.	If you just disabled, can you immediately re-enable?

	The answer is "yes".  The reason that this works is that you
	allocate a new structure for the re-enabling, and that new
	structure has its own rcu_head field.

2.	If you repeatedly disable and re-enable in a tight loop,
	can this cause problems?

	The answer to this is also "yes" -- you can run the system
	out of memory doing that.  However, there are a number of
	simple ways to avoid this problem:

	a.	Do a synchronize_rcu() on every (say) thousandth
		disable operation.

	b.	As above, but only do the synchronize_rcu() if
		all 1,000 disable operations occurred within
		(say) a second of each other.

	c.	As above, but actually count the number of
		pending call_rcu() callbacks.

	Both (a) and (b) can be carried out on a per-CPU basis if there
	is no convenient locked structure in which to track the state.
	You cannot carry (c) out on a per-CPU basis because RCU callbacks
	can sometimes be invoked on a different CPU from the one that
	call_rcu()ed them.  Rare, but it can happen.

	I would expect that option (a) would work in almost all cases.

If this can be exercised freely from user space, then you probably
really do need #2 above.

						Thanx, Paul

> Regards,
> Simon
> >
> >                                                        Thanx, Paul
> >
> 

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Hi Paul,

On Wed, Jan 18, 2012 at 2:43 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
>> Hi Paul,
>>
>> On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
>> >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>> >>
>> >> Hi Alan, Paul,
>> >>
>> >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> >> <paulmck@linux.vnet.ibm.com> wrote:
>> >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> >> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> >> >> Simon Glass <sjg@chromium.org> wrote:
>> >> >>
>> >> >> > Since serial_core now does not make serial ports wake-up capable by
>> >> >> > default, add a parameter to support this feature in the 8250 UART.
>> >> >> > This is the only UART where I think this feature is useful.
>> >> >>
>> >> >> NAK
>> >> >>
>> >> >> Things should just work for users. Magic parameters is not an
>> >> >> improvement. If its a performance problem someone needs to fix the rcu
>> >> >> sync overhead or stop using rcu on that path.
>> >>
>> >> OK fair enough, I agree. Every level I move down the source tree
>> >> affects more people though.
>> >>
>> >> >
>> >> > I must say that I lack context here, even after looking at the patch,
>> >> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> >> > of synchronize_rcu() is too large.
>> >> >
>> >>
>> >> Let me provide a bit of context. The serial_core code seems to be the
>> >> only place in the kernel that does this:
>> >>
>> >>               device_init_wakeup(tty_dev, 1);
>> >>               device_set_wakeup_enable(tty_dev, 0);
>> >>
>> >> The first call makes the device wakeup capable and enables wakeup, The
>> >> second call disabled wakeup.
>> >>
>> >> The code that removes the wakeup source looks like this:
>> >>
>> >> void wakeup_source_remove(struct wakeup_source *ws)
>> >> {
>> >>       if (WARN_ON(!ws))
>> >>               return;
>> >>
>> >>       spin_lock_irq(&events_lock);
>> >>       list_del_rcu(&ws->entry);
>> >>       spin_unlock_irq(&events_lock);
>> >>       synchronize_rcu();
>> >> }
>> >>
>> >> The sync is there because we are about to destroy the actual ws
>> >> structure (in wakeup_source_destroy()). I wonder if it should be in
>> >> wakeup_source_destroy() but that wouldn't help me anyway.
>> >>
>> >> synchronize_rcu_expedited() is a bit faster but not really fast
>> >> enough. Anyway surely people will complain if I put this in the wakeup
>> >> code - it will affect all wakeup users. It seems to me that the right
>> >> solution is to avoid enabling and then immediately disabling wakeup.
>> >
>> > Hmmm...  What hardware are you running this one?  Normally,
>> > synchronize_rcu_expedited() will be a couple of orders of magnitude
>> > faster than synchronize_rcu().
>> >
>> >> I assume we can't and shouldn't change device_init_wakeup() . We could
>> >> add a call like device_init_wakeup_disabled() which makes the device
>> >> wakeup capable but does not actually enable it. Does that work?
>> >
>> > If the only reason for the synchronize_rcu() is to defer the pair of
>> > kfree()s in wakeup_source_destroy(), then another possible approach
>> > would be to remove the synchronize_rcu() from wakeup_source_remove()
>> > and then use call_rcu() to defer the two kfree()s.
>> >
>> > If this is a reasonable change to make, the approach is as follows:
>> >
>> > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
>> >        Or adjust the following to suit your choice of name.
>> >
>> > 2.      Replace the pair of kfree()s with:
>> >
>> >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
>> >
>> > 3.      Create the wakeup_source_destroy_rcu() as follows:
>> >
>> >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
>> >        {
>> >                struct wakeup_source *ws =
>> >                        container_of(head, struct wakeup_source, rcu);
>> >
>> >                kfree(ws->name);
>> >                kfree(ws);
>> >        }
>> >
>> > Of course, this assumes that it is OK for wakeup_source_unregister()
>> > to return before the memory is freed up.  This often is OK, but there
>> > are some cases where the caller requires that there be no further
>> > RCU readers with access to the old data.  In these cases, you really
>> > do need the wait.
>>
>> Thanks very much for that. I'm not sure if it is a reasonable change,
>> but it does bug me that we add it to a data structure knowing that we
>> will immediately remove it!
>>
>> >From what I can see, making a device wakeup-enabled mostly happens on
>> init or in response to a request to the driver (presumably from user
>> space). In the latter case I suspect the synchronise_rcu() is fine. In
>> the former it feels like we should make up our minds which of the
>> three options is required (incapable, capable but not enabled, capable
>> and enabled).
>>
>> I will try a patch first based on splitting the two options (capable
>> and enable) and see if that get a NAK.
>>
>> Then I will come back to your solution - it seems fine to me and not a
>> lot of code. Do we have to worry about someone enabling, disabled,
>> enabling and then disabling wakeup quickly? Will this method break in
>> that case if the second call to call_rcu() uses the same wc->rcu?
>
> There are a couple of questions here, let me take them one at a time:
>
> 1.      If you just disabled, can you immediately re-enable?
>
>        The answer is "yes".  The reason that this works is that you
>        allocate a new structure for the re-enabling, and that new
>        structure has its own rcu_head field.
>
> 2.      If you repeatedly disable and re-enable in a tight loop,
>        can this cause problems?
>
>        The answer to this is also "yes" -- you can run the system
>        out of memory doing that.  However, there are a number of
>        simple ways to avoid this problem:
>
>        a.      Do a synchronize_rcu() on every (say) thousandth
>                disable operation.
>
>        b.      As above, but only do the synchronize_rcu() if
>                all 1,000 disable operations occurred within
>                (say) a second of each other.
>
>        c.      As above, but actually count the number of
>                pending call_rcu() callbacks.
>
>        Both (a) and (b) can be carried out on a per-CPU basis if there
>        is no convenient locked structure in which to track the state.
>        You cannot carry (c) out on a per-CPU basis because RCU callbacks
>        can sometimes be invoked on a different CPU from the one that
>        call_rcu()ed them.  Rare, but it can happen.
>
>        I would expect that option (a) would work in almost all cases.
>
> If this can be exercised freely from user space, then you probably
> really do need #2 above.

OK I see, thank you. It does sound a bit complicated although the
chances of anyone actually doing this are probably remote.

I will send my patch to avoid getting into this situation and see what
you think.

Regards,
Simon

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Rafael J. Wysocki]

On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > Hi Paul,
> > 
> > On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> > >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> > >>
> > >> Hi Alan, Paul,
> > >>
> > >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> > >> <paulmck@linux.vnet.ibm.com> wrote:
> > >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> > >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> > >> >> Simon Glass <sjg@chromium.org> wrote:
> > >> >>
> > >> >> > Since serial_core now does not make serial ports wake-up capable by
> > >> >> > default, add a parameter to support this feature in the 8250 UART.
> > >> >> > This is the only UART where I think this feature is useful.
> > >> >>
> > >> >> NAK
> > >> >>
> > >> >> Things should just work for users. Magic parameters is not an
> > >> >> improvement. If its a performance problem someone needs to fix the rcu
> > >> >> sync overhead or stop using rcu on that path.
> > >>
> > >> OK fair enough, I agree. Every level I move down the source tree
> > >> affects more people though.
> > >>
> > >> >
> > >> > I must say that I lack context here, even after looking at the patch,
> > >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > >> > of synchronize_rcu() is too large.
> > >> >
> > >>
> > >> Let me provide a bit of context. The serial_core code seems to be the
> > >> only place in the kernel that does this:
> > >>
> > >>               device_init_wakeup(tty_dev, 1);
> > >>               device_set_wakeup_enable(tty_dev, 0);
> > >>
> > >> The first call makes the device wakeup capable and enables wakeup, The
> > >> second call disabled wakeup.
> > >>
> > >> The code that removes the wakeup source looks like this:
> > >>
> > >> void wakeup_source_remove(struct wakeup_source *ws)
> > >> {
> > >>       if (WARN_ON(!ws))
> > >>               return;
> > >>
> > >>       spin_lock_irq(&events_lock);
> > >>       list_del_rcu(&ws->entry);
> > >>       spin_unlock_irq(&events_lock);
> > >>       synchronize_rcu();
> > >> }
> > >>
> > >> The sync is there because we are about to destroy the actual ws
> > >> structure (in wakeup_source_destroy()). I wonder if it should be in
> > >> wakeup_source_destroy() but that wouldn't help me anyway.
> > >>
> > >> synchronize_rcu_expedited() is a bit faster but not really fast
> > >> enough. Anyway surely people will complain if I put this in the wakeup
> > >> code - it will affect all wakeup users. It seems to me that the right
> > >> solution is to avoid enabling and then immediately disabling wakeup.
> > >
> > > Hmmm...  What hardware are you running this one?  Normally,
> > > synchronize_rcu_expedited() will be a couple of orders of magnitude
> > > faster than synchronize_rcu().
> > >
> > >> I assume we can't and shouldn't change device_init_wakeup() . We could
> > >> add a call like device_init_wakeup_disabled() which makes the device
> > >> wakeup capable but does not actually enable it. Does that work?
> > >
> > > If the only reason for the synchronize_rcu() is to defer the pair of
> > > kfree()s in wakeup_source_destroy(), then another possible approach
> > > would be to remove the synchronize_rcu() from wakeup_source_remove()
> > > and then use call_rcu() to defer the two kfree()s.
> > >
> > > If this is a reasonable change to make, the approach is as follows:
> > >
> > > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
> > >        Or adjust the following to suit your choice of name.
> > >
> > > 2.      Replace the pair of kfree()s with:
> > >
> > >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> > >
> > > 3.      Create the wakeup_source_destroy_rcu() as follows:
> > >
> > >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
> > >        {
> > >                struct wakeup_source *ws =
> > >                        container_of(head, struct wakeup_source, rcu);
> > >
> > >                kfree(ws->name);
> > >                kfree(ws);
> > >        }
> > >
> > > Of course, this assumes that it is OK for wakeup_source_unregister()
> > > to return before the memory is freed up.  This often is OK, but there
> > > are some cases where the caller requires that there be no further
> > > RCU readers with access to the old data.  In these cases, you really
> > > do need the wait.
> > 
> > Thanks very much for that. I'm not sure if it is a reasonable change,
> > but it does bug me that we add it to a data structure knowing that we
> > will immediately remove it!
> > 
> > >From what I can see, making a device wakeup-enabled mostly happens on
> > init or in response to a request to the driver (presumably from user
> > space). In the latter case I suspect the synchronise_rcu() is fine. In
> > the former it feels like we should make up our minds which of the
> > three options is required (incapable, capable but not enabled, capable
> > and enabled).
> > 
> > I will try a patch first based on splitting the two options (capable
> > and enable) and see if that get a NAK.
> > 
> > Then I will come back to your solution - it seems fine to me and not a
> > lot of code. Do we have to worry about someone enabling, disabled,
> > enabling and then disabling wakeup quickly? Will this method break in
> > that case if the second call to call_rcu() uses the same wc->rcu?
> 
> There are a couple of questions here, let me take them one at a time:
> 
> 1.	If you just disabled, can you immediately re-enable?
> 
> 	The answer is "yes".  The reason that this works is that you
> 	allocate a new structure for the re-enabling, and that new
> 	structure has its own rcu_head field.
> 
> 2.	If you repeatedly disable and re-enable in a tight loop,
> 	can this cause problems?
> 
> 	The answer to this is also "yes" -- you can run the system
> 	out of memory doing that.  However, there are a number of
> 	simple ways to avoid this problem:
> 
> 	a.	Do a synchronize_rcu() on every (say) thousandth
> 		disable operation.
> 
> 	b.	As above, but only do the synchronize_rcu() if
> 		all 1,000 disable operations occurred within
> 		(say) a second of each other.
> 
> 	c.	As above, but actually count the number of
> 		pending call_rcu() callbacks.
> 
> 	Both (a) and (b) can be carried out on a per-CPU basis if there
> 	is no convenient locked structure in which to track the state.
> 	You cannot carry (c) out on a per-CPU basis because RCU callbacks
> 	can sometimes be invoked on a different CPU from the one that
> 	call_rcu()ed them.  Rare, but it can happen.
> 
> 	I would expect that option (a) would work in almost all cases.
> 
> If this can be exercised freely from user space, then you probably
> really do need #2 above.

Yes, you can, but then I'd say it's not necessary for user space to
be able to carry that out in a tight loop.  So, it seems, alternatively,
we could make that loop a bit less tight, e.g. by adding an arbitrary
sleep to the user space interface for the "disable" case.

Thanks,
Rafael

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > > Hi Paul,
> > > 
> > > On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> > > <paulmck@linux.vnet.ibm.com> wrote:
> > > > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> > > >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> > > >>
> > > >> Hi Alan, Paul,
> > > >>
> > > >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> > > >> <paulmck@linux.vnet.ibm.com> wrote:
> > > >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> > > >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> > > >> >> Simon Glass <sjg@chromium.org> wrote:
> > > >> >>
> > > >> >> > Since serial_core now does not make serial ports wake-up capable by
> > > >> >> > default, add a parameter to support this feature in the 8250 UART.
> > > >> >> > This is the only UART where I think this feature is useful.
> > > >> >>
> > > >> >> NAK
> > > >> >>
> > > >> >> Things should just work for users. Magic parameters is not an
> > > >> >> improvement. If its a performance problem someone needs to fix the rcu
> > > >> >> sync overhead or stop using rcu on that path.
> > > >>
> > > >> OK fair enough, I agree. Every level I move down the source tree
> > > >> affects more people though.
> > > >>
> > > >> >
> > > >> > I must say that I lack context here, even after looking at the patch,
> > > >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > > >> > of synchronize_rcu() is too large.
> > > >> >
> > > >>
> > > >> Let me provide a bit of context. The serial_core code seems to be the
> > > >> only place in the kernel that does this:
> > > >>
> > > >>               device_init_wakeup(tty_dev, 1);
> > > >>               device_set_wakeup_enable(tty_dev, 0);
> > > >>
> > > >> The first call makes the device wakeup capable and enables wakeup, The
> > > >> second call disabled wakeup.
> > > >>
> > > >> The code that removes the wakeup source looks like this:
> > > >>
> > > >> void wakeup_source_remove(struct wakeup_source *ws)
> > > >> {
> > > >>       if (WARN_ON(!ws))
> > > >>               return;
> > > >>
> > > >>       spin_lock_irq(&events_lock);
> > > >>       list_del_rcu(&ws->entry);
> > > >>       spin_unlock_irq(&events_lock);
> > > >>       synchronize_rcu();
> > > >> }
> > > >>
> > > >> The sync is there because we are about to destroy the actual ws
> > > >> structure (in wakeup_source_destroy()). I wonder if it should be in
> > > >> wakeup_source_destroy() but that wouldn't help me anyway.
> > > >>
> > > >> synchronize_rcu_expedited() is a bit faster but not really fast
> > > >> enough. Anyway surely people will complain if I put this in the wakeup
> > > >> code - it will affect all wakeup users. It seems to me that the right
> > > >> solution is to avoid enabling and then immediately disabling wakeup.
> > > >
> > > > Hmmm...  What hardware are you running this one?  Normally,
> > > > synchronize_rcu_expedited() will be a couple of orders of magnitude
> > > > faster than synchronize_rcu().
> > > >
> > > >> I assume we can't and shouldn't change device_init_wakeup() . We could
> > > >> add a call like device_init_wakeup_disabled() which makes the device
> > > >> wakeup capable but does not actually enable it. Does that work?
> > > >
> > > > If the only reason for the synchronize_rcu() is to defer the pair of
> > > > kfree()s in wakeup_source_destroy(), then another possible approach
> > > > would be to remove the synchronize_rcu() from wakeup_source_remove()
> > > > and then use call_rcu() to defer the two kfree()s.
> > > >
> > > > If this is a reasonable change to make, the approach is as follows:
> > > >
> > > > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
> > > >        Or adjust the following to suit your choice of name.
> > > >
> > > > 2.      Replace the pair of kfree()s with:
> > > >
> > > >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> > > >
> > > > 3.      Create the wakeup_source_destroy_rcu() as follows:
> > > >
> > > >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
> > > >        {
> > > >                struct wakeup_source *ws =
> > > >                        container_of(head, struct wakeup_source, rcu);
> > > >
> > > >                kfree(ws->name);
> > > >                kfree(ws);
> > > >        }
> > > >
> > > > Of course, this assumes that it is OK for wakeup_source_unregister()
> > > > to return before the memory is freed up.  This often is OK, but there
> > > > are some cases where the caller requires that there be no further
> > > > RCU readers with access to the old data.  In these cases, you really
> > > > do need the wait.
> > > 
> > > Thanks very much for that. I'm not sure if it is a reasonable change,
> > > but it does bug me that we add it to a data structure knowing that we
> > > will immediately remove it!
> > > 
> > > >From what I can see, making a device wakeup-enabled mostly happens on
> > > init or in response to a request to the driver (presumably from user
> > > space). In the latter case I suspect the synchronise_rcu() is fine. In
> > > the former it feels like we should make up our minds which of the
> > > three options is required (incapable, capable but not enabled, capable
> > > and enabled).
> > > 
> > > I will try a patch first based on splitting the two options (capable
> > > and enable) and see if that get a NAK.
> > > 
> > > Then I will come back to your solution - it seems fine to me and not a
> > > lot of code. Do we have to worry about someone enabling, disabled,
> > > enabling and then disabling wakeup quickly? Will this method break in
> > > that case if the second call to call_rcu() uses the same wc->rcu?
> > 
> > There are a couple of questions here, let me take them one at a time:
> > 
> > 1.	If you just disabled, can you immediately re-enable?
> > 
> > 	The answer is "yes".  The reason that this works is that you
> > 	allocate a new structure for the re-enabling, and that new
> > 	structure has its own rcu_head field.
> > 
> > 2.	If you repeatedly disable and re-enable in a tight loop,
> > 	can this cause problems?
> > 
> > 	The answer to this is also "yes" -- you can run the system
> > 	out of memory doing that.  However, there are a number of
> > 	simple ways to avoid this problem:
> > 
> > 	a.	Do a synchronize_rcu() on every (say) thousandth
> > 		disable operation.
> > 
> > 	b.	As above, but only do the synchronize_rcu() if
> > 		all 1,000 disable operations occurred within
> > 		(say) a second of each other.
> > 
> > 	c.	As above, but actually count the number of
> > 		pending call_rcu() callbacks.
> > 
> > 	Both (a) and (b) can be carried out on a per-CPU basis if there
> > 	is no convenient locked structure in which to track the state.
> > 	You cannot carry (c) out on a per-CPU basis because RCU callbacks
> > 	can sometimes be invoked on a different CPU from the one that
> > 	call_rcu()ed them.  Rare, but it can happen.
> > 
> > 	I would expect that option (a) would work in almost all cases.
> > 
> > If this can be exercised freely from user space, then you probably
> > really do need #2 above.
> 
> Yes, you can, but then I'd say it's not necessary for user space to
> be able to carry that out in a tight loop.  So, it seems, alternatively,
> we could make that loop a bit less tight, e.g. by adding an arbitrary
> sleep to the user space interface for the "disable" case.

Good point, that would work just as well and be simpler.

							Thanx, Paul

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Hi Paul,

On Wed, Jan 18, 2012 at 5:37 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
>> On Wednesday, January 18, 2012, Paul E. McKenney wrote:
>> > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
>> > > Hi Paul,
>> > >
>> > > On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
>> > > <paulmck@linux.vnet.ibm.com> wrote:
>> > > > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
>> > > >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>> > > >>
>> > > >> Hi Alan, Paul,
>> > > >>
>> > > >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> > > >> <paulmck@linux.vnet.ibm.com> wrote:
>> > > >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> > > >> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> > > >> >> Simon Glass <sjg@chromium.org> wrote:
>> > > >> >>
>> > > >> >> > Since serial_core now does not make serial ports wake-up capable by
>> > > >> >> > default, add a parameter to support this feature in the 8250 UART.
>> > > >> >> > This is the only UART where I think this feature is useful.
>> > > >> >>
>> > > >> >> NAK
>> > > >> >>
>> > > >> >> Things should just work for users. Magic parameters is not an
>> > > >> >> improvement. If its a performance problem someone needs to fix the rcu
>> > > >> >> sync overhead or stop using rcu on that path.
>> > > >>
>> > > >> OK fair enough, I agree. Every level I move down the source tree
>> > > >> affects more people though.
>> > > >>
>> > > >> >
>> > > >> > I must say that I lack context here, even after looking at the patch,
>> > > >> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> > > >> > of synchronize_rcu() is too large.
>> > > >> >
>> > > >>
>> > > >> Let me provide a bit of context. The serial_core code seems to be the
>> > > >> only place in the kernel that does this:
>> > > >>
>> > > >>               device_init_wakeup(tty_dev, 1);
>> > > >>               device_set_wakeup_enable(tty_dev, 0);
>> > > >>
>> > > >> The first call makes the device wakeup capable and enables wakeup, The
>> > > >> second call disabled wakeup.
>> > > >>
>> > > >> The code that removes the wakeup source looks like this:
>> > > >>
>> > > >> void wakeup_source_remove(struct wakeup_source *ws)
>> > > >> {
>> > > >>       if (WARN_ON(!ws))
>> > > >>               return;
>> > > >>
>> > > >>       spin_lock_irq(&events_lock);
>> > > >>       list_del_rcu(&ws->entry);
>> > > >>       spin_unlock_irq(&events_lock);
>> > > >>       synchronize_rcu();
>> > > >> }
>> > > >>
>> > > >> The sync is there because we are about to destroy the actual ws
>> > > >> structure (in wakeup_source_destroy()). I wonder if it should be in
>> > > >> wakeup_source_destroy() but that wouldn't help me anyway.
>> > > >>
>> > > >> synchronize_rcu_expedited() is a bit faster but not really fast
>> > > >> enough. Anyway surely people will complain if I put this in the wakeup
>> > > >> code - it will affect all wakeup users. It seems to me that the right
>> > > >> solution is to avoid enabling and then immediately disabling wakeup.
>> > > >
>> > > > Hmmm...  What hardware are you running this one?  Normally,
>> > > > synchronize_rcu_expedited() will be a couple of orders of magnitude
>> > > > faster than synchronize_rcu().
>> > > >
>> > > >> I assume we can't and shouldn't change device_init_wakeup() . We could
>> > > >> add a call like device_init_wakeup_disabled() which makes the device
>> > > >> wakeup capable but does not actually enable it. Does that work?
>> > > >
>> > > > If the only reason for the synchronize_rcu() is to defer the pair of
>> > > > kfree()s in wakeup_source_destroy(), then another possible approach
>> > > > would be to remove the synchronize_rcu() from wakeup_source_remove()
>> > > > and then use call_rcu() to defer the two kfree()s.
>> > > >
>> > > > If this is a reasonable change to make, the approach is as follows:
>> > > >
>> > > > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
>> > > >        Or adjust the following to suit your choice of name.
>> > > >
>> > > > 2.      Replace the pair of kfree()s with:
>> > > >
>> > > >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
>> > > >
>> > > > 3.      Create the wakeup_source_destroy_rcu() as follows:
>> > > >
>> > > >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
>> > > >        {
>> > > >                struct wakeup_source *ws =
>> > > >                        container_of(head, struct wakeup_source, rcu);
>> > > >
>> > > >                kfree(ws->name);
>> > > >                kfree(ws);
>> > > >        }
>> > > >
>> > > > Of course, this assumes that it is OK for wakeup_source_unregister()
>> > > > to return before the memory is freed up.  This often is OK, but there
>> > > > are some cases where the caller requires that there be no further
>> > > > RCU readers with access to the old data.  In these cases, you really
>> > > > do need the wait.
>> > >
>> > > Thanks very much for that. I'm not sure if it is a reasonable change,
>> > > but it does bug me that we add it to a data structure knowing that we
>> > > will immediately remove it!
>> > >
>> > > >From what I can see, making a device wakeup-enabled mostly happens on
>> > > init or in response to a request to the driver (presumably from user
>> > > space). In the latter case I suspect the synchronise_rcu() is fine. In
>> > > the former it feels like we should make up our minds which of the
>> > > three options is required (incapable, capable but not enabled, capable
>> > > and enabled).
>> > >
>> > > I will try a patch first based on splitting the two options (capable
>> > > and enable) and see if that get a NAK.
>> > >
>> > > Then I will come back to your solution - it seems fine to me and not a
>> > > lot of code. Do we have to worry about someone enabling, disabled,
>> > > enabling and then disabling wakeup quickly? Will this method break in
>> > > that case if the second call to call_rcu() uses the same wc->rcu?
>> >
>> > There are a couple of questions here, let me take them one at a time:
>> >
>> > 1.  If you just disabled, can you immediately re-enable?
>> >
>> >     The answer is "yes".  The reason that this works is that you
>> >     allocate a new structure for the re-enabling, and that new
>> >     structure has its own rcu_head field.
>> >
>> > 2.  If you repeatedly disable and re-enable in a tight loop,
>> >     can this cause problems?
>> >
>> >     The answer to this is also "yes" -- you can run the system
>> >     out of memory doing that.  However, there are a number of
>> >     simple ways to avoid this problem:
>> >
>> >     a.      Do a synchronize_rcu() on every (say) thousandth
>> >             disable operation.
>> >
>> >     b.      As above, but only do the synchronize_rcu() if
>> >             all 1,000 disable operations occurred within
>> >             (say) a second of each other.
>> >
>> >     c.      As above, but actually count the number of
>> >             pending call_rcu() callbacks.
>> >
>> >     Both (a) and (b) can be carried out on a per-CPU basis if there
>> >     is no convenient locked structure in which to track the state.
>> >     You cannot carry (c) out on a per-CPU basis because RCU callbacks
>> >     can sometimes be invoked on a different CPU from the one that
>> >     call_rcu()ed them.  Rare, but it can happen.
>> >
>> >     I would expect that option (a) would work in almost all cases.
>> >
>> > If this can be exercised freely from user space, then you probably
>> > really do need #2 above.
>>
>> Yes, you can, but then I'd say it's not necessary for user space to
>> be able to carry that out in a tight loop.  So, it seems, alternatively,
>> we could make that loop a bit less tight, e.g. by adding an arbitrary
>> sleep to the user space interface for the "disable" case.
>
> Good point, that would work just as well and be simpler.

OK, well I am expecting that this will now be a very small patch to
change just serial_core.

Thanks for your help with this.

Regards,
Simon

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Wed, Jan 18, 2012 at 06:35:56PM -0800, Simon Glass wrote:
> Hi Paul,
> 
> On Wed, Jan 18, 2012 at 5:37 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> >> On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> >> > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> >> > > Hi Paul,
> >> > >
> >> > > On Wed, Jan 18, 2012 at 1:42 PM, Paul E. McKenney
> >> > > <paulmck@linux.vnet.ibm.com> wrote:
> >> > > > On Wed, Jan 18, 2012 at 01:08:13PM -0800, Simon Glass wrote:
> >> > > >> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> >> > > >>
> >> > > >> Hi Alan, Paul,
> >> > > >>
> >> > > >> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> >> > > >> <paulmck@linux.vnet.ibm.com> wrote:
> >> > > >> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> > > >> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> > > >> >> Simon Glass <sjg@chromium.org> wrote:
> >> > > >> >>
> >> > > >> >> > Since serial_core now does not make serial ports wake-up capable by
> >> > > >> >> > default, add a parameter to support this feature in the 8250 UART.
> >> > > >> >> > This is the only UART where I think this feature is useful.
> >> > > >> >>
> >> > > >> >> NAK
> >> > > >> >>
> >> > > >> >> Things should just work for users. Magic parameters is not an
> >> > > >> >> improvement. If its a performance problem someone needs to fix the rcu
> >> > > >> >> sync overhead or stop using rcu on that path.
> >> > > >>
> >> > > >> OK fair enough, I agree. Every level I move down the source tree
> >> > > >> affects more people though.
> >> > > >>
> >> > > >> >
> >> > > >> > I must say that I lack context here, even after looking at the patch,
> >> > > >> > but the synchronize_rcu_expedited() primitives can be used if the latency
> >> > > >> > of synchronize_rcu() is too large.
> >> > > >> >
> >> > > >>
> >> > > >> Let me provide a bit of context. The serial_core code seems to be the
> >> > > >> only place in the kernel that does this:
> >> > > >>
> >> > > >>               device_init_wakeup(tty_dev, 1);
> >> > > >>               device_set_wakeup_enable(tty_dev, 0);
> >> > > >>
> >> > > >> The first call makes the device wakeup capable and enables wakeup, The
> >> > > >> second call disabled wakeup.
> >> > > >>
> >> > > >> The code that removes the wakeup source looks like this:
> >> > > >>
> >> > > >> void wakeup_source_remove(struct wakeup_source *ws)
> >> > > >> {
> >> > > >>       if (WARN_ON(!ws))
> >> > > >>               return;
> >> > > >>
> >> > > >>       spin_lock_irq(&events_lock);
> >> > > >>       list_del_rcu(&ws->entry);
> >> > > >>       spin_unlock_irq(&events_lock);
> >> > > >>       synchronize_rcu();
> >> > > >> }
> >> > > >>
> >> > > >> The sync is there because we are about to destroy the actual ws
> >> > > >> structure (in wakeup_source_destroy()). I wonder if it should be in
> >> > > >> wakeup_source_destroy() but that wouldn't help me anyway.
> >> > > >>
> >> > > >> synchronize_rcu_expedited() is a bit faster but not really fast
> >> > > >> enough. Anyway surely people will complain if I put this in the wakeup
> >> > > >> code - it will affect all wakeup users. It seems to me that the right
> >> > > >> solution is to avoid enabling and then immediately disabling wakeup.
> >> > > >
> >> > > > Hmmm...  What hardware are you running this one?  Normally,
> >> > > > synchronize_rcu_expedited() will be a couple of orders of magnitude
> >> > > > faster than synchronize_rcu().
> >> > > >
> >> > > >> I assume we can't and shouldn't change device_init_wakeup() . We could
> >> > > >> add a call like device_init_wakeup_disabled() which makes the device
> >> > > >> wakeup capable but does not actually enable it. Does that work?
> >> > > >
> >> > > > If the only reason for the synchronize_rcu() is to defer the pair of
> >> > > > kfree()s in wakeup_source_destroy(), then another possible approach
> >> > > > would be to remove the synchronize_rcu() from wakeup_source_remove()
> >> > > > and then use call_rcu() to defer the two kfree()s.
> >> > > >
> >> > > > If this is a reasonable change to make, the approach is as follows:
> >> > > >
> >> > > > 1.      Add a struct rcu_head to wakeup_source, call it "rcu".
> >> > > >        Or adjust the following to suit your choice of name.
> >> > > >
> >> > > > 2.      Replace the pair of kfree()s with:
> >> > > >
> >> > > >                call_rcu(&ws->rcu, wakeup_source_destroy_rcu);
> >> > > >
> >> > > > 3.      Create the wakeup_source_destroy_rcu() as follows:
> >> > > >
> >> > > >        static void wakeup_source_destroy_rcu(struct rcu_head *head)
> >> > > >        {
> >> > > >                struct wakeup_source *ws =
> >> > > >                        container_of(head, struct wakeup_source, rcu);
> >> > > >
> >> > > >                kfree(ws->name);
> >> > > >                kfree(ws);
> >> > > >        }
> >> > > >
> >> > > > Of course, this assumes that it is OK for wakeup_source_unregister()
> >> > > > to return before the memory is freed up.  This often is OK, but there
> >> > > > are some cases where the caller requires that there be no further
> >> > > > RCU readers with access to the old data.  In these cases, you really
> >> > > > do need the wait.
> >> > >
> >> > > Thanks very much for that. I'm not sure if it is a reasonable change,
> >> > > but it does bug me that we add it to a data structure knowing that we
> >> > > will immediately remove it!
> >> > >
> >> > > >From what I can see, making a device wakeup-enabled mostly happens on
> >> > > init or in response to a request to the driver (presumably from user
> >> > > space). In the latter case I suspect the synchronise_rcu() is fine. In
> >> > > the former it feels like we should make up our minds which of the
> >> > > three options is required (incapable, capable but not enabled, capable
> >> > > and enabled).
> >> > >
> >> > > I will try a patch first based on splitting the two options (capable
> >> > > and enable) and see if that get a NAK.
> >> > >
> >> > > Then I will come back to your solution - it seems fine to me and not a
> >> > > lot of code. Do we have to worry about someone enabling, disabled,
> >> > > enabling and then disabling wakeup quickly? Will this method break in
> >> > > that case if the second call to call_rcu() uses the same wc->rcu?
> >> >
> >> > There are a couple of questions here, let me take them one at a time:
> >> >
> >> > 1.  If you just disabled, can you immediately re-enable?
> >> >
> >> >     The answer is "yes".  The reason that this works is that you
> >> >     allocate a new structure for the re-enabling, and that new
> >> >     structure has its own rcu_head field.
> >> >
> >> > 2.  If you repeatedly disable and re-enable in a tight loop,
> >> >     can this cause problems?
> >> >
> >> >     The answer to this is also "yes" -- you can run the system
> >> >     out of memory doing that.  However, there are a number of
> >> >     simple ways to avoid this problem:
> >> >
> >> >     a.      Do a synchronize_rcu() on every (say) thousandth
> >> >             disable operation.
> >> >
> >> >     b.      As above, but only do the synchronize_rcu() if
> >> >             all 1,000 disable operations occurred within
> >> >             (say) a second of each other.
> >> >
> >> >     c.      As above, but actually count the number of
> >> >             pending call_rcu() callbacks.
> >> >
> >> >     Both (a) and (b) can be carried out on a per-CPU basis if there
> >> >     is no convenient locked structure in which to track the state.
> >> >     You cannot carry (c) out on a per-CPU basis because RCU callbacks
> >> >     can sometimes be invoked on a different CPU from the one that
> >> >     call_rcu()ed them.  Rare, but it can happen.
> >> >
> >> >     I would expect that option (a) would work in almost all cases.
> >> >
> >> > If this can be exercised freely from user space, then you probably
> >> > really do need #2 above.
> >>
> >> Yes, you can, but then I'd say it's not necessary for user space to
> >> be able to carry that out in a tight loop.  So, it seems, alternatively,
> >> we could make that loop a bit less tight, e.g. by adding an arbitrary
> >> sleep to the user space interface for the "disable" case.
> >
> > Good point, that would work just as well and be simpler.
> 
> OK, well I am expecting that this will now be a very small patch to
> change just serial_core.
> 
> Thanks for your help with this.

Glad to help, and even more glad that Alan and Rafael were able to help.  ;-)

							Thanx, Paul

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Rafael J. Wysocki]

On Thursday, January 19, 2012, Paul E. McKenney wrote:
> On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
[...] 
> > Yes, you can, but then I'd say it's not necessary for user space to
> > be able to carry that out in a tight loop.  So, it seems, alternatively,
> > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > sleep to the user space interface for the "disable" case.
> 
> Good point, that would work just as well and be simpler.

Thanks for the confirmation! :-)

By the way, I wonder, would it help to add synchronize_rcu() to
wakeup_source_add() too?  Then, even if device_wakeup_enable() and
device_wakeup_disable() are executed in a tight loop for the same
device, the list_add/list_del operations will always happen in
different RCU cycles (or at least it seems so).

Thanks,
Rafael

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> [...] 
> > > Yes, you can, but then I'd say it's not necessary for user space to
> > > be able to carry that out in a tight loop.  So, it seems, alternatively,
> > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > sleep to the user space interface for the "disable" case.
> > 
> > Good point, that would work just as well and be simpler.
> 
> Thanks for the confirmation! :-)
> 
> By the way, I wonder, would it help to add synchronize_rcu() to
> wakeup_source_add() too?  Then, even if device_wakeup_enable() and
> device_wakeup_disable() are executed in a tight loop for the same
> device, the list_add/list_del operations will always happen in
> different RCU cycles (or at least it seems so).

I cannot immediately see how adding a synchronize_rcu() to
wakeup_source_add() would help anything.  You only need to wait for a
grace period on removal, not (normally) on addition.  The single grace
period during removal will catch up all other asynchronous RCU grace
period requests on that CPU.

Or am I missing your point?

							Thanx, Paul

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Rafael J. Wysocki]

On Friday, January 20, 2012, Paul E. McKenney wrote:
> On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > [...] 
> > > > Yes, you can, but then I'd say it's not necessary for user space to
> > > > be able to carry that out in a tight loop.  So, it seems, alternatively,
> > > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > > sleep to the user space interface for the "disable" case.
> > > 
> > > Good point, that would work just as well and be simpler.
> > 
> > Thanks for the confirmation! :-)
> > 
> > By the way, I wonder, would it help to add synchronize_rcu() to
> > wakeup_source_add() too?  Then, even if device_wakeup_enable() and
> > device_wakeup_disable() are executed in a tight loop for the same
> > device, the list_add/list_del operations will always happen in
> > different RCU cycles (or at least it seems so).
> 
> I cannot immediately see how adding a synchronize_rcu() to
> wakeup_source_add() would help anything.  You only need to wait for a
> grace period on removal, not (normally) on addition.  The single grace
> period during removal will catch up all other asynchronous RCU grace
> period requests on that CPU.
> 
> Or am I missing your point?

Well, I was thinking about the failure scenario you mentioned where
executing enable/disable in a tight loop might exhaust system memory
(if I understood it correctly).

Thanks,
Rafael

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Paul E. McKenney]

On Sat, Jan 21, 2012 at 12:49:35AM +0100, Rafael J. Wysocki wrote:
> On Friday, January 20, 2012, Paul E. McKenney wrote:
> > On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > > > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > > [...] 
> > > > > Yes, you can, but then I'd say it's not necessary for user space to
> > > > > be able to carry that out in a tight loop.  So, it seems, alternatively,
> > > > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > > > sleep to the user space interface for the "disable" case.
> > > > 
> > > > Good point, that would work just as well and be simpler.
> > > 
> > > Thanks for the confirmation! :-)
> > > 
> > > By the way, I wonder, would it help to add synchronize_rcu() to
> > > wakeup_source_add() too?  Then, even if device_wakeup_enable() and
> > > device_wakeup_disable() are executed in a tight loop for the same
> > > device, the list_add/list_del operations will always happen in
> > > different RCU cycles (or at least it seems so).
> > 
> > I cannot immediately see how adding a synchronize_rcu() to
> > wakeup_source_add() would help anything.  You only need to wait for a
> > grace period on removal, not (normally) on addition.  The single grace
> > period during removal will catch up all other asynchronous RCU grace
> > period requests on that CPU.
> > 
> > Or am I missing your point?
> 
> Well, I was thinking about the failure scenario you mentioned where
> executing enable/disable in a tight loop might exhaust system memory
> (if I understood it correctly).

Ah, got it.  If they are executing this in a tight loop, there will be
little difference between doing one synchronize_rcu() per pass through
the loop or doing two.  So we should be just fine with the single instance
of synchronize_rcu() per loop.

							Thanx, Paul

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Rafael J. Wysocki]

On Monday, January 23, 2012, Paul E. McKenney wrote:
> On Sat, Jan 21, 2012 at 12:49:35AM +0100, Rafael J. Wysocki wrote:
> > On Friday, January 20, 2012, Paul E. McKenney wrote:
> > > On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > > > > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > > > [...] 
> > > > > > Yes, you can, but then I'd say it's not necessary for user space to
> > > > > > be able to carry that out in a tight loop.  So, it seems, alternatively,
> > > > > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > > > > sleep to the user space interface for the "disable" case.
> > > > > 
> > > > > Good point, that would work just as well and be simpler.
> > > > 
> > > > Thanks for the confirmation! :-)
> > > > 
> > > > By the way, I wonder, would it help to add synchronize_rcu() to
> > > > wakeup_source_add() too?  Then, even if device_wakeup_enable() and
> > > > device_wakeup_disable() are executed in a tight loop for the same
> > > > device, the list_add/list_del operations will always happen in
> > > > different RCU cycles (or at least it seems so).
> > > 
> > > I cannot immediately see how adding a synchronize_rcu() to
> > > wakeup_source_add() would help anything.  You only need to wait for a
> > > grace period on removal, not (normally) on addition.  The single grace
> > > period during removal will catch up all other asynchronous RCU grace
> > > period requests on that CPU.
> > > 
> > > Or am I missing your point?
> > 
> > Well, I was thinking about the failure scenario you mentioned where
> > executing enable/disable in a tight loop might exhaust system memory
> > (if I understood it correctly).
> 
> Ah, got it.  If they are executing this in a tight loop, there will be
> little difference between doing one synchronize_rcu() per pass through
> the loop or doing two.  So we should be just fine with the single instance
> of synchronize_rcu() per loop.

Good! :-)

Thanks a lot,
Rafael

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Alan Cox]

> Let me provide a bit of context. The serial_core code seems to be the
> only place in the kernel that does this:
> 
> 		device_init_wakeup(tty_dev, 1);
> 		device_set_wakeup_enable(tty_dev, 0);
> 
> The first call makes the device wakeup capable and enables wakeup, The
> second call disabled wakeup.

> I assume we can't and shouldn't change device_init_wakeup() . We could
> add a call like device_init_wakeup_disabled() which makes the device
> wakeup capable but does not actually enable it. Does that work?

Is that not

	device_init_wakeup(tty_dev, 0)

or am I missing something ?

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Hi Alan,

On Wed, Jan 18, 2012 at 2:12 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Let me provide a bit of context. The serial_core code seems to be the
>> only place in the kernel that does this:
>>
>>               device_init_wakeup(tty_dev, 1);
>>               device_set_wakeup_enable(tty_dev, 0);
>>
>> The first call makes the device wakeup capable and enables wakeup, The
>> second call disabled wakeup.
>
>> I assume we can't and shouldn't change device_init_wakeup() . We could
>> add a call like device_init_wakeup_disabled() which makes the device
>> wakeup capable but does not actually enable it. Does that work?
>
> Is that not
>
>        device_init_wakeup(tty_dev, 0)
>
> or am I missing something ?

Bearing in mind that I know very little about this, I think
serial_core wants to have the device wakeup capable in any case, but
not actually enable wakeup on the device until requested. It seems
that drivers rely on that behaviour too.

device_init_wakeup(tty_dev, 0) calls device_set_wakeup_capable(dev,
false) which I think is not what we want.

Regards,
Simon

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Rafael J. Wysocki]

On Wednesday, January 18, 2012, Simon Glass wrote:
> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
> 
> Hi Alan, Paul,
> 
> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
> >> On Tue, 17 Jan 2012 10:56:03 -0800
> >> Simon Glass <sjg@chromium.org> wrote:
> >>
> >> > Since serial_core now does not make serial ports wake-up capable by
> >> > default, add a parameter to support this feature in the 8250 UART.
> >> > This is the only UART where I think this feature is useful.
> >>
> >> NAK
> >>
> >> Things should just work for users. Magic parameters is not an
> >> improvement. If its a performance problem someone needs to fix the rcu
> >> sync overhead or stop using rcu on that path.
> 
> OK fair enough, I agree. Every level I move down the source tree
> affects more people though.
> 
> >
> > I must say that I lack context here, even after looking at the patch,
> > but the synchronize_rcu_expedited() primitives can be used if the latency
> > of synchronize_rcu() is too large.
> >
> 
> Let me provide a bit of context. The serial_core code seems to be the
> only place in the kernel that does this:
> 
> 		device_init_wakeup(tty_dev, 1);
> 		device_set_wakeup_enable(tty_dev, 0);

It shouldn't do that.

It should just do device_set_wakeup_capable(tty_dev, true) instead.

Thanks,
Rafael

Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param

[Simon Glass]

Hi Rafael,

On Wed, Jan 18, 2012 at 4:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, January 18, 2012, Simon Glass wrote:
>> [+cc Rafael J. Wysocki <rjw@sisk.pl> who I think wrote the wakeup.c code]
>>
>> Hi Alan, Paul,
>>
>> On Tue, Jan 17, 2012 at 8:17 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Tue, Jan 17, 2012 at 08:10:36PM +0000, Alan Cox wrote:
>> >> On Tue, 17 Jan 2012 10:56:03 -0800
>> >> Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >> > Since serial_core now does not make serial ports wake-up capable by
>> >> > default, add a parameter to support this feature in the 8250 UART.
>> >> > This is the only UART where I think this feature is useful.
>> >>
>> >> NAK
>> >>
>> >> Things should just work for users. Magic parameters is not an
>> >> improvement. If its a performance problem someone needs to fix the rcu
>> >> sync overhead or stop using rcu on that path.
>>
>> OK fair enough, I agree. Every level I move down the source tree
>> affects more people though.
>>
>> >
>> > I must say that I lack context here, even after looking at the patch,
>> > but the synchronize_rcu_expedited() primitives can be used if the latency
>> > of synchronize_rcu() is too large.
>> >
>>
>> Let me provide a bit of context. The serial_core code seems to be the
>> only place in the kernel that does this:
>>
>>               device_init_wakeup(tty_dev, 1);
>>               device_set_wakeup_enable(tty_dev, 0);
>
> It shouldn't do that.
>
> It should just do device_set_wakeup_capable(tty_dev, true) instead.

Ok, that makes sense - thank you for clearing it up. I will do a new patch.

Regards
Simon

>
> Thanks,
> Rafael

[PATCH 1/2] power: Add function to init wakeup capability without enabling

[Simon Glass]

device_init_wakeup() offers two options:

1. Wakeup capable and wakeup enabled
2  Not wakeup capable and not wakeup enabled

serial_core wants a third option:

3. Wakeup capable but not wakeup enabled (yet)

Add a new init routine which takes a flag indicating which of the three
options is required. This avoids creating and then destroying the wakeup
source to no purpose, and a sometimes large rcu_synchronise() delay.

Q: What is a good name for the new function?

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/base/power/wakeup.c |   26 ++++++++++++++++++++++----
 include/linux/pm_wakeup.h   |   22 +++++++++++++++++++---
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index caf995f..ae3b3c5 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -273,7 +273,7 @@ EXPORT_SYMBOL_GPL(device_set_wakeup_capable);
 /**
  * device_init_wakeup - Device wakeup initialization.
  * @dev: Device to handle.
- * @enable: Whether or not to enable @dev as a wakeup device.
+ * @flags: flags PM_WAKEUP_...
  *
  * By default, most devices should leave wakeup disabled.  The exceptions are
  * devices that everyone expects to be wakeup sources: keyboards, power buttons,
@@ -281,19 +281,37 @@ EXPORT_SYMBOL_GPL(device_set_wakeup_capable);
  * own wakeup requests but merely forward requests from one bus to another
  * (like PCI bridges) should have wakeup enabled by default.
  */
-int device_init_wakeup(struct device *dev, bool enable)
+int device_init_wakeup_flag(struct device *dev, int flags)
 {
 	int ret = 0;
 
-	if (enable) {
+	if (flags & PM_WAKEUP_CAP) {
 		device_set_wakeup_capable(dev, true);
-		ret = device_wakeup_enable(dev);
+		if (flags & PM_WAKEUP_EN)
+			ret = device_wakeup_enable(dev);
 	} else {
+		WARN_ON(flags & PM_WAKEUP_EN);
 		device_set_wakeup_capable(dev, false);
 	}
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(device_init_wakeup_flag);
+
+/**
+ * device_init_wakeup - Device wakeup initialization.
+ * @dev: Device to handle.
+ * @enable: Whether or not to enable @dev as a wakeup device.
+ *
+ * By default, most devices should leave wakeup disabled.  The exceptions are
+ * devices that everyone expects to be wakeup sources: keyboards, power buttons,
+ * possibly network interfaces, etc.
+ */
+int device_init_wakeup(struct device *dev, bool enable)
+{
+	return device_init_wakeup_flag(dev, enable ? PM_WAKEUP_CAP_EN :
+				       PM_WAKEUP_NONE);
+}
 EXPORT_SYMBOL_GPL(device_init_wakeup);
 
 /**
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index a32da96..3035875 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -56,6 +56,14 @@ struct wakeup_source {
 	unsigned int		active:1;
 };
 
+/* Flags for device_init_wakeup_flag() */
+enum {
+	PM_WAKEUP_NONE,		/* Not wakeup capable */
+	PM_WAKEUP_CAP,		/* Wakeup capable but not enabled */
+	PM_WAKEUP_EN,		/* Don't use */
+	PM_WAKEUP_CAP_EN,	/* Wakeup capable and enabled */
+};
+
 #ifdef CONFIG_PM_SLEEP
 
 /*
@@ -83,6 +91,7 @@ extern int device_wakeup_enable(struct device *dev);
 extern int device_wakeup_disable(struct device *dev);
 extern void device_set_wakeup_capable(struct device *dev, bool capable);
 extern int device_init_wakeup(struct device *dev, bool val);
+extern int device_init_wakeup_flag(struct device *dev, int flags);
 extern int device_set_wakeup_enable(struct device *dev, bool enable);
 extern void __pm_stay_awake(struct wakeup_source *ws);
 extern void pm_stay_awake(struct device *dev);
@@ -139,13 +148,20 @@ static inline int device_set_wakeup_enable(struct device *dev, bool enable)
 	return 0;
 }
 
-static inline int device_init_wakeup(struct device *dev, bool val)
+static inline device_init_wakeup_flag(struct device *dev, int flags)
 {
-	device_set_wakeup_capable(dev, val);
-	device_set_wakeup_enable(dev, val);
+	device_set_wakeup_capable(dev, flags & PM_WAKEUP_CAP);
+	if (flags & PM_WAKEUP_EN)
+		device_set_wakeup_enable(dev, true);
 	return 0;
 }
 
+static inline int device_init_wakeup(struct device *dev, bool enable)
+{
+	return device_init_wakeup_flag(enable ? PM_WAKEUP_CAP_EN :
+				       PM_WAKEUP_NONE);
+}
+
 static inline bool device_may_wakeup(struct device *dev)
 {
 	return dev->power.can_wakeup && dev->power.should_wakeup;
-- 
1.7.7.3

[PATCH 2/2] serial: Use device_init_wakeup_flag() to make device wakeup-capable

[Simon Glass]

We want the serial device to be wakeup-capable but not actually enable
the wakeup function. The original code results in making the serial
device wakeup-capable, then enabling and then immediately disabling
the actual wakeup feature on the device. This ends up in a
synchronise_rcu() in wakeup_source_remove() which can take some 15ms
to run, yet is entirely avoidable. The cost over 4 ports is about 600ms
on my Tegra2x system (with CONFIG_PREEMPT disabled).

Using the new function avoids this problem.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 drivers/tty/serial/serial_core.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c7bf31a..a8b01db 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2348,11 +2348,11 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 */
 	tty_dev = tty_register_device(drv->tty_driver, uport->line, uport->dev);
 	if (likely(!IS_ERR(tty_dev))) {
-		device_init_wakeup(tty_dev, 1);
-		device_set_wakeup_enable(tty_dev, 0);
-	} else
+		device_init_wakeup_flag(tty_dev, PM_WAKEUP_CAP);
+	} else {
 		printk(KERN_ERR "Cannot register tty device on line %d\n",
 		       uport->line);
+	}
 
 	/*
 	 * Ensure UPF_DEAD is not set.
-- 
1.7.7.3

Re: [PATCH 1/2] power: Add function to init wakeup capability without enabling

[Rafael J. Wysocki]

On Thursday, January 19, 2012, Simon Glass wrote:
> device_init_wakeup() offers two options:
> 
> 1. Wakeup capable and wakeup enabled
> 2  Not wakeup capable and not wakeup enabled
> 
> serial_core wants a third option:
> 
> 3. Wakeup capable but not wakeup enabled (yet)

Which is device_set_wakeup_capable(dev, true).

Thanks,
Rafael