seq-buf/x86/printk: Print all stacks from NMI safely

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kvm/mmutrace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +1 -1

index 5aaf35641768..ce463a9cc8fb 100644
@@ -22,7 +22,7 @@
 	__entry->unsync = sp->unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({				        \
-	const u32 saved_len = p->len;					\
+	const char *saved_ptr = trace_seq_buffer_ptr(p);		\
 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
arch/x86/kvm/mmutrace.h | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +0 -0

index 5aaf35641768..ce463a9cc8fb 100644

arch/x86/kvm/mmutrace.h +1 -1

 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

@@ -41,7 +41,7 @@
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
 			 __entry->unsync ? "unsync" : "sync", 0);	\
-	p->buffer + saved_len;						\
+	saved_ptr;							\
 		})
 
 #define kvm_mmu_trace_pferr_flags       \

Signature · 1 line

2.1.1

Quoted 21 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
arch/x86/kvm/mmutrace.h | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +0 -0

index 5aaf35641768..ce463a9cc8fb 100644
@@ -22,7 +22,7 @@
 	__entry->unsync = sp->unsync;
 

arch/x86/kvm/mmutrace.h line 25

Quoted 3 lines

arch/x86/kvm/mmutrace.h +1 -1

 #define KVM_MMU_PAGE_PRINTK() ({				        \
-	const u32 saved_len = p->len;					\
+	const char *saved_ptr = trace_seq_buffer_ptr(p);		\

I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.

Quoted 15 lines

arch/x86/kvm/mmutrace.h +1 -1

 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

@@ -41,7 +41,7 @@
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
 			 __entry->unsync ? "unsync" : "sync", 0);	\
-	p->buffer + saved_len;						\
+	saved_ptr;							\
 		})
 
 #define kvm_mmu_trace_pferr_flags       \

Signature · 1 line

2.1.1

Quoted 23 lines · Mark D

Quoted 21 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
arch/x86/kvm/mmutrace.h | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +0 -0

index 5aaf35641768..ce463a9cc8fb 100644
@@ -22,7 +22,7 @@
 	__entry->unsync = sp->unsync;
 

Quoted 5 lines

arch/x86/kvm/mmutrace.h line 25

Quoted 3 lines

arch/x86/kvm/mmutrace.h +1 -1

 #define KVM_MMU_PAGE_PRINTK() ({				        \
-	const u32 saved_len = p->len;					\
+	const char *saved_ptr = trace_seq_buffer_ptr(p);		\

I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.

Ah right. It should be 'char * const'.

Quoted 17 lines

Quoted 16 lines

arch/x86/kvm/mmutrace.h +1 -1

 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

@@ -41,7 +41,7 @@
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
 			 __entry->unsync ? "unsync" : "sync", 0);	\
-	p->buffer + saved_len;						\
+	saved_ptr;							\
 		})
 
 #define kvm_mmu_trace_pferr_flags       \

Signature · 1 line

2.1.1

Quoted 32 lines · On Tue, 4 Nov 2014 14:09:54 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 28 lines · Mark D

Quoted 19 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
arch/x86/kvm/mmutrace.h | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +0 -0

index 5aaf35641768..ce463a9cc8fb 100644

arch/x86/kvm/mmutrace.h line 25

Quoted 5 lines

arch/x86/kvm/mmutrace.h +1 -1

@@ -22,7 +22,7 @@
 	__entry->unsync = sp->unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({				        \
-	const u32 saved_len = p->len;					\
+	const char *saved_ptr = trace_seq_buffer_ptr(p);		\

I think the above should not be a const char *, because the location pointed to is surely being changed. It should either be a char * or a char * const.

Quoted 2 lines

Ah right. It should be 'char * const'.

Actually, I take that back. The contents of saved_ptr should not be modified.

It may seem strange, but the update is done via the trace_seq_printf(). Then that content is return back to the user. The user should definitely *not* modify the contents of saved_ptr.

This patch is good as is. It should not be a char *, or char * const.

-- Steve

Quoted 18 lines

Quoted 17 lines

Quoted 16 lines

arch/x86/kvm/mmutrace.h +1 -1

 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

@@ -41,7 +41,7 @@
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
 			 __entry->unsync ? "unsync" : "sync", 0);	\
-	p->buffer + saved_len;						\
+	saved_ptr;							\
 		})
 
 #define kvm_mmu_trace_pferr_flags       \

Signature · 1 line

2.1.1

From: "Steven Rostedt (Red Hat)" <[email protected]>

To allow for the restructiong of the trace_seq code, we need users of it to use the helper functions instead of accessing the internals of the trace_seq structure itself.

Cc Mark Rustad <[email protected]>

Cc Jeff Kirsher <[email protected]>

Cc Paolo Bonzini <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kvm/mmutrace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kvm/mmutrace.h +2 -2

index 5aaf35641768..ce463a9cc8fb 100644
@@ -22,7 +22,7 @@
 	__entry->unsync = sp->unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({				        \
-	const u32 saved_len = p->len;					\
+	const char *saved_ptr = trace_seq_buffer_ptr(p);		\
 	static const char *access_str[] = {			        \
 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
 	};							        \

@@ -41,7 +41,7 @@
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
 			 __entry->unsync ? "unsync" : "sync", 0);	\
-	p->buffer + saved_len;						\
+	saved_ptr;							\
 		})
 
 #define kvm_mmu_trace_pferr_flags       \

Signature · 1 line

2.1.1

From: "Steven Rostedt (Red Hat)" <[email protected]>

Use the helper function trace_seq_buffer_ptr() to get the current location of the next buffer write of a trace_seq object, instead of open coding it.

This facilitates the conversion of trace_seq to use seq_buf.

Cc Chen Gong <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tony Luck <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

From: "Steven Rostedt (Red Hat)" <[email protected]>

Use the helper function trace_seq_buffer_ptr() to get the current location of the next buffer write of a trace_seq object, instead of open coding it.

This facilitates the conversion of trace_seq to use seq_buf.

Cc Chen Gong <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tony Luck <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

From: "Steven Rostedt (Red Hat)" <[email protected]>

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/seq_buf.h              |  72 ++++++++
 include/linux/trace_seq.h            |  10 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 184 +++++++++----------
 8 files changed, 534 insertions(+), 125 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c

include/linux/seq_buf.h new file +9 -0

new file mode 100644
index 000000000000..97872154d51c
@@ -0,0 +1,72 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include <linux/fs.h>
+
+#include <asm/page.h>
+
+/*
+ * Trace sequences are used to allow a function to call several other functions

include/linux/seq_buf.h new file +8 -0

+ */
+struct seq_buf {
+	unsigned char		*buffer;
+	unsigned int		size;
+	unsigned int		len;
+	unsigned int		readpos;
+};
+

