All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Dmitriy Vyukov <dvyukov@google.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Alexander Potapenko <glider@google.com>,
	Fengguang Wu <fengguang.wu@intel.com>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v5] printk: Add line-buffered printk() API.
Date: Fri, 2 Nov 2018 22:31:35 +0900	[thread overview]
Message-ID: <c1f21a72-6eb3-8e16-06fe-262cb427f100@i-love.sakura.ne.jp> (raw)
In-Reply-To: <20181101141342.t65mqwxdpqs3mu5i@pathway.suse.cz>

On 2018/11/01 23:13, Petr Mladek wrote:
> On Wed 2018-10-24 19:11:10, Tetsuo Handa wrote:
>> After this patch, we will consider how we can add context identifier to
>> each line of printk() output (so that we can group multiple lines into
>> one block when parsing). Therefore, if converting to this API does not
>> fit for some reason, you might be able to consider converting to multiple
>> printk() calls which end with '\n'.
> 
> The buffered printk API is for continuous lines. It is more
> complicated than a simple printk. You need to get, use, and put
> the buffer. It might be acceptable for continuous lines that
> should be rare and the related calls typically located in a single
> function.
> 
> I prefer to solve the related lines on another level, for example,
> by storing/showing PID+context_mask for each printed line. This
> way it would work transparently even for normal printk().

Yes. I'm expecting that identifier part is added by printk()
rather than by this line buffering API.



>> +	/*
>> +	 * Skip KERN_CONT here based on an assumption that KERN_CONT will be
>> +	 * given via "fmt" argument when KERN_CONT is given.
>> +	 */
>> +	fmt_offset = (printk_get_level(fmt) == 'c') ? 2 : 0;
>> + retry:
>> +	va_copy(tmp_args, args);
>> +	r = vsnprintf(ptr->buf + ptr->used, sizeof(ptr->buf) - ptr->used,
>> +		      fmt + fmt_offset, tmp_args);
>> +	va_end(tmp_args);
>> +	if (r + ptr->used < sizeof(ptr->buf)) {
>> +		ptr->used += r;
>> +		/* Flush already completed lines if any. */
>> +		for (r = ptr->used - 1; r >= 0; r--) {
>> +			if (ptr->buf[r] != '\n')
>> +				continue;
> 
> I thought about using strrchr(). But this is more effective
> because we know the length of the string. It might deserve
> a comment though.

At first, I tried to use memrchr().

>> +			break;
>> +		}
>> +		return r;
> 
> We need to use another variable in the for-cycle. Otherwise, we would
> not return the number of printed characters here.

But since memrchr() is not available, I forgot to introduce another
variable when rewriting this loop...



>> +bool flush_printk_buffer(struct printk_buffer *ptr)
>> +{
>> +	if (!ptr || !ptr->used)
>> +		return false;
>> +	/* vprintk_buffered() keeps 0 <= ptr->used < sizeof(ptr->buf) true. */
>> +	ptr->buf[ptr->used] = '\0';
> 
> We do not need this when there is always the trailing '\0' in
> non-empty buffer. It looks more sane to me.

We need this when called from vprintk_buffered(), for vsnprintf()
has overwritten ptr->buf[ptr->used] with non-'\0' byte.

>> +	printk("%s", ptr->buf);
>> +	ptr->used = 0;
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(flush_printk_buffer);



> We are getting close. Please, split the debugging stuff into separate
> patch.

Too many comments on debugging stuff. I will for now drop debugging stuff
 from next version. We can add debugging stuff after core patch is merged.

> 
> Also it would be great to do add a sample conversion from pr_cont() to
> this API in another separate patch.

OK. I will add two example users in next version.


      reply	other threads:[~2018-11-02 13:31 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-24 10:11 Tetsuo Handa
2018-11-01 14:13 ` Petr Mladek
2018-11-02 13:31   ` Tetsuo Handa [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=c1f21a72-6eb3-8e16-06fe-262cb427f100@i-love.sakura.ne.jp \
    --to=penguin-kernel@i-love.sakura.ne.jp \
    --cc=akpm@linux-foundation.org \
    --cc=dvyukov@google.com \
    --cc=fengguang.wu@intel.com \
    --cc=glider@google.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH v5] printk: Add line-buffered printk() API.' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.