Messages in this thread | | | From | Sergey Senozhatsky <> | Date | Fri, 14 Sep 2018 20:50:28 +0900 | Subject | Re: [PATCH] printk: inject caller information into the body of message |
| |
On (09/14/18 19:37), Tetsuo Handa wrote: > > @@ -20,6 +20,9 @@ > > * Annotation for a "continued" line of log printout (only done after a > > * line that had no enclosing \n). Only to be used by core/arch code > > * during early bootup (a continued line is not SMP-safe otherwise). > > + * > > + * Please consider pr_line()/vpr_line() functions for SMP-safe continued > > + * line printing. > > I think the advantage is not limited to SMP-safeness. Reducing the frequency of > calling printk() will reduce overhead. Also, latency for netconsole will be > reduced by sending a whole line in one printk().
Hmm. These are very good points, indeed. But do we want to list all advantages here? I just wanted to mention SMP-unsafe pr_cont/printk(KERN_CONT), because I also mention pr_line in kern_levels.h.
> > + * Defines a new pr_line varialbe, which would use an implicit > > s/varialbe/variable/ .
Thanks.
> > +#define DEFINE_PR_LINE(lev, name) \ > > + char __line_##name[__PR_LINE_BUF_SZ]; \ > > + struct pr_line name = { \ > > + .sb = __SEQ_BUF_INITIALIZER(__line_##name, \ > > + __PR_LINE_BUF_SZ), \ > > + .level = lev, \ > > + } > > Want a note that > > static DEFINE_PR_LINE(lev, name); > > won't make "name" variable "static" ?
Interesting point. Any hint what the comment should look like? Do we want to have static pr_line buffers?
> > +#define DEFINE_PR_LINE_BUF(lev, name, buf, sz) \ > > + struct pr_line name = { \ > > + .sb = __SEQ_BUF_INITIALIZER(buf, (sz)), \ > > + .level = lev, \ > > + } > > + > > I would use this one for the OOM killer. 80 bytes is too short.
80 bytes is quite short for OOM, agreed.
> static char oom_print_buf[1024]; > DEFINE_PR_LINE_BUF(level, oom_print_buf);
Do I get it right that you suggest to drop the "size" param? Do OOM people agree on 1024 bytes stack usage?
> > @@ -131,4 +187,8 @@ extern int > > seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary); > > #endif > > > > +extern __printf(2, 0) > > +int vpr_line(struct pr_line *pl, const char *fmt, va_list args); > > +extern __printf(2, 3) > > +int pr_line(struct pr_line *pl, const char *fmt, ...); > > Do we want to mark "asmlinkage" like printk() ?
Dunno, do we? Does code written in assembly call pr_cont that often? We are not turning pr_line() into syscall anyway.
> > @@ -324,3 +324,60 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt) > > s->readpos += cnt; > > return cnt; > > } > > + > > +/** > > + * vpr_line - Append data to the printk() line buffer > > + * @pl: the pr_line descriptor > > s/descriptor/structure/ ?
Yeah, I used the term "descriptor", just because it's used in seq_buf.c. So, it's sort of common in seq_buf. E.g. seq_buf_vprintf(), seq_buf_print_seq(), seq_buf_can_fit() and so on.
> > + * @fmt: printf format string > > + * @args: va_list of arguments from a printf() type function > > + * > > + * Writes a vnprintf() format into the printk() pr_line buffer. > > s/vnprintf/vprintf/ ?
Indeed. We also need to fix a typo in seq_buf_vprintf() comment then.
-ss
| |