kernel/trace/seq_buf.c new file +4 -0

new file mode 100644
index 000000000000..2bf582753902
@@ -0,0 +1,341 @@
+/*
+ * seq_buf.c
+ *
+ * Copyright (C) 2014 Red Hat Inc, Steven Rostedt <[email protected]>

kernel/trace/seq_buf.c new file +1 -0

+#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)

kernel/trace/seq_buf.c new file +23 -0

+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+/**
+ * seq_buf_bprintf - Write the printf string from binary arguments
+ * @s: seq_buf descriptor
+ * @fmt: The format string for the @binary arguments
+ * @binary: The binary arguments for @fmt.
+ *
+ * When recording in a fast path, a printf may be recorded with just
+ * saving the format and the arguments as they were passed to the
+ * function, instead of wasting cycles converting the arguments into
+ * ASCII characters. Instead, the arguments are saved in a 32 bit
+ * word array that is defined by the format string constraints.
+ *
+ * This function will take the format and the binary array and finish
+ * the conversion into the ASCII string within the buffer.
+ *

kernel/trace/seq_buf.c new file +20 -0

+		ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
+		if (s->len + ret < s->size) {
+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+/**
+ * seq_buf_puts - sequence printing of simple string
+ * @s: seq_buf descriptor
+ * @str: simple string to record
+ *
+ * Copy a simple string into the sequence buffer.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_puts(struct seq_buf *s, const char *str)

kernel/trace/seq_buf.c new file +4 -0

+	seq_buf_set_overflow(s);
+	return -1;
+}
+

kernel/trace/seq_buf.c new file +12 -0

+
+/**
+ * seq_buf_putmem - write raw data into the sequenc buffer
+ * @s: seq_buf descriptor
+ * @mem: The raw memory to copy into the buffer
+ * @len: The length of the raw memory to copy (in bytes)
+ *
+ * There may be cases where raw memory needs to be written into the
+ * buffer and a strcpy() would not work. Using this function allows
+ * for such cases.
+ *
+ * Returns zero on success, -1 on overflow

kernel/trace/seq_buf.c new file +38 -0

+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+#define MAX_MEMHEX_BYTES	8U
+#define HEX_CHARS		(MAX_MEMHEX_BYTES*2 + 1)
+
+/**
+ * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
+ * @s: seq_buf descriptor
+ * @mem: The raw memory to write its hex ASCII representation of
+ * @len: The length of the raw memory to copy (in bytes)
+ *
+ * This is similar to seq_buf_putmem() except instead of just copying the
+ * raw memory into the buffer it writes its ASCII representation of it
+ * in hex characters.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+		       unsigned int len)
+{
+	unsigned char hex[HEX_CHARS];
+	const unsigned char *data = mem;
+	unsigned int start_len;
+	int i, j;
+
+	WARN_ON(s->size == 0);
+
+	while (len) {
+		start_len = min(len, HEX_CHARS - 1);
+#ifdef __BIG_ENDIAN
+		for (i = 0, j = 0; i < start_len; i++) {
+#else
+		for (i = start_len-1, j = 0; i >= 0; i--) {
+#endif
+			hex[j++] = hex_asc_hi(data[i]);
+			hex[j++] = hex_asc_lo(data[i]);

kernel/trace/seq_buf.c new file +5 -0

+			return -1;
+	}
+	return 0;
+}
+

kernel/trace/seq_buf.c new file +8 -0

+
+/**
+ * seq_buf_to_user - copy the squence buffer to user space
+ * @s: seq_buf descriptor
+ * @ubuf: The userspace memory location to copy to
+ * @cnt: The amount to copy
+ *
+ * Copies the sequence buffer into the userspace memory pointed to

+ *
+ * Returns -EFAULT if the copy to userspace fails.
+ */

kernel/trace/seq_buf.c new file +23 -0

+int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
+{
+	int len;
+	int ret;
+
+	if (!cnt)
+		return 0;
+
+	if (s->len <= s->readpos)
+		return -EBUSY;
+
+	len = s->len - s->readpos;
+	if (cnt > len)
+		cnt = len;
+	ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
+	if (ret == cnt)
+		return -EFAULT;
+
+	cnt -= ret;
+
+	s->readpos += cnt;
+	return cnt;
+}

kernel/trace/trace_seq.c +15 -6

 /* How much buffer is written? */
