[PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Chuansheng Liu]

Like 8250 driver, when pstore is registered as a console,
to avoid recursive spinlocks when panic happening, change the
spin_lock_irqsave to spin_trylock_irqsave when oops_in_progress
is true.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 fs/pstore/platform.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 29996e8..cbcb5fc 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -164,7 +164,12 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
 
 		if (c > psinfo->bufsize)
 			c = psinfo->bufsize;
-		spin_lock_irqsave(&psinfo->buf_lock, flags);
+
+		if (oops_in_progress) {
+			if (!spin_trylock_irqsave(&psinfo->buf_lock, flags))
+				break;
+		} else
+			spin_lock_irqsave(&psinfo->buf_lock, flags);
 		memcpy(psinfo->buf, s, c);
 		psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
 		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
-- 
1.7.0.4

Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Anton Vorontsov]

On Tue, Sep 18, 2012 at 01:43:44AM +0800, Chuansheng Liu wrote:
> Like 8250 driver, when pstore is registered as a console,
> to avoid recursive spinlocks when panic happening, change the
> spin_lock_irqsave to spin_trylock_irqsave when oops_in_progress
> is true.
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  fs/pstore/platform.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 29996e8..cbcb5fc 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -164,7 +164,12 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c)
>  
>  		if (c > psinfo->bufsize)
>  			c = psinfo->bufsize;
> -		spin_lock_irqsave(&psinfo->buf_lock, flags);
> +
> +		if (oops_in_progress) {
> +			if (!spin_trylock_irqsave(&psinfo->buf_lock, flags))
> +				break;

Mm... why break?

Like 8250, in this case it's better still log the console.
That is, something along these lines:

bool locked = 1;

if (oops_in_progress)
	locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
else
	spin_lock_irqsave(&psinfo->buf_lock, flags);

memcpy(psinfo->buf, s, c);
psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);

if (locked)
	spin_unlock_irqrestore(&psinfo->buf_lock, flags);


Thanks,

Anton.

> +		} else
> +			spin_lock_irqsave(&psinfo->buf_lock, flags);
>  		memcpy(psinfo->buf, s, c);
>  		psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
>  		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> -- 
> 1.7.0.4

RE: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 730 bytes --]

> Mm... why break?

We don't know what the back-end driver will do if we allow another call
while a previous one is still in progress.  It might end up corrupting the
backing non-volatile storage and losing some previously saved records.

Existing drivers (ERST and EFI) are dependent on f/w ... so things might
work on some platforms, yet be horribly bad on others.

The patch as it was written converts a deadlock (hang) case into a "lose
this log, but keep going" case. Which seems to be an improvement without
taking any risks about what the backend will do.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Anton Vorontsov]

On Thu, Sep 20, 2012 at 11:09:36PM +0000, Luck, Tony wrote:
> > Mm... why break?
> 
> We don't know what the back-end driver will do if we allow another call
> while a previous one is still in progress.  It might end up corrupting the
> backing non-volatile storage and losing some previously saved records.

True, but the lock is used to protect pstore->buf, I doubt that
any backend will actually want to grab it, no?

Since it is pstore that is handing the buffer to backend, it is
pstore's worry to do proper locking.

> Existing drivers (ERST and EFI) are dependent on f/w ... so things might
> work on some platforms, yet be horribly bad on others.
> 
> The patch as it was written converts a deadlock (hang) case into a "lose
> this log, but keep going" case. Which seems to be an improvement without
> taking any risks about what the backend will do.

But why backends should (or want/will want to) grab this lock?..

If a backend needs its own locking in ->write callback, then it'll
have to use its own lock, I guess.

Thanks,
Anton.

RE: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 794 bytes --]

> True, but the lock is used to protect pstore->buf, I doubt that
> any backend will actually want to grab it, no?

The lock is doing double duty to protect the buffer, and the back-end driver.

But even if we split it into two (one for the buffer, taken by pstore, and one
internal to the backend to protect interaction with the f/w). Ifwe ignore the
fact that we can't get the lock that protects  the buffer means it is very likely
that we corrupt the previous record that was being written by clobbering the
buffer with the data for this new record.

I'd prefer to maximize the chances that the earlier record gets written.

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Anton Vorontsov]

