[PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Vivek Goyal]

There are tools like makedumpfile and vmcore-dmesg which can extract
kernel log buffer from vmcore. Since we introduced structured logging,
that functionality is broken. Now user space tools need to know about
"struct log" and offsets of various fields to be able to parse struct
log data and extract text message or dictonary.

This patch exports some of the fields.

Currently I am not exporting log "level" info as that is a bitfield and
offsetof() bitfields can't be calculated. But if people start asking for
log level info in the output then we probably either need to seprate
out "level" or use bit shift operations for flags and level.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 kernel/printk.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2012-07-20 14:02:38.213581253 -0400
+++ linux-2.6/kernel/printk.c	2012-07-20 14:02:42.004581438 -0400
@@ -646,6 +646,15 @@ void log_buf_kexec_setup(void)
 	VMCOREINFO_SYMBOL(log_buf_len);
 	VMCOREINFO_SYMBOL(log_first_idx);
 	VMCOREINFO_SYMBOL(log_next_idx);
+	/*
+	 * Export struct log size and field offsets. User space tools can
+	 * parse it and detect any changes to structure down the line.
+	 */
+	VMCOREINFO_STRUCT_SIZE(log);
+	VMCOREINFO_OFFSET(log, ts_nsec);
+	VMCOREINFO_OFFSET(log, len);
+	VMCOREINFO_OFFSET(log, text_len);
+	VMCOREINFO_OFFSET(log, dict_len);
 }
 #endif
 

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Kay Sievers]

On Wed, Jul 18, 2012 at 7:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:

> Currently I am not exporting log "level" info as that is a bitfield and
> offsetof() bitfields can't be calculated.

We could make the level the lower 3 bits of the byte, export the byte,
and define that only 3 bits of the byte are valid? Would that help?

>  kernel/printk.c |    9 +++++++++
>  1 file changed, 9 insertions(+)

> +       /*
> +        * Export struct log size and field offsets. User space tools can
> +        * parse it and detect any changes to structure down the line.
> +        */
> +       VMCOREINFO_STRUCT_SIZE(log);
> +       VMCOREINFO_OFFSET(log, ts_nsec);
> +       VMCOREINFO_OFFSET(log, len);
> +       VMCOREINFO_OFFSET(log, text_len);
> +       VMCOREINFO_OFFSET(log, dict_len);

Ah, nice, that's how you handle exporting structures, it was still on
my list, to find out how all that should look like.

Cc:ing Greg, to pick it up.

Thanks a lot for taking care of it,
Kay

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Vivek Goyal]

On Wed, Jul 18, 2012 at 07:27:08PM +0200, Kay Sievers wrote:
> On Wed, Jul 18, 2012 at 7:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > Currently I am not exporting log "level" info as that is a bitfield and
> > offsetof() bitfields can't be calculated.
> 
> We could make the level the lower 3 bits of the byte, export the byte,
> and define that only 3 bits of the byte are valid? Would that help?

Yes, that should work. Here is the prototype patch which stores 5 bits
of flag and 3 bits of level in a byte. I have not tested it yet, but
if you like the approach, I will test it.

Thanks
Vivek


---
 kernel/printk.c |   48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2012-07-20 14:02:42.000000000 -0400
+++ linux-2.6/kernel/printk.c	2012-07-20 16:34:24.088964153 -0400
@@ -200,14 +200,19 @@ enum log_flags {
 	LOG_CONT	= 8,	/* text is a fragment of a continuation line */
 };
 