-#define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
+#define TRACE_SEQ_BUF_USED(s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1))
+
+/*
+ * trace_seq should work with being initialized with 0s.
+ */
+static inline void __trace_seq_init(struct trace_seq *s)
+{
+	if (unlikely(!s->seq.size))
+		trace_seq_init(s);
+}
 
 /**
  * trace_print_seq - move the contents of trace_seq into a seq_file

@@ -43,10 +52,11 @@
  */
 int trace_print_seq(struct seq_file *m, struct trace_seq *s)
 {
-	unsigned int len = TRACE_SEQ_BUF_USED(s);
 	int ret;
 
-	ret = seq_write(m, s->buffer, len);
+	__trace_seq_init(s);
+
+	ret = seq_buf_print_seq(m, &s->seq);
 
 	/*
 	 * Only reset this buffer if we successfully wrote to the

@@ -77,25 +87,25 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s)
  */
 int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;
 	va_list ap;
-	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return 0;
 

Quoted 39 lines · Steven Rostedt

From: "Steven Rostedt (Red Hat)" <[email protected]>

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/seq_buf.h              |  72 ++++++++
 include/linux/trace_seq.h            |  10 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 184 +++++++++----------
 8 files changed, 534 insertions(+), 125 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c

include/linux/seq_buf.h new file +13 -0

new file mode 100644
index 000000000000..97872154d51c
@@ -0,0 +1,72 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include <linux/fs.h>
+
+#include <asm/page.h>
+
+/*
+ * Trace sequences are used to allow a function to call several other functions
+ * to create a string of data to use.
+ */
+
+/**

include/linux/seq_buf.h line 19

Quoted 6 lines

include/linux/seq_buf.h new file +6 -0

+ * seq_buf - seq buffer structure
+ * @buffer:	pointer to the buffer
+ * @size:	size of the buffer
+ * @len:	the amount of data inside the buffer
+ * @readpos:	The next position to read in the buffer.
+ * @overflow:	Set if more bytes should have been written to buffer

There is no @overflow flag in the end.

Also I would add an explanation of the overall logic. If I get it correctly from the code, it is:

/*
* The last byte of the buffer is used to detect an overflow in some
* operations. Therefore, the buffer offers (@size - 1) bytes for valid
* data.

*/

Quoted 8 lines

include/linux/seq_buf.h new file +8 -0

+ */
+struct seq_buf {
+	unsigned char		*buffer;
+	unsigned int		size;
+	unsigned int		len;
+	unsigned int		readpos;
+};
+

[...]

Quoted 14 lines

kernel/trace/seq_buf.c new file +8 -0

new file mode 100644
index 000000000000..2bf582753902
@@ -0,0 +1,341 @@
+/*
+ * seq_buf.c
+ *
+ * Copyright (C) 2014 Red Hat Inc, Steven Rostedt <[email protected]>
+ *
+ * The seq_buf is a handy tool that allows you to pass a descriptor around
+ * to a buffer that other functions can write to. It is similar to the
+ * seq_file functionality but has some differences.

kernel/trace/seq_buf.c line 14

Quoted 6 lines

kernel/trace/seq_buf.c new file +6 -0

+ *
+ * To use it, the seq_buf must be initialized with seq_buf_init().
+ * This will set up the counters within the descriptor. You can call
+ * seq_buf_init() more than once to reset the seq_buf to start
+ * from scratch.
+ * 

^ trailing whitespace :-)

kernel/trace/seq_buf.c line 20

Quoted 7 lines

kernel/trace/seq_buf.c new file +6 -0

+ */
+#include <linux/uaccess.h>
+#include <linux/seq_file.h>
+#include <linux/seq_buf.h>
+
+/* How much buffer is left on the seq_buf? */

I would write the following to explain the -1:

/* How much buffer is left for valid data */

kernel/trace/seq_buf.c line 21

Quoted 1 line

kernel/trace/seq_buf.c new file +1 -0

+#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)

Hmm, it might overflow when the buffer has overflown (s->len == s->size) or when the buffer is not initialized (s->size == 0). Note that the result should be unsigned int.

I can't find any cool solution as a macro at the moment. It might be better to define an inline function for this.

kernel/trace/seq_buf.c line 23

Quoted 1 line

kernel/trace/seq_buf.c new file +1 -0

+/* How much buffer is written? */

I would write the following to explain the -1:

/* How much buffer is written with valid data */

Quoted 1 line

kernel/trace/seq_buf.c new file +1 -0

+#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)

[...]

kernel/trace/seq_buf.c line 96

Quoted 10 lines

kernel/trace/seq_buf.c new file +10 -0

+
+/**
+ * seq_buf_bitmask - write a bitmask array in its ASCII representation
+ * @s:		seq_buf descriptor
+ * @maskp:	points to an array of unsigned longs that represent a bitmask
+ * @nmaskbits:	The number of bits that are valid in @maskp
+ *
+ * Writes a ASCII representation of a bitmask string into @s.
+ *
+ * Returns the number of bytes written.

The text should be:

* Returns zero on success, -1 on overflow.

kernel/trace/seq_buf.c line 107

Quoted 13 lines

+ */

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = SEQ_BUF_LEFT(s);
 
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It writes to the beginning of the buffer. It should be

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

kernel/trace/seq_buf.c

Quoted 2 lines

kernel/trace/seq_buf.c new file +1 -0

+		if (s->len + ret < s->size) {

This will always happen because bitmap_scnprintf() is limited by SEQ_BUF_LEFT(s) and it currently returns the remaining size - len - 1.

You might want to use "s->size - s->len" instead of SEQ_BUF_LEFT(s).

Quoted 29 lines

kernel/trace/seq_buf.c new file +27 -0

+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+/**
+ * seq_buf_bprintf - Write the printf string from binary arguments
+ * @s: seq_buf descriptor
+ * @fmt: The format string for the @binary arguments
+ * @binary: The binary arguments for @fmt.
+ *
+ * When recording in a fast path, a printf may be recorded with just
+ * saving the format and the arguments as they were passed to the
+ * function, instead of wasting cycles converting the arguments into
+ * ASCII characters. Instead, the arguments are saved in a 32 bit
+ * word array that is defined by the format string constraints.
+ *
+ * This function will take the format and the binary array and finish
+ * the conversion into the ASCII string within the buffer.
+ *
+ * Returns zero on success, -1 on overflow.
+ */
+int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
+{

kernel/trace/seq_buf.c line 106

Quoted 6 lines

kernel/trace/seq_buf.c new file +6 -0

+	unsigned int len = SEQ_BUF_LEFT(s);
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {

Always true. It is the same problem as in seq_buf_bitmask().

kernel/trace/seq_buf.c line 143

Quoted 3 lines

kernel/trace/seq_buf.c new file +2 -0

+		ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
+		if (s->len + ret < s->size) {

Quoted 23 lines

kernel/trace/seq_buf.c new file +23 -0

+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+/**
+ * seq_buf_puts - sequence printing of simple string
+ * @s: seq_buf descriptor
+ * @str: simple string to record
+ *
+ * Copy a simple string into the sequence buffer.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_puts(struct seq_buf *s, const char *str)
+{
+	unsigned int len = strlen(str);
+
+	WARN_ON(s->size == 0);
+

kernel/trace/seq_buf.c line 171

Quoted 5 lines

kernel/trace/seq_buf.c new file +5 -0

+	if (s->len + len < s->size) {
+		memcpy(s->buffer + s->len, str, len);
+		s->len += len;
+		return 0;
+	}

We might want to copy the maximum possible number of bytes. It will then behave the same as the other functions.

Quoted 5 lines

kernel/trace/seq_buf.c new file +4 -0

+	seq_buf_set_overflow(s);
+	return -1;
+}
+

[...]

Quoted 17 lines

kernel/trace/seq_buf.c new file +17 -0

+
+/**
+ * seq_buf_putmem - write raw data into the sequenc buffer
+ * @s: seq_buf descriptor
+ * @mem: The raw memory to copy into the buffer
+ * @len: The length of the raw memory to copy (in bytes)
+ *
+ * There may be cases where raw memory needs to be written into the
+ * buffer and a strcpy() would not work. Using this function allows
+ * for such cases.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
+{
+	WARN_ON(s->size == 0);
+

kernel/trace/seq_buf.c line 217

Quoted 5 lines

kernel/trace/seq_buf.c new file +5 -0

+	if (s->len + len < s->size) {
+		memcpy(s->buffer + s->len, mem, len);
+		s->len += len;
+		return 0;
+	}

Same as seq_buf_puts(). Do we want to always copy the possible number of bytes?

Quoted 43 lines

kernel/trace/seq_buf.c new file +42 -0

+	seq_buf_set_overflow(s);
+	return -1;
+}
+
+#define MAX_MEMHEX_BYTES	8U
+#define HEX_CHARS		(MAX_MEMHEX_BYTES*2 + 1)
+
+/**
+ * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
+ * @s: seq_buf descriptor
+ * @mem: The raw memory to write its hex ASCII representation of
+ * @len: The length of the raw memory to copy (in bytes)
+ *
+ * This is similar to seq_buf_putmem() except instead of just copying the
+ * raw memory into the buffer it writes its ASCII representation of it
+ * in hex characters.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+		       unsigned int len)
+{
+	unsigned char hex[HEX_CHARS];
+	const unsigned char *data = mem;
+	unsigned int start_len;
+	int i, j;
+
+	WARN_ON(s->size == 0);
+
+	while (len) {
+		start_len = min(len, HEX_CHARS - 1);
+#ifdef __BIG_ENDIAN
+		for (i = 0, j = 0; i < start_len; i++) {
+#else
+		for (i = start_len-1, j = 0; i >= 0; i--) {
+#endif
+			hex[j++] = hex_asc_hi(data[i]);
+			hex[j++] = hex_asc_lo(data[i]);
+		}
+		if (WARN_ON_ONCE(j == 0 || j/2 > len))
+			break;
+

kernel/trace/seq_buf.c line 265

Quoted 6 lines

kernel/trace/seq_buf.c new file +6 -0

+		/* j increments twice per loop */
+		len -= j / 2;
+		hex[j++] = ' ';
+
+		seq_buf_putmem(s, hex, j);
+		if (seq_buf_has_overflowed(s))

We might want to use the seq_buf_putmem() return value here.

Quoted 6 lines

kernel/trace/seq_buf.c new file +5 -0

+			return -1;
+	}
+	return 0;
+}
+

[...]

Quoted 12 lines

kernel/trace/seq_buf.c new file +12 -0

+
+/**
+ * seq_buf_to_user - copy the squence buffer to user space
+ * @s: seq_buf descriptor
+ * @ubuf: The userspace memory location to copy to
+ * @cnt: The amount to copy
+ *
+ * Copies the sequence buffer into the userspace memory pointed to
+ * by @ubuf. It starts from the last read position (@s->readpos)
+ * and writes up to @cnt characters or till it reaches the end of
+ * the content in the buffer (@s->len), which ever comes first.
+ *

Quoted 6 lines

+ * On success, it returns a positive number of the number of bytes
+ * it copied.
+ *
+ * On failure it returns -EBUSY if all of the content in the
+ * sequence has been already read, which includes nothing in the
+ * sequenc (@s->len == @s->readpos).

sequenc -> sequence

Quoted 27 lines

+ *
+ * Returns -EFAULT if the copy to userspace fails.
+ */

kernel/trace/seq_buf.c new file +23 -0

+int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
+{
+	int len;
+	int ret;
+
+	if (!cnt)
+		return 0;
+
+	if (s->len <= s->readpos)
+		return -EBUSY;
+
+	len = s->len - s->readpos;
+	if (cnt > len)
+		cnt = len;
+	ret = copy_to_user(ubuf, s->buffer + s->readpos, cnt);
+	if (ret == cnt)
+		return -EFAULT;
+
+	cnt -= ret;
+
+	s->readpos += cnt;
+	return cnt;
+}

[...]

Quoted 7 lines

kernel/trace/trace_seq.c +0 -0

index 1f24ed99dca2..960ccfb2f50c 100644
@@ -27,10 +27,19 @@
 #include <linux/trace_seq.h>
 

kernel/trace/trace_seq.c line 30

Quoted 3 lines

kernel/trace/trace_seq.c +1 -1

 /* How much buffer is left on the trace_seq? */
-#define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->len)
+#define TRACE_SEQ_BUF_LEFT(s) ((PAGE_SIZE - 1) - (s)->seq.len)

This might overflow when s->len == PAGE_SIZE. I think that it newer happenes because we always check s->full before. The question is if we really want to depend on this.

Quoted 50 lines

kernel/trace/trace_seq.c +18 -7

 /* How much buffer is written? */
-#define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
+#define TRACE_SEQ_BUF_USED(s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1))
+
+/*
+ * trace_seq should work with being initialized with 0s.
+ */
+static inline void __trace_seq_init(struct trace_seq *s)
+{
+	if (unlikely(!s->seq.size))
+		trace_seq_init(s);
+}
 
 /**
  * trace_print_seq - move the contents of trace_seq into a seq_file

@@ -43,10 +52,11 @@
  */
 int trace_print_seq(struct seq_file *m, struct trace_seq *s)
 {
-	unsigned int len = TRACE_SEQ_BUF_USED(s);
 	int ret;
 
-	ret = seq_write(m, s->buffer, len);
+	__trace_seq_init(s);
+
+	ret = seq_buf_print_seq(m, &s->seq);
 
 	/*
 	 * Only reset this buffer if we successfully wrote to the

@@ -77,25 +87,25 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s)
  */
 int trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;
 	va_list ap;
-	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return 0;
 
+	__trace_seq_init(s);
+
 	va_start(ap, fmt);
-	ret = vsnprintf(s->buffer + s->len, len, fmt, ap);
+	seq_buf_vprintf(&s->seq, fmt, ap);
 	va_end(ap);
 

kernel/trace/trace_seq.c int trace_print_seq(struct seq_file *m, struct trace_seq *s) · line 103

Quoted 3 lines

kernel/trace/trace_seq.c +1 -1

 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {

We might check the return value from seq_buf_vprintf() here.

We could do similar thing also in the other functions. We even already store the ret value in some of them.

Best Regards,
Petr

Quoted 16 lines

kernel/trace/seq_buf.c line 107

Quoted 13 lines

+ */

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = SEQ_BUF_LEFT(s);
 
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It writes to the beginning of the buffer. It should be

Quoted 3 lines

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

Yep thanks. Luckily its only user didn't care.

Will fix.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		if (s->len + ret < s->size) {

This will always happen because bitmap_scnprintf() is limited by SEQ_BUF_LEFT(s) and it currently returns the remaining size - len - 1.

Hmm, that's correct, as bitmap_scnprintf() returns the amount written instead of the amount that it would write like snprintf() would.

Quoted 1 line

Quoted 1 line

You might want to use "s->size - s->len" instead of SEQ_BUF_LEFT(s).

That wont help when we make overflow len > size.

Probably should see if ret == the amount of bits required for the bitmask.

Quoted 48 lines

Quoted 48 lines

kernel/trace/seq_buf.c new file +42 -0

+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
 
 
 
+/**
+ * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex
+ * @s: seq_buf descriptor
+ * @mem: The raw memory to write its hex ASCII representation of
+ * @len: The length of the raw memory to copy (in bytes)
+ *
+ * This is similar to seq_buf_putmem() except instead of just copying the
+ * raw memory into the buffer it writes its ASCII representation of it
+ * in hex characters.
+ *
+ * Returns zero on success, -1 on overflow
+ */
+int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+		       unsigned int len)
+{
+	unsigned char hex[HEX_CHARS];
+	const unsigned char *data = mem;
+	unsigned int start_len;
+	int i, j;
+
+	WARN_ON(s->size == 0);
+
+	while (len) {
+		start_len = min(len, HEX_CHARS - 1);
+#ifdef __BIG_ENDIAN
+		for (i = 0, j = 0; i < start_len; i++) {
+#else
+		for (i = start_len-1, j = 0; i >= 0; i--) {
+#endif
+			hex[j++] = hex_asc_hi(data[i]);
+			hex[j++] = hex_asc_lo(data[i]);
+		}
+		if (WARN_ON_ONCE(j == 0 || j/2 > len))
+			break;
+

Quoted 8 lines

kernel/trace/seq_buf.c line 265

Quoted 6 lines

kernel/trace/seq_buf.c new file +6 -0

+		/* j increments twice per loop */
+		len -= j / 2;
+		hex[j++] = ' ';
+
+		seq_buf_putmem(s, hex, j);
+		if (seq_buf_has_overflowed(s))

We might want to use the seq_buf_putmem() return value here.

We could do that.

Quoted 8 lines

Quoted 7 lines

kernel/trace/seq_buf.c new file +5 -0

+			return -1;
+	}
+	return 0;
+}
+

Quoted 35 lines · On Wed, 5 Nov 2014 13:41:47 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 16 lines

kernel/trace/seq_buf.c line 107

Quoted 13 lines

+ */

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = SEQ_BUF_LEFT(s);
 
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It writes to the beginning of the buffer. It should be

Quoted 3 lines

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

Yep thanks. Luckily its only user didn't care.

Will fix.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		if (s->len + ret < s->size) {

This will always happen because bitmap_scnprintf() is limited by SEQ_BUF_LEFT(s) and it currently returns the remaining size - len - 1.

Hmm, that's correct, as bitmap_scnprintf() returns the amount written instead of the amount that it would write like snprintf() would.

Quoted 1 line

Quoted 6 lines

Quoted 1 line

You might want to use "s->size - s->len" instead of SEQ_BUF_LEFT(s).

That wont help when we make overflow len > size.

Probably should see if ret == the amount of bits required for the bitmask.

Here's the new version:

Quoted 38 lines · On Wed, 5 Nov 2014 15:00:07 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 35 lines · On Wed, 5 Nov 2014 13:41:47 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 16 lines

kernel/trace/seq_buf.c line 107

Quoted 13 lines

+ */

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = SEQ_BUF_LEFT(s);
 
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It writes to the beginning of the buffer. It should be

Quoted 3 lines

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

Yep thanks. Luckily its only user didn't care.

Will fix.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		if (s->len + ret < s->size) {

This will always happen because bitmap_scnprintf() is limited by SEQ_BUF_LEFT(s) and it currently returns the remaining size - len - 1.

Hmm, that's correct, as bitmap_scnprintf() returns the amount written instead of the amount that it would write like snprintf() would.

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

You might want to use "s->size - s->len" instead of SEQ_BUF_LEFT(s).

That wont help when we make overflow len > size.

Probably should see if ret == the amount of bits required for the bitmask.

Here's the new version:

Take 2:

int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
int nmaskbits)

{
	unsigned int len = seq_buf_buffer_left(s);

	int ret;

	WARN_ON(s->size == 0);

	if (s->len < s->size) {
		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
		/*
		 * Note, because bitmap_scnprintf() only returns the
		 * number of bytes written and not the number that
		 * would be written, we use the last byte of the buffer
		 * to let us know if we overflowed. There's a small
		 * chance that the bitmap could have fit exactly inside
		 * the buffer, but it's not that critical if that does
		 * happen.
		 */
		if (s->len + ret < s->size) {
			s->len += ret;
			return 0;
		}
	}
	seq_buf_set_overflow(s);
	return -1;
}

-- Steve

Quoted 53 lines · On Wed, 5 Nov 2014 16:17:20 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 38 lines · On Wed, 5 Nov 2014 15:00:07 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 35 lines · On Wed, 5 Nov 2014 13:41:47 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 16 lines

kernel/trace/seq_buf.c line 107

Quoted 13 lines

+ */

kernel/trace/seq_buf.c new file +10 -0

+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = SEQ_BUF_LEFT(s);
 
+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	if (s->len < s->size) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It writes to the beginning of the buffer. It should be

Quoted 3 lines

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

Yep thanks. Luckily its only user didn't care.

Will fix.

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		if (s->len + ret < s->size) {

This will always happen because bitmap_scnprintf() is limited by SEQ_BUF_LEFT(s) and it currently returns the remaining size - len - 1.

Hmm, that's correct, as bitmap_scnprintf() returns the amount written instead of the amount that it would write like snprintf() would.

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

You might want to use "s->size - s->len" instead of SEQ_BUF_LEFT(s).

That wont help when we make overflow len > size.

Probably should see if ret == the amount of bits required for the bitmask.

Here's the new version:

Take 2:

Quoted 4 lines

int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
int nmaskbits)

{
	unsigned int len = seq_buf_buffer_left(s);

Bah, I need to make this: len = s->size - s->len.

As this would work for both cases of what a overflowed buffer is.

Quoted 5 lines

	int ret;

	WARN_ON(s->size == 0);

	if (s->len < s->size) {

Quoted 10 lines

ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits); /*
* Note, because bitmap_scnprintf() only returns the
* number of bytes written and not the number that
* would be written, we use the last byte of the buffer
* to let us know if we overflowed. There's a small
* chance that the bitmap could have fit exactly inside
* the buffer, but it's not that critical if that does
* happen.
*/

Quoted 1 line

		if (s->len + ret < s->size) {

Quoted 7 lines

			s->len += ret;
			return 0;
		}
	}
	seq_buf_set_overflow(s);
	return -1;
}

From 11674c8df0580a03a2517e3a8e4705c47c663564 Mon Sep 17 00:00:00 2001

I'm not going to waist bandwidth reposting the entire series. Here's a new version of this patch. I'm getting ready to pull it into my next queue.

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Link http://lkml.kernel.org/r/[email protected] open here

include/linux/seq_buf.h +8 -0

+extern __printf(2, 3)
+int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
+extern __printf(2, 0)
+int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
+extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,

include/linux/trace_seq.h +0 -2

@@ -12,16 +12,14 @@
 
 struct trace_seq {
 	unsigned char		buffer[PAGE_SIZE];
-	unsigned int		len;
-	unsigned int		readpos;

Quoted 5 lines

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Link http://lkml.kernel.org/r/[email protected] open here

[]

include/linux/seq_buf.h

Quoted 1 line

include/linux/seq_buf.h +0 -0

[]

Quoted 12 lines

include/linux/seq_buf.h +12 -0

+extern __printf(2, 3)
+int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
+extern __printf(2, 0)
+int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args);
+extern int
+seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s);
+extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
+			   int cnt);
+extern int seq_buf_puts(struct seq_buf *s, const char *str);
+extern int seq_buf_putc(struct seq_buf *s, unsigned char c);
+extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);

include/linux/seq_buf.h line 76

Quoted 6 lines

include/linux/seq_buf.h +6 -0

+extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
+			      unsigned int len);
+extern int seq_buf_path(struct seq_buf *s, const struct path *path);
+
+extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+			   int nmaskbits);

Maybe remove the extern bits from the function definitions?

And maybe the unsigned int len/cnt could be size_t

include/linux/trace_seq.h

Quoted 2 lines

include/linux/trace_seq.h +0 -0

[]

Quoted 6 lines

include/linux/trace_seq.h +0 -2

@@ -12,16 +12,14 @@
 
 struct trace_seq {
 	unsigned char		buffer[PAGE_SIZE];
-	unsigned int		len;
-	unsigned int		readpos;

include/linux/trace_seq.h line 22

Quoted 10 lines

include/linux/trace_seq.h +2 -2

+	struct seq_buf		seq;
 	int			full;
 };
 
 static inline void
 trace_seq_init(struct trace_seq *s)
 {
-	s->len = 0;
-	s->readpos = 0;
+	seq_buf_init(&s->seq, s->buffer, PAGE_SIZE);

Maybe the PAGE_SIZE uses could be ARRAY_SIZE(s->buffer)

kernel/trace/trace_seq.c

Quoted 1 line

kernel/trace/trace_seq.c +0 -0

[]

kernel/trace/trace_seq.c line 33

Quoted 3 lines

/* How much buffer is written? */

kernel/trace/trace_seq.c +1 -1

-#define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE - 1))
+#define TRACE_SEQ_BUF_USED(s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1))

min_t?
maybe this should be FIELD_SIZEOF(struct trace_seq, buffer) - 1

I'm not going to waist bandwidth reposting the entire series. Here's a new version of this patch. I'm getting ready to pull it into my next queue.

-- Steve

From 11674c8df0580a03a2517e3a8e4705c47c663564 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 25 Jun 2014 15:54:42 -0400
Subject: [PATCH] tracing: Create seq_buf layer in trace_seq

Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.

Link http://lkml.kernel.org/r/[email protected] open here

Tested-by Jiri Kosina <[email protected]>

Acked-by Jiri Kosina <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 include/linux/seq_buf.h              |  78 ++++++++
 include/linux/trace_seq.h            |  10 +-
 kernel/trace/Makefile                |   1 +
 kernel/trace/seq_buf.c               | 341 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace.c                 |  39 ++--
 kernel/trace/trace_events.c          |   6 +-
 kernel/trace/trace_functions_graph.c |   6 +-
 kernel/trace/trace_seq.c             | 184 +++++++++----------
 8 files changed, 540 insertions(+), 125 deletions(-)
 create mode 100644 include/linux/seq_buf.h
 create mode 100644 kernel/trace/seq_buf.c

include/linux/seq_buf.h new file +46 -0

new file mode 100644
index 000000000000..64bf5a43411e
@@ -0,0 +1,78 @@
+#ifndef _LINUX_SEQ_BUF_H
+#define _LINUX_SEQ_BUF_H
+
+#include <linux/fs.h>
+
+/*
+ * Trace sequences are used to allow a function to call several other functions
+ * to create a string of data to use.
+ */
+
+/**
+ * seq_buf - seq buffer structure
+ * @buffer:	pointer to the buffer
+ * @size:	size of the buffer
+ * @len:	the amount of data inside the buffer
+ * @readpos:	The next position to read in the buffer.
+ */
+struct seq_buf {
+	unsigned char		*buffer;
+	unsigned int		size;
+	unsigned int		len;
+	unsigned int		readpos;
+};
+
+static inline void
+seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
+{
+	s->buffer = buf;
+	s->size = size;
+	s->len = 0;
+	s->readpos = 0;
+}
+
+/*
+ * seq_buf have a buffer that might overflow. When this happens
+ * the len and size are set to be equal.
+ */
+static inline bool
+seq_buf_has_overflowed(struct seq_buf *s)
+{
+	return s->len == s->size;
+}
+
+static inline void
+seq_buf_set_overflow(struct seq_buf *s)
+{

kernel/trace/seq_buf.c new file +0 -0

new file mode 100644
index 000000000000..88738b200bf3

kernel/trace/seq_buf.c new file +15 -0

+
+/**
+ * seq_buf_bitmask - write a bitmask array in its ASCII representation
+ * @s:		seq_buf descriptor
+ * @maskp:	points to an array of unsigned longs that represent a bitmask
+ * @nmaskbits:	The number of bits that are valid in @maskp
+ *
+ * Writes a ASCII representation of a bitmask string into @s.
+ *
+ * Returns zero on success, -1 on overflow.
+ */
+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = seq_buf_buffer_left(s);

kernel/trace/seq_buf.c new file +9 -0

+		if (ret < len) {
+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+

kernel/trace/trace_seq.c +0 -0

index 1f24ed99dca2..3ad8738aea19 100644

kernel/trace/trace_seq.c +1 -1

@@ -144,23 +160,24 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
  */
 int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c +1 -1

 }
 EXPORT_SYMBOL_GPL(trace_seq_vprintf);
 

@@ -183,23 +200,24 @@ EXPORT_SYMBOL_GPL(trace_seq_vprintf);
  */
 int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c +0 -0

 }
 EXPORT_SYMBOL_GPL(trace_seq_bprintf);
 

/**
* trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
* @s: trace sequence descriptor

kernel/trace/trace_seq.c +2 -23

@@ -309,35 +328,30 @@ EXPORT_SYMBOL_GPL(trace_seq_putmem);
 int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 			 unsigned int len)
 {
-	unsigned char hex[HEX_CHARS];
-	const unsigned char *data = mem;
-	unsigned int start_len;
-	int i, j;
-	int cnt = 0;
+	unsigned int save_len = s->seq.len;
+	int ret;
 
 	if (s->full)
 		return 0;
 
-	while (len) {
-		start_len = min(len, HEX_CHARS - 1);
-#ifdef __BIG_ENDIAN
-		for (i = 0, j = 0; i < start_len; i++) {
-#else
-		for (i = start_len-1, j = 0; i >= 0; i--) {
-#endif
-			hex[j++] = hex_asc_hi(data[i]);
-			hex[j++] = hex_asc_lo(data[i]);
-		}
-		if (WARN_ON_ONCE(j == 0 || j/2 > len))
-			break;
-
-		/* j increments twice per loop */
-		len -= j / 2;
-		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);

kernel/trace/trace_seq.c +4 -2

 }
 EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
 

