LKML Archive on lore.kernel.org
 help / color / Atom feed
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
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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-08 13:04 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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git