+#define LOG_FLAG_SHIFT		3
+#define LOG_LEVEL_MASK		((1 << LOG_FLAG_SHIFT) - 1)
+#define LOG_FLAGS(log)		((log)->flags_level >> LOG_FLAG_SHIFT)
+#define LOG_LEVEL(log)		(((log)->flags_level) & LOG_LEVEL_MASK)
+
 struct log {
 	u64 ts_nsec;		/* timestamp in nanoseconds */
 	u16 len;		/* length of entire record */
 	u16 text_len;		/* length of text buffer */
 	u16 dict_len;		/* length of dictionary buffer */
 	u8 facility;		/* syslog facility */
-	u8 flags:5;		/* internal record flags */
-	u8 level:3;		/* syslog level */
+	u8 flags_level;		/* 5 bit internal record flags, 3 bits syslog
+				 * level */
 };
 
 /*
@@ -342,8 +347,8 @@ static void log_store(int facility, int 
 	memcpy(log_dict(msg), dict, dict_len);
 	msg->dict_len = dict_len;
 	msg->facility = facility;
-	msg->level = level & 7;
-	msg->flags = flags & 0x1f;
+	msg->flags_level = level & 7;
+	msg->flags_level |= (flags & 0x1f) << LOG_FLAG_SHIFT;
 	if (ts_nsec > 0)
 		msg->ts_nsec = ts_nsec;
 	else
@@ -463,7 +468,8 @@ static ssize_t devkmsg_read(struct file 
 	ts_usec = msg->ts_nsec;
 	do_div(ts_usec, 1000);
 	len = sprintf(user->buf, "%u,%llu,%llu;",
-		      (msg->facility << 3) | msg->level, user->seq, ts_usec);
+		      (msg->facility << 3) | LOG_LEVEL(msg), user->seq,
+	 	      ts_usec);
 
 	/* escape non-printable characters */
 	for (i = 0; i < msg->text_len; i++) {
@@ -655,6 +661,8 @@ void log_buf_kexec_setup(void)
 	VMCOREINFO_OFFSET(log, len);
 	VMCOREINFO_OFFSET(log, text_len);
 	VMCOREINFO_OFFSET(log, dict_len);
+	VMCOREINFO_OFFSET(log, flags_level);
+	VMCOREINFO_LENGTH(log_level_bits, 3);
 }
 #endif
 