@@ -355,30 +369,28 @@ EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
  */
 int trace_seq_path(struct trace_seq *s, const struct path *path)
 {
-	unsigned char *p;
+	unsigned int save_len = s->seq.len;
+	int ret;
 
 	if (s->full)
 		return 0;
 
+	__trace_seq_init(s);
+
 	if (TRACE_SEQ_BUF_LEFT(s) < 1) {
 		s->full = 1;
 		return 0;
 	}
 
-	p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);

include/linux/seq_buf.h line 56

Quoted 7 lines

include/linux/seq_buf.h new file +7 -0

+/*
+ * How much buffer is left on the seq_buf?
+ */
+static inline unsigned int
+seq_buf_buffer_left(struct seq_buf *s)
+{
+	return (s->size - 1) - s->len;

This should be

	if (seq_buf_has_overflowed(s)
		return 0;
	return (s->size - 1) - s->len;

otherwise, it would return UNIT_MAX for the overflown state. If I am not mistaken.

[...]

Quoted 5 lines

kernel/trace/seq_buf.c new file +0 -0

new file mode 100644
index 000000000000..88738b200bf3

[...]

Quoted 15 lines

kernel/trace/seq_buf.c new file +15 -0

+
+/**
+ * seq_buf_bitmask - write a bitmask array in its ASCII representation
+ * @s:		seq_buf descriptor
+ * @maskp:	points to an array of unsigned longs that represent a bitmask
+ * @nmaskbits:	The number of bits that are valid in @maskp
+ *
+ * Writes a ASCII representation of a bitmask string into @s.
+ *
+ * Returns zero on success, -1 on overflow.
+ */
+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = seq_buf_buffer_left(s);

kernel/trace/seq_buf.c line 107

Quoted 10 lines

kernel/trace/seq_buf.c new file +10 -0

+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	/*
+	 * The last byte of the buffer is used to determine if we
+	 * overflowed or not.
+	 */
+	if (len > 1) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It should be:

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

otherwise, we would write to the beginning to the buffer.

Quoted 10 lines

kernel/trace/seq_buf.c new file +9 -0

+		if (ret < len) {
+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+

[...]

Quoted 4 lines

kernel/trace/trace_seq.c +0 -0

index 1f24ed99dca2..3ad8738aea19 100644

[...]

Quoted 6 lines

kernel/trace/trace_seq.c +1 -1

@@ -144,23 +160,24 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
  */
 int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 171

Quoted 10 lines

kernel/trace/trace_seq.c +4 -2

 	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return 0;
 
-	ret = vsnprintf(s->buffer + s->len, len, fmt, args);
+	__trace_seq_init(s);
+
+	ret = seq_buf_vprintf(&s->seq, fmt, args);

Note that this returns 0 on success => we do not need to store it

Quoted 8 lines

kernel/trace/trace_seq.c +2 -1

 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return 0;
 	}
 

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 180

Quoted 4 lines

kernel/trace/trace_seq.c +1 -3

-	s->len += ret;
-
-	return len;
+	return ret;

Instead, we have to do something like:

	return s->seq.len - save_len;

Quoted 10 lines

kernel/trace/trace_seq.c +1 -1

 }
 EXPORT_SYMBOL_GPL(trace_seq_vprintf);
 

@@ -183,23 +200,24 @@ EXPORT_SYMBOL_GPL(trace_seq_vprintf);
  */
 int trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_vprintf); · line 211

