From: Anton Vorontsov <anton.vorontsov@linaro.org>
To: "Luck, Tony" <tony.luck@intel.com>
Cc: "Liu, Chuansheng" <chuansheng.liu@intel.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"keescook@chromium.org" <keescook@chromium.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Colin Cross <ccross@android.com>
Subject: Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case
Date: Wed, 26 Sep 2012 16:35:56 -0700 [thread overview]
Message-ID: <20120926233553.GA7203@lizard> (raw)
In-Reply-To: <3908561D78D1C84285E8C5FCA982C28F19D4404F@ORSMSX108.amr.corp.intel.com>
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.
prev parent reply other threads:[~2012-09-26 23:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-17 17:43 [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case Chuansheng Liu
2012-09-20 22:57 ` Anton Vorontsov
2012-09-20 23:09 ` Luck, Tony
2012-09-20 23:25 ` Anton Vorontsov
2012-09-20 23:48 ` Luck, Tony
2012-09-21 0:37 ` Anton Vorontsov
2012-09-24 15:02 ` Luck, Tony
2012-09-26 23:35 ` Anton Vorontsov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120926233553.GA7203@lizard \
--to=anton.vorontsov@linaro.org \
--cc=ccross@android.com \
--cc=chuansheng.liu@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).