@@ -831,7 +839,7 @@ static size_t print_time(u64 ts, char *b
 static size_t print_prefix(const struct log *msg, bool syslog, char *buf)
 {
 	size_t len = 0;
-	unsigned int prefix = (msg->facility << 3) | msg->level;
+	unsigned int prefix = (msg->facility << 3) | LOG_LEVEL(msg);
 
 	if (syslog) {
 		if (buf) {
@@ -860,14 +868,14 @@ static size_t msg_print_text(const struc
 	bool newline = true;
 	size_t len = 0;
 
-	if ((prev & LOG_CONT) && !(msg->flags & LOG_PREFIX))
+	if ((prev & LOG_CONT) && !(LOG_FLAGS(msg) & LOG_PREFIX))
 		prefix = false;
 
-	if (msg->flags & LOG_CONT) {
+	if (LOG_FLAGS(msg) & LOG_CONT) {
 		if ((prev & LOG_CONT) && !(prev & LOG_NEWLINE))
 			prefix = false;
 
-		if (!(msg->flags & LOG_NEWLINE))
+		if (!(LOG_FLAGS(msg) & LOG_NEWLINE))
 			newline = false;
 	}
 
@@ -944,7 +952,7 @@ static int syslog_print(char __user *buf
 			/* message fits into buffer, move forward */
 			syslog_idx = log_next(syslog_idx);
 			syslog_seq++;
-			syslog_prev = msg->flags;
+			syslog_prev = LOG_FLAGS(msg);
 			n -= syslog_partial;
 			syslog_partial = 0;
 		} else if (!len){
@@ -1038,7 +1046,7 @@ static int syslog_print_all(char __user 
 			}
 			idx = log_next(idx);
 			seq++;
-			prev = msg->flags;
+			prev = LOG_FLAGS(msg);
 
 			raw_spin_unlock_irq(&logbuf_lock);
 			if (copy_to_user(buf + len, text, textlen))
@@ -1178,7 +1186,7 @@ int do_syslog(int type, char __user *buf
 				error += msg_print_text(msg, prev, true, NULL, 0);
 				idx = log_next(idx);
 				seq++;
-				prev = msg->flags;
+				prev = LOG_FLAGS(msg);
 			}
 			error -= syslog_partial;
 		}
@@ -1979,6 +1987,7 @@ again:
 		struct log *msg;
 		size_t len;
 		int level;
+		u16 log_flags;
 
 		raw_spin_lock_irqsave(&logbuf_lock, flags);
 		if (seen_seq != log_next_seq) {
@@ -1997,7 +2006,7 @@ skip:
 			break;
 
 		msg = log_from_idx(console_idx);
-		if (msg->flags & LOG_NOCONS) {
+		if (LOG_FLAGS(msg) & LOG_NOCONS) {
 			/*
 			 * Skip record we have buffered and already printed
 			 * directly to the console when we received it.
@@ -2009,16 +2018,17 @@ skip:
 			 * CON_PRINTBUFFER console. Clear the flag so we
 			 * will properly dump everything later.
 			 */
-			msg->flags &= ~LOG_NOCONS;
+			log_flags = LOG_FLAGS(msg) & ~ LOG_NOCONS;
+			msg->flags_level = log_flags << LOG_FLAG_SHIFT | LOG_LEVEL(msg);
 			goto skip;
 		}
 
-		level = msg->level;
+		level = LOG_LEVEL(msg);
 		len = msg_print_text(msg, console_prev, false,
 				     text, sizeof(text));
 		console_idx = log_next(console_idx);
 		console_seq++;
-		console_prev = msg->flags;
+		console_prev = LOG_FLAGS(msg);
 		raw_spin_unlock(&logbuf_lock);
 
 		stop_critical_timings();	/* don't trace print latency */
@@ -2645,7 +2655,7 @@ bool kmsg_dump_get_buffer(struct kmsg_du
 		l += msg_print_text(msg, prev, true, NULL, 0);
 		idx = log_next(idx);
 		seq++;
-		prev = msg->flags;
+		prev = LOG_FLAGS(msg);
 	}
 
 	/* move first record forward until length fits into the buffer */
@@ -2658,7 +2668,7 @@ bool kmsg_dump_get_buffer(struct kmsg_du
 		l -= msg_print_text(msg, prev, true, NULL, 0);
 		idx = log_next(idx);
 		seq++;
-		prev = msg->flags;
+		prev = LOG_FLAGS(msg);
 	}
 
 	/* last message in next interation */
@@ -2673,7 +2683,7 @@ bool kmsg_dump_get_buffer(struct kmsg_du
 		l += msg_print_text(msg, prev, syslog, buf + l, size - l);
 		idx = log_next(idx);
 		seq++;
-		prev = msg->flags;
+		prev = LOG_FLAGS(msg);
 	}
 
 	dumper->next_seq = next_seq;

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Kay Sievers]

On Wed, Jul 18, 2012 at 7:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jul 18, 2012 at 07:27:08PM +0200, Kay Sievers wrote:
>> On Wed, Jul 18, 2012 at 7:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> > Currently I am not exporting log "level" info as that is a bitfield and
>> > offsetof() bitfields can't be calculated.
>>
>> We could make the level the lower 3 bits of the byte, export the byte,
>> and define that only 3 bits of the byte are valid? Would that help?
>
> Yes, that should work. Here is the prototype patch which stores 5 bits
> of flag and 3 bits of level in a byte. I have not tested it yet, but
> if you like the approach, I will test it.

> -       u8 flags:5;             /* internal record flags */
> -       u8 level:3;             /* syslog level */
> +       u8 flags_level;         /* 5 bit internal record flags, 3 bits syslog

Looks ok.

If we would swap the 5 + 3 bit field byte declaration, and add
__packed, we can still not rely on the level to be consistently the
lower 3 bits of the byte, right?

Thanks,
Kay

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Vivek Goyal]

On Thu, Jul 19, 2012 at 11:38:57AM +0200, Kay Sievers wrote:
> On Wed, Jul 18, 2012 at 7:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Jul 18, 2012 at 07:27:08PM +0200, Kay Sievers wrote:
> >> On Wed, Jul 18, 2012 at 7:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >>
> >> > Currently I am not exporting log "level" info as that is a bitfield and
> >> > offsetof() bitfields can't be calculated.
> >>
> >> We could make the level the lower 3 bits of the byte, export the byte,
> >> and define that only 3 bits of the byte are valid? Would that help?
> >
> > Yes, that should work. Here is the prototype patch which stores 5 bits
> > of flag and 3 bits of level in a byte. I have not tested it yet, but
> > if you like the approach, I will test it.
> 
> > -       u8 flags:5;             /* internal record flags */
> > -       u8 level:3;             /* syslog level */
> > +       u8 flags_level;         /* 5 bit internal record flags, 3 bits syslog
> 
> Looks ok.
> 
> If we would swap the 5 + 3 bit field byte declaration, and add
> __packed, we can still not rely on the level to be consistently the
> lower 3 bits of the byte, right?

Current code assumes that level bits are least significant bits in
flags_level. I could export another field say "log_level_bit_offset=0" to
represent the offset of level bits and that way you will retain the
flexibilty of changing the position of level bits. I am not sure if it
is worth. Down the line if numeber of flags outgrow 5bits, one can 
just move flags to a separate field and possibly use those 5bits for
something else.

I guess I will change the patch to also level bit offset and remove
the assumption that level bits are always starting at offset 0. Will test
changes and repost the V2 of patches.

Thanks
Vivek

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Vivek Goyal]

On Thu, Jul 19, 2012 at 09:57:36AM -0400, Vivek Goyal wrote:
> On Thu, Jul 19, 2012 at 11:38:57AM +0200, Kay Sievers wrote:
> > On Wed, Jul 18, 2012 at 7:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > On Wed, Jul 18, 2012 at 07:27:08PM +0200, Kay Sievers wrote:
> > >> On Wed, Jul 18, 2012 at 7:18 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > >>
> > >> > Currently I am not exporting log "level" info as that is a bitfield and
> > >> > offsetof() bitfields can't be calculated.
> > >>
> > >> We could make the level the lower 3 bits of the byte, export the byte,
> > >> and define that only 3 bits of the byte are valid? Would that help?
> > >
> > > Yes, that should work. Here is the prototype patch which stores 5 bits
> > > of flag and 3 bits of level in a byte. I have not tested it yet, but
> > > if you like the approach, I will test it.
> > 
> > > -       u8 flags:5;             /* internal record flags */
> > > -       u8 level:3;             /* syslog level */
> > > +       u8 flags_level;         /* 5 bit internal record flags, 3 bits syslog
> > 
> > Looks ok.
> > 
> > If we would swap the 5 + 3 bit field byte declaration, and add
> > __packed, we can still not rely on the level to be consistently the
> > lower 3 bits of the byte, right?
> 

I think I missed your point in last response. Are you saying that retain
bit fields  for flags and level, and add __packed() and that will make sure
level bits are always lowest 3bits? I am really not sure how that is going
to work. Also if you want to add more fields to struct log down the line,
it will be a problem to determine the offset of byte where level bits are
stored. 

Thanks
Vivek

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Kay Sievers]

On Thu, Jul 19, 2012 at 4:08 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jul 19, 2012 at 09:57:36AM -0400, Vivek Goyal wrote:
>> On Thu, Jul 19, 2012 at 11:38:57AM +0200, Kay Sievers wrote:

>> > If we would swap the 5 + 3 bit field byte declaration, and add
>> > __packed, we can still not rely on the level to be consistently the
>> > lower 3 bits of the byte, right?
>
> I think I missed your point in last response. Are you saying that retain
> bit fields  for flags and level, and add __packed() and that will make sure
> level bits are always lowest 3bits?

It was more a question, I don't know how reliable that would be.

> I am really not sure how that is going
> to work. Also if you want to add more fields to struct log down the line,
> it will be a problem to determine the offset of byte where level bits are
> stored.

I guess, we could make sure that it's always the lowest 3 bits of a
byte. But the question if that is safe to do at all still remains. :)

Kay

Re: [PATCH] printk: Export struct log size and member offsets through vmcoreinfo

[Eric W. Biederman]

Kay Sievers <kay@vrfy.org> writes:

> On Thu, Jul 19, 2012 at 4:08 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Thu, Jul 19, 2012 at 09:57:36AM -0400, Vivek Goyal wrote:
>>> On Thu, Jul 19, 2012 at 11:38:57AM +0200, Kay Sievers wrote:
>
>>> > If we would swap the 5 + 3 bit field byte declaration, and add
>>> > __packed, we can still not rely on the level to be consistently the
>>> > lower 3 bits of the byte, right?
>>
>> I think I missed your point in last response. Are you saying that retain
>> bit fields  for flags and level, and add __packed() and that will make sure
>> level bits are always lowest 3bits?
>
> It was more a question, I don't know how reliable that would be.
>
>> I am really not sure how that is going
>> to work. Also if you want to add more fields to struct log down the line,
>> it will be a problem to determine the offset of byte where level bits are
>> stored.
>
> I guess, we could make sure that it's always the lowest 3 bits of a
> byte. But the question if that is safe to do at all still remains. :)

Using bit fields in interfaces is probably not a good idea in practice.

The order of the bits is constrained by whatever your C ABI is.  However
the C abi can choose different orders on different architectures.

So if my memory is correct you can not use bitfields portably to choose
the low 3 bits of a byte, without a lot of #ifdef LITTLE_ENDIAN_BIT_FIELD
and the like.

So as general advice bitfields are good for saving space for purely
internal structures (if your compiler generates good code for them)
but for interfacing with other code or hardware you don't want to use
them.  Too much complexity for too little gain.

If bitfields were easily portable the kernel would be full of them as
they would make talking with hardware control registers much easier.

Eric