Quoted 10 lines

kernel/trace/trace_seq.c +4 -2

 	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return 0;
 
-	ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
+	__trace_seq_init(s);
+
+	ret = seq_buf_bprintf(&s->seq, fmt, binary);

Same here, it returns 0 on success => no need to store.

Quoted 8 lines

kernel/trace/trace_seq.c +2 -1

 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return 0;
 	}
 

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 180

Quoted 4 lines

kernel/trace/trace_seq.c +1 -3

-	s->len += ret;
-
-	return len;
+	return ret;

and

	return s->seq.len - save_len;

Quoted 3 lines

kernel/trace/trace_seq.c +0 -0

 }
 EXPORT_SYMBOL_GPL(trace_seq_bprintf);
 

[...]

Quoted 36 lines

/**
* trace_seq_putmem_hex - write raw memory into the buffer in ASCII hex
* @s: trace sequence descriptor

kernel/trace/trace_seq.c +2 -23

@@ -309,35 +328,30 @@ EXPORT_SYMBOL_GPL(trace_seq_putmem);
 int trace_seq_putmem_hex(struct trace_seq *s, const void *mem,
 			 unsigned int len)
 {
-	unsigned char hex[HEX_CHARS];
-	const unsigned char *data = mem;
-	unsigned int start_len;
-	int i, j;
-	int cnt = 0;
+	unsigned int save_len = s->seq.len;
+	int ret;
 
 	if (s->full)
 		return 0;
 
-	while (len) {
-		start_len = min(len, HEX_CHARS - 1);
-#ifdef __BIG_ENDIAN
-		for (i = 0, j = 0; i < start_len; i++) {
-#else
-		for (i = start_len-1, j = 0; i >= 0; i--) {
-#endif
-			hex[j++] = hex_asc_hi(data[i]);
-			hex[j++] = hex_asc_lo(data[i]);
-		}
-		if (WARN_ON_ONCE(j == 0 || j/2 > len))
-			break;
-
-		/* j increments twice per loop */
-		len -= j / 2;
-		hex[j++] = ' ';
-
-		cnt += trace_seq_putmem(s, hex, j);

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_putmem); · line 346

