From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
linux-kernel@vger.kernel.org, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH v2] printk: Relocate wake_klogd check close to the end of console_unlock()
Date: Sat, 10 Feb 2018 16:33:33 +0900 [thread overview]
Message-ID: <20180210073333.GC485@tigerII.localdomain> (raw)
In-Reply-To: <20180209103911.mljxpgkw6njyoibe@pathway.suse.cz>
Hello,
On (02/09/18 11:39), Petr Mladek wrote:
> On Fri 2018-02-09 12:28:30, Sergey Senozhatsky wrote:
> > On (02/08/18 17:48), Petr Mladek wrote:
> > By postponing klogd wakeup we don't really address logbuf_lock
> > contention. We have no guarantees that no new printk will come
> > while klogd is active. Besides, consoles don't really delay
> > klogd - I tend to ignore the impact of msg_print_text(), it should
> > be fast. We call console drivers outside of logbuf_lock scope, so
> > everything should fine (tm).
>
> First, I am all for waking klogd earlier.
>
> Second, sigh, I do not have much experience with kernel performace issues.
> It is very likely that we really do not need to mind about it.
I really don't think we need to bother.
klogd loggers are as important as consoles, we need to run them anyway.
> > Basically,
> > - if consoles are suspended, we also "suspend" user space klogd.
> > Does it really make sense?
> >
> > - if console_lock is acquired by a preemptible task and that task
> > is getting scheduled out for a long time (OOM, etc) then we postpone
> > user space logging for unknown period of time. First the console_lock
> > will have to flush pending messages and only afterwards it will wakeup
> > klogd. Does it really make sense?
> >
> > - If current console_lock owner jumps to retry (new pending messages
> > were appended to the logbuf) label, user space klogd wakeup is getting
> > postponed even further.
> >
> > So, the final question is - since there in only one legitimate way
> > (modulo user space writes to kmsg) to append new messages to the
> > logbuf, shall we put klogd wakeup there? IOW, to vprintk_emit().
>
> Good points! In fact, if we wakeup klogd before calling consoles,
> we would need to do it for every new message. Otherwise, klogd
> processes might miss new messages that are added in parallel
> when console_lock is taken.
[..]
> Then klogd might start sleeping while new messages are still comming
Loggers sleep when `user->seq == log_next_seq' or when
`syslog_seq == log_next_seq'. We need to just wakeup them.
Once woken up they will check the condition again, if there
are no new messages, they will schedule.
> Now, your proposed solution looked fine. Just note that we do not need
> seen_seq. vprintk_emit() knows when log_next_seq is increased.
> It would always wake klogd in this case.
Probably can wake_up_klogd() unconditionally.
> Anyway, I think about slightly different way that would prevent
> scheduling klogd intead of the vprintk_emit() caller. The trick
> is to do the wakeup with preemtion disabled. I mean something like:
[..]
> @@ -1899,9 +1899,13 @@ asmlinkage int vprintk_emit(int facility, int level,
> */
> preempt_disable();
> /*
> + * Wake klogd with disabled preemtion so that they can run
> + * in parallel but they could not delay console_handling.
> + */
> + wake_up_klogd();
> + /*
> * Try to acquire and then immediately release the console
> - * semaphore. The release will print out buffers and wake up
> - * /dev/kmsg and syslog() users.
> + * semaphore. The release will print out buffers.
> */
> if (console_trylock_spinning())
> console_unlock();
[..]
It doesn't wakeup loggers for printk_deferred() output this way.
I want to wakeup them for every new logbuf message.
If logger preempts current printk() that it's for 1 message only.
printk() will return back and finish printing. If there will be another
printk()-s from other CPUs, then one of those CPUs will print messages
to the consoles - CPU that woke up logger had not acquired console_sem
before it was preempted by logger.
-ss
next prev parent reply other threads:[~2018-02-10 7:33 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-08 13:04 [PATCH v2] printk: Relocate wake_klogd check close to the end of console_unlock() Petr Mladek
2018-02-08 14:53 ` Sergey Senozhatsky
2018-02-08 16:48 ` Petr Mladek
2018-02-09 3:28 ` Sergey Senozhatsky
2018-02-09 10:39 ` Petr Mladek
2018-02-10 7:33 ` Sergey Senozhatsky [this message]
2018-02-09 10:47 ` Petr Mladek
2018-02-19 15:58 ` Petr Mladek
2018-02-19 16:01 ` [PATCH v3] " Petr Mladek
2018-02-26 6:37 ` Sergey Senozhatsky
2018-02-26 15:57 ` Petr Mladek
2018-02-26 16:01 ` Sergey Senozhatsky
2018-02-26 6:27 ` [PATCH v2] " Sergey Senozhatsky
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=20180210073333.GC485@tigerII.localdomain \
--to=sergey.senozhatsky@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=tj@kernel.org \
/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).