Messages in this thread | | | Date | Fri, 6 Jan 2023 11:39:15 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk v4 7/8 v2] printk: use printk_buffers for devkmsg |
| |
On Thu 2023-01-05 13:22:16, John Ogness wrote: > Replace the buffers in struct devkmsg_user with a struct > printk_buffers. This reduces the number of buffers to track. > > As a side-effect, @text_buf was 8kB large, even though it only > needed to be the max size of a ringbuffer record. By switching to > struct printk_buffers, ~7kB less memory is allocated when opening > /dev/kmsg. > > And since struct printk_buffers will be used now, reduce duplicate > code by calling printk_get_next_message() to handle the record > reading and formatting.
> Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > kernel/printk/printk.c | 40 ++++++++++++++++------------------------ > 1 file changed, 16 insertions(+), 24 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 4fb7d29fb05d..32996b0c5d03 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -695,16 +695,14 @@ static ssize_t msg_print_ext_body(char *buf, size_t size, > return len; > } > > +static bool printk_get_next_message(struct printk_message *pmsg, u64 seq, bool is_extended); > + > /* /dev/kmsg - userspace message inject/listen interface */ > struct devkmsg_user { > atomic64_t seq; > struct ratelimit_state rs; > struct mutex lock; > - char buf[CONSOLE_EXT_LOG_MAX]; > - > - struct printk_info info; > - char text_buf[CONSOLE_EXT_LOG_MAX]; > - struct printk_record record; > + struct printk_buffers pbufs; > }; > > static __printf(3, 4) __cold > @@ -786,8 +784,10 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf, > size_t count, loff_t *ppos) > { > struct devkmsg_user *user = file->private_data; > - struct printk_record *r = &user->record; > - size_t len; > + char *outbuf = &user->pbufs.outbuf[0]; > + struct printk_message pmsg = { > + .pbufs = &user->pbufs, > + }; > ssize_t ret; > > if (!user) > @@ -797,7 +797,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf, > if (ret) > return ret; > > - if (!prb_read_valid(prb, atomic64_read(&user->seq), r)) { > + while (!printk_get_next_message(&pmsg, atomic64_read(&user->seq), true)) {
A problem is that printk_get_next_message() does not format the message when it shoud get supressed on the console.
I would solve it be adding a parameter to printk_get_next_message() that will tell whether to suppress or not, e.g. @can_suppress.
> if (file->f_flags & O_NONBLOCK) { > ret = -EAGAIN; > goto out; > @@ -814,36 +814,31 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf, > * This pairs with __wake_up_klogd:A. > */ > ret = wait_event_interruptible(log_wait, > - prb_read_valid(prb, > - atomic64_read(&user->seq), r)); /* LMM(devkmsg_read:A) */ > + prb_read_valid(prb, atomic64_read(&user->seq), > + NULL)); /* LMM(devkmsg_read:A) */
The above change from "if" to "while" could be avoided if we use printk_get_next_message() here as well. It looks slightly more strightfoward to me. Or do I miss something, please?
> if (ret) > goto out; > } >
Otherwise, I like this change. It simplifies the code.
Best Regards, Petr
| |