Quoted 10 lines

kernel/trace/trace_seq.c +10 -0

+	__trace_seq_init(s);
+
+	/* Each byte is represented by two chars */
+	if (len * 2 > TRACE_SEQ_BUF_LEFT(s)) {
+		s->full = 1;
+		return 0;
+	}
+
+	/* The added spaces can still cause an overflow */
+	ret = seq_buf_putmem_hex(&s->seq, mem, len);

and here, it returns zero on success

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_putmem); · line 354

Quoted 9 lines

kernel/trace/trace_seq.c +7 -1

+
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
+		s->full = 1;
+		return 0;
 	}
-	return cnt;
+
+	return ret;

Therefore we need something like:

	return s->seq.len - save_len;

Quoted 22 lines

kernel/trace/trace_seq.c +4 -2

 }
 EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
 

@@ -355,30 +369,28 @@ EXPORT_SYMBOL_GPL(trace_seq_putmem_hex);
  */
 int trace_seq_path(struct trace_seq *s, const struct path *path)
 {
-	unsigned char *p;
+	unsigned int save_len = s->seq.len;
+	int ret;
 
 	if (s->full)
 		return 0;
 
+	__trace_seq_init(s);
+
 	if (TRACE_SEQ_BUF_LEFT(s) < 1) {
 		s->full = 1;
 		return 0;
 	}
 
-	p = d_path(path, s->buffer + s->len, PAGE_SIZE - s->len);

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_putmem_hex); · line 385