On Thu, Sep 20, 2012 at 11:48:32PM +0000, Luck, Tony wrote:
> > True, but the lock is used to protect pstore->buf, I doubt that
> > any backend will actually want to grab it, no?
> 
> The lock is doing double duty to protect the buffer, and the back-end driver.
> 
> But even if we split it into two (one for the buffer, taken by pstore, and one
> internal to the backend to protect interaction with the f/w). Ifwe ignore the
> fact that we can't get the lock that protects  the buffer means it is very likely
> that we corrupt the previous record that was being written by clobbering the
> buffer with the data for this new record.
> 
> I'd prefer to maximize the chances that the earlier record gets written.

Sure, I applied the original patch.

Btw, do you expect that backends protect themselves from concurrent
->write calls, or pstore guarantees to protect backends?

Because the latter is not always possible, for example in tracing: we
won't able to grab locks at all (but not all backends can do tracing
anyway -- they must do things atomically).

Plus, sometimes having the global lock is not "efficient", backends
know better: they might have separate locks per message type.

And my plan was to get rid of the fact that backends touch pstore->buf
directly. Backends would always receive anonymous 'buf' pointer (we
already have write_buf callback that does exactly this), and thus it
would be backends' worry to protect against concurrency. In this
scheme, pstore's console code won't need to grab locks at all: we'll
just pass console string to the backend directly.

And backends, if they can't do writes atomically, will grab their
own locks.

Thanks,
Anton.

RE: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 767 bytes --]

> And my plan was to get rid of the fact that backends touch pstore->buf
> directly. Backends would always receive anonymous 'buf' pointer (we
> already have write_buf callback that does exactly this), and thus it

It feels like we are just shuffling the lock problem from one place
to another.  In the panic case we have to use a pre-allocated buffer
(hoping that we can allocate one seems to be a foolish plan). So
we'd need a lock around use of that buffer somewhere - whether
it is in the panic code, the pstore generic code, or the back-end
driver.

Can you describe where you'd like to end up?

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case

[Anton Vorontsov]

On Mon, Sep 24, 2012 at 03:02:35PM +0000, Luck, Tony wrote:
> > And my plan was to get rid of the fact that backends touch pstore->buf
> > directly. Backends would always receive anonymous 'buf' pointer (we
> > already have write_buf callback that does exactly this), and thus it
> 
> It feels like we are just shuffling the lock problem from one place
> to another.  In the panic case we have to use a pre-allocated buffer
> (hoping that we can allocate one seems to be a foolish plan).

Yes, we definitely need some buffer pre-allocated for panics, so I have no
plans to get rid of the 'buf' -- both 'buf' and 'buf_lock' are going to
stay. But it will not be multi-purpose anymore (which I see as an
improvement).

The thing is, what I dislike is the whole console_write routine:

static void pstore_console_write(struct console *con, const char *s, unsigned c)
{
	const char *e = s + c;

	while (s < e) {
		unsigned long flags;

		if (c > psinfo->bufsize)
			c = psinfo->bufsize;
		spin_lock_irqsave(&psinfo->buf_lock, flags);
		memcpy(psinfo->buf, s, c);
		psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo);
		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
		s += c;
		c = e - s;
	}
}

It's bad not because of the locks, but because we unnecessary copy things
around -- and that's the only reason why we need the lock in the first
place.

With write_buf, the above would turn into just:

static void pstore_console_write(struct console *con, const char *s, unsigned c)
{
	psinfo->write_buf(PSTORE_TYPE_CONSOLE, 0, NULL, 0, s, c, psinfo);
}

Of course, this assumes that write_buf does its own hw/driver-specific
protection, but only if necessary: for ram backend no locks would be
necessary at all.

And as it appears, both erst and efivars drivers do their own locks in the
->write callback anyways. (Btw, efi pstore backend just grabs its lock w/o
trying it first, is it in trouble?)


But for panic case, we will use buf and buf_lock:

pstore_dump()
{
	lock(buf_lock);
	...
	psinfo->write_buf(PSTORE_TYPE_DMESG, ..., psinfo->buf, ...);
	...
	unlock(buf_lock);
}

So, we will use them, but only when necessary (for dumping). We can think
of them as dump_buf and dump_buf_lock, because that's the only when this
stuff will be needed, actually.

Thanks,
Anton.