Skip to contentSkip to conversation

printk: Export struct log size and member offsets through vmcoreinfo

PATCH In review Vivek Goyal 3 participants 8 messages Jul 18, 2012 – Jul 20, 2012

Subsystems (2)
Live update kexec handover (KHO)
lore
Share & tools
lore

Vivek Goyal

author
#0 lore raw

PATCH 1/1 printk: Export struct log size and member offsets through vmcoreinfo

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 <[email protected]>
---
 kernel/printk.c |    9 +++++++++
 1 file changed, 9 insertions(+)
kernel/printk.c +9 -0
Index: linux-2.6/kernel/printk.c
===================================================================
@@ -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

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#1 lore raw
Quoted 2 lines · Vivek Goyal

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 void log_buf_kexec_setup(void) · line 657

Quoted 12 lines
 kernel/printk.c |    9 +++++++++
 1 file changed, 9 insertions(+)
kernel/printk.c +9 -0
+       /*
+        * 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

author
#2 lore raw
Quoted 7 lines · Kay Sievers
Quoted 2 lines · Vivek Goyal

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(-)
kernel/printk.c +29 -19
Index: linux-2.6/kernel/printk.c
===================================================================
@@ -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;

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#3 lore raw
Quoted 13 lines · Vivek Goyal
Quoted 7 lines · Kay Sievers
Quoted 2 lines · Vivek Goyal

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.

kernel/printk.c enum log_flags { · line 214

Quoted 3 lines
kernel/printk.c +1 -2
-       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

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#4 lore raw
Quoted 18 lines · Kay Sievers
Quoted 13 lines · Vivek Goyal
Quoted 7 lines · Kay Sievers
Quoted 2 lines · Vivek Goyal

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.

kernel/printk.c enum log_flags { · line 214

Quoted 3 lines
kernel/printk.c +1 -2
-       u8 flags:5;             /* internal record flags */
-       u8 level:3;             /* syslog level */
+       u8 flags_level;         /* 5 bit internal record flags, 3 bits syslog
Quoted 5 lines

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

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#5 lore raw
Quoted 19 lines · Vivek Goyal
Quoted 18 lines · Kay Sievers
Quoted 16 lines · Vivek Goyal
Quoted 7 lines · Kay Sievers
Quoted 2 lines · Vivek Goyal

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.

kernel/printk.c +1 -2
-       u8 flags:5;             /* internal record flags */
-       u8 level:3;             /* syslog level */
+       u8 flags_level;         /* 5 bit internal record flags, 3 bits syslog
Quoted 6 lines
Quoted 5 lines

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

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#6 lore raw
Quoted 10 lines · Vivek Goyal
Quoted 5 lines · Vivek Goyal
Quoted 3 lines · Kay Sievers

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.

Quoted 4 lines

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

#7 lore raw
Quoted 14 lines · Kay Sievers
Quoted 10 lines · Vivek Goyal
Quoted 5 lines · Vivek Goyal
Quoted 3 lines · Kay Sievers

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.

Quoted 7 lines
Quoted 4 lines

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