Quoted 10 lines

kernel/trace/trace_seq.c +1 -9

-	if (!IS_ERR(p)) {
-		p = mangle_path(s->buffer + s->len, p, "\n");
-		if (p) {
-			s->len = p - s->buffer;
-			return 1;
-		}
-	} else {
-		s->buffer[s->len++] = '?';
-		return 1;
+	ret = seq_buf_path(&s->seq, path);

This returns zero on sucess.

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_putmem_hex); · line 393

Quoted 9 lines

kernel/trace/trace_seq.c +5 -2

+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
+		s->full = 1;
+		return 0;
 	}
 
-	s->full = 1;
-	return 0;
+	return ret;

and we want to return 1 on success =>

	return 1;

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_putmem_hex); · line 395

Quoted 2 lines

kernel/trace/trace_seq.c +0 -0

 }
 EXPORT_SYMBOL_GPL(trace_seq_path);

Best Regards,
Petr

PS: I have to leave for a bit now. I hope that I will be able to look at the other patches later today.

Quoted 14 lines · On Mon, 10 Nov 2014 14:53:30 +0100 Petr Mladek <[email protected]> wrote:

include/linux/seq_buf.h line 56

Quoted 7 lines

include/linux/seq_buf.h new file +7 -0

+/*
+ * How much buffer is left on the seq_buf?
+ */
+static inline unsigned int
+seq_buf_buffer_left(struct seq_buf *s)
+{
+	return (s->size - 1) - s->len;

This should be

	if (seq_buf_has_overflowed(s)
		return 0;
	return (s->size - 1) - s->len;

Quoted 2 lines

otherwise, it would return UNIT_MAX for the overflown state. If I am not mistaken.

I guess I could add that. Probably the safer bet. Or document it that this is undefined if buffer has overflowed. I have to check how my use cases worked.

Probably best to add the overflow check anyway.

Quoted 38 lines

[...]

Quoted 5 lines

kernel/trace/seq_buf.c new file +0 -0

new file mode 100644
index 000000000000..88738b200bf3

[...]

Quoted 15 lines

kernel/trace/seq_buf.c new file +15 -0

+
+/**
+ * seq_buf_bitmask - write a bitmask array in its ASCII representation
+ * @s:		seq_buf descriptor
+ * @maskp:	points to an array of unsigned longs that represent a bitmask
+ * @nmaskbits:	The number of bits that are valid in @maskp
+ *
+ * Writes a ASCII representation of a bitmask string into @s.
+ *
+ * Returns zero on success, -1 on overflow.
+ */
+int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
+		    int nmaskbits)
+{
+	unsigned int len = seq_buf_buffer_left(s);

kernel/trace/seq_buf.c line 107

Quoted 10 lines

kernel/trace/seq_buf.c new file +10 -0

+	int ret;
+
+	WARN_ON(s->size == 0);
+
+	/*
+	 * The last byte of the buffer is used to determine if we
+	 * overflowed or not.
+	 */
+	if (len > 1) {
+		ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);

It should be:

Quoted 4 lines

		ret = bitmap_scnprintf(s->buffer + s->len, len,
				       maskp, nmaskbits);

otherwise, we would write to the beginning to the buffer.

You are correct. But I'll make that a separate patch. This is just keeping the bug that was in the original code.

Quoted 47 lines

Quoted 10 lines

kernel/trace/seq_buf.c new file +9 -0

+		if (ret < len) {
+			s->len += ret;
+			return 0;
+		}
+	}
+	seq_buf_set_overflow(s);
+	return -1;
+}
+

[...]

Quoted 4 lines

kernel/trace/trace_seq.c +0 -0

index 1f24ed99dca2..3ad8738aea19 100644

[...]

Quoted 6 lines

kernel/trace/trace_seq.c +1 -1

@@ -144,23 +160,24 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);
  */
 int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
 {
-	unsigned int len = TRACE_SEQ_BUF_LEFT(s);
+	unsigned int save_len = s->seq.len;

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 171

Quoted 10 lines

kernel/trace/trace_seq.c +4 -2

 	int ret;
 
-	if (s->full || !len)
+	if (s->full)
 		return 0;
 
-	ret = vsnprintf(s->buffer + s->len, len, fmt, args);
+	__trace_seq_init(s);
+
+	ret = seq_buf_vprintf(&s->seq, fmt, args);

Note that this returns 0 on success => we do not need to store it

Quoted 8 lines

kernel/trace/trace_seq.c +2 -1

 	/* If we can't write it all, don't bother writing anything */
-	if (ret >= len) {
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
 		s->full = 1;
 		return 0;
 	}
 

Quoted 8 lines

kernel/trace/trace_seq.c EXPORT_SYMBOL_GPL(trace_seq_bitmask); · line 180

Quoted 4 lines

kernel/trace/trace_seq.c +1 -3

-	s->len += ret;
-
-	return len;
+	return ret;

Instead, we have to do something like:

	return s->seq.len - save_len;

Actually, I need to make the trace_seq_*() functions return the same as the seq_buf_*() functions.