[RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Joe Perches]

Instead of converting the 800 or so uses of seq_printf with
a constant format (without a % substitution) to seq_puts,
maybe there's another way to slightly speed up these outputs.

Taking a similar approach to commit abd84d60eb
("tracing: Optimize trace_printk() with one arg to use trace_puts()")
use the preprocessor to convert seq_printf(seq, "string constant")
to seq_puts(seq, "string constant")

By stringifying __VA_ARGS__, we can, at compile time, determine
the number of args that are being passed to seq_printf() and
call seq_puts or seq_printf appropriately.

The actual function definition for seq_printf must now
be enclosed in parenthesis to avoid further macro expansion.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/seq_file.c            |  7 ++++++-
 include/linux/seq_file.h | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 38bb59f..d3a957d 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -405,7 +405,12 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
 }
 EXPORT_SYMBOL(seq_vprintf);
 
-int seq_printf(struct seq_file *m, const char *f, ...)
+/*
+ * seq_printf is also a macro that expands to seq_printf or seq_puts.
+ * This means that the actual function definition must be in parenthesis
+ * to prevent the preprocessor from expanding this function name again.
+ */
+int (seq_printf)(struct seq_file *m, const char *f, ...)
 {
 	int ret;
 	va_list args;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 68a04a3..7255f01 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/cpumask.h>
 #include <linux/nodemask.h>
+#include <linux/stringify.h>
 
 struct seq_operations;
 struct file;
@@ -92,6 +93,29 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
 __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
 __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
 
+/*
+ * A little optimization trick is done here. If there's only one
+ * argument, there's no need to scan the string for printf formats.
+ * seq_puts() will suffice. But how can we take advantage of
+ * using seq_puts() when seq_printf() has only one argument?
+ * By stringifying the args and checking the size we can tell
+ * whether or not there are args. __stringify(__VA_ARGS__) will
+ * turn into "" with a size of 1 when there are no args, anything
+ * else will be bigger. All we need to do is define a string to this,
+ * and then take its size and compare to 1. If it's bigger, use
+ * seq_printf() otherwise, optimize it to seq_puts(). Then just
+ * let gcc optimize the rest.  The actual function definition of
+ * seq_printf must be (seq_printf) to prevent further macro expansion.
+ */
+#define seq_printf(seq, fmt, ...)				\
+do {								\
+	char va_args[] = __stringify(__VA_ARGS__);		\
+	if (sizeof(va_args) > 1)				\
+		seq_printf(seq, fmt, ##__VA_ARGS__);		\
+	else							\
+		seq_puts(seq, fmt);				\
+} while (0)
+
 int seq_path(struct seq_file *, const struct path *, const char *);
 int seq_dentry(struct seq_file *, struct dentry *, const char *);
 int seq_path_root(struct seq_file *m, const struct path *path,

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Bjorn Helgaas]

On Sat, Mar 16, 2013 at 7:50 AM, Joe Perches <joe@perches.com> wrote:
> Instead of converting the 800 or so uses of seq_printf with
> a constant format (without a % substitution) to seq_puts,
> maybe there's another way to slightly speed up these outputs.
>
> Taking a similar approach to commit abd84d60eb
> ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> use the preprocessor to convert seq_printf(seq, "string constant")
> to seq_puts(seq, "string constant")
>
> By stringifying __VA_ARGS__, we can, at compile time, determine
> the number of args that are being passed to seq_printf() and
> call seq_puts or seq_printf appropriately.
>
> The actual function definition for seq_printf must now
> be enclosed in parenthesis to avoid further macro expansion.

This is certainly a neat trick.

But I don't really like the fact that it complicates things for every
future code reader, especially when a trivial change in the caller
would accomplish the same thing.  Do you have any idea how much
performance we would gain in exchange for the complication?

Checkpatch could look for additions of seq_printf() with constant formats.

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/seq_file.c            |  7 ++++++-
>  include/linux/seq_file.h | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 38bb59f..d3a957d 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -405,7 +405,12 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
>  }
>  EXPORT_SYMBOL(seq_vprintf);
>
> -int seq_printf(struct seq_file *m, const char *f, ...)
> +/*
> + * seq_printf is also a macro that expands to seq_printf or seq_puts.
> + * This means that the actual function definition must be in parenthesis
> + * to prevent the preprocessor from expanding this function name again.
> + */
> +int (seq_printf)(struct seq_file *m, const char *f, ...)
>  {
>         int ret;
>         va_list args;
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 68a04a3..7255f01 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -7,6 +7,7 @@
>  #include <linux/mutex.h>
>  #include <linux/cpumask.h>
>  #include <linux/nodemask.h>
> +#include <linux/stringify.h>
>
>  struct seq_operations;
>  struct file;
> @@ -92,6 +93,29 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
>  __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
>  __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
>
> +/*
> + * A little optimization trick is done here. If there's only one
> + * argument, there's no need to scan the string for printf formats.
> + * seq_puts() will suffice. But how can we take advantage of
> + * using seq_puts() when seq_printf() has only one argument?
> + * By stringifying the args and checking the size we can tell
> + * whether or not there are args. __stringify(__VA_ARGS__) will
> + * turn into "" with a size of 1 when there are no args, anything
> + * else will be bigger. All we need to do is define a string to this,
> + * and then take its size and compare to 1. If it's bigger, use
> + * seq_printf() otherwise, optimize it to seq_puts(). Then just
> + * let gcc optimize the rest.  The actual function definition of
> + * seq_printf must be (seq_printf) to prevent further macro expansion.
> + */
> +#define seq_printf(seq, fmt, ...)                              \
> +do {                                                           \
> +       char va_args[] = __stringify(__VA_ARGS__);              \
> +       if (sizeof(va_args) > 1)                                \
> +               seq_printf(seq, fmt, ##__VA_ARGS__);            \
> +       else                                                    \
> +               seq_puts(seq, fmt);                             \
> +} while (0)
> +
>  int seq_path(struct seq_file *, const struct path *, const char *);
>  int seq_dentry(struct seq_file *, struct dentry *, const char *);
>  int seq_path_root(struct seq_file *m, const struct path *path,
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Steven Rostedt]

My macro nastiness is contagious ;-)

On Sat, 2013-03-16 at 06:50 -0700, Joe Perches wrote:
> Instead of converting the 800 or so uses of seq_printf with
> a constant format (without a % substitution) to seq_puts,
> maybe there's another way to slightly speed up these outputs.
> 
> Taking a similar approach to commit abd84d60eb
> ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> use the preprocessor to convert seq_printf(seq, "string constant")
> to seq_puts(seq, "string constant")
> 
> By stringifying __VA_ARGS__, we can, at compile time, determine
> the number of args that are being passed to seq_printf() and
> call seq_puts or seq_printf appropriately.
> 
> The actual function definition for seq_printf must now
> be enclosed in parenthesis to avoid further macro expansion.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/seq_file.c            |  7 ++++++-
>  include/linux/seq_file.h | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 38bb59f..d3a957d 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -405,7 +405,12 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
>  }
>  EXPORT_SYMBOL(seq_vprintf);
>  
> -int seq_printf(struct seq_file *m, const char *f, ...)
> +/*
> + * seq_printf is also a macro that expands to seq_printf or seq_puts.
> + * This means that the actual function definition must be in parenthesis
> + * to prevent the preprocessor from expanding this function name again.
> + */
> +int (seq_printf)(struct seq_file *m, const char *f, ...)

That's rather ugly. Why not just #undef seq_printf before defining it?

>  {
>  	int ret;
>  	va_list args;
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 68a04a3..7255f01 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -7,6 +7,7 @@
>  #include <linux/mutex.h>
>  #include <linux/cpumask.h>
>  #include <linux/nodemask.h>
> +#include <linux/stringify.h>
>  
>  struct seq_operations;
>  struct file;
> @@ -92,6 +93,29 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
>  __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
>  __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
>  
> +/*
> + * A little optimization trick is done here. If there's only one
> + * argument, there's no need to scan the string for printf formats.
> + * seq_puts() will suffice. But how can we take advantage of
> + * using seq_puts() when seq_printf() has only one argument?
> + * By stringifying the args and checking the size we can tell
> + * whether or not there are args. __stringify(__VA_ARGS__) will
> + * turn into "" with a size of 1 when there are no args, anything
> + * else will be bigger. All we need to do is define a string to this,
> + * and then take its size and compare to 1. If it's bigger, use
> + * seq_printf() otherwise, optimize it to seq_puts(). Then just
> + * let gcc optimize the rest.  The actual function definition of
> + * seq_printf must be (seq_printf) to prevent further macro expansion.
> + */
> +#define seq_printf(seq, fmt, ...)				\
> +do {								\
> +	char va_args[] = __stringify(__VA_ARGS__);		\

Interesting, how did you not get errors with multiple args? Although, I
did this macro testing in a normal C file and not in the kernel. Maybe I
screwed something up there and it would have worked for me too :-/

Anyway, not making va_args a whacky name is dangerous. This is why I add
those crazy underscores. If someone does:

	var = 1;
	va_args[] = "abc";
	seq_printf(m, "%d %s", var, va_args);

What will be printed is:

	1 var, va_args

That will be very confusing to people.

> +	if (sizeof(va_args) > 1)				\
> +		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> +	else							\
> +		seq_puts(seq, fmt);				\
> +} while (0)

BTW, you need to return a value.


> +
>  int seq_path(struct seq_file *, const struct path *, const char *);
>  int seq_dentry(struct seq_file *, struct dentry *, const char *);
>  int seq_path_root(struct seq_file *m, const struct path *path,
> 

Here's an update to your patch, although I didn't change va_args.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


Index: linux-trace.git/fs/seq_file.c
===================================================================
--- linux-trace.git.orig/fs/seq_file.c
+++ linux-trace.git/fs/seq_file.c
@@ -407,10 +407,11 @@ EXPORT_SYMBOL(seq_vprintf);
 
 /*
  * seq_printf is also a macro that expands to seq_printf or seq_puts.
- * This means that the actual function definition must be in parenthesis
- * to prevent the preprocessor from expanding this function name again.
+ * Undefine the macro before defining the actual function.
  */
-int (seq_printf)(struct seq_file *m, const char *f, ...)
+#undef seq_printf
+
+int seq_printf(struct seq_file *m, const char *f, ...)
 {
 	int ret;
 	va_list args;
Index: linux-trace.git/include/linux/seq_file.h
===================================================================
--- linux-trace.git.orig/include/linux/seq_file.h
+++ linux-trace.git/include/linux/seq_file.h
@@ -108,13 +108,15 @@ __printf(2, 0) int seq_vprintf(struct se
  * seq_printf must be (seq_printf) to prevent further macro expansion.
  */
 #define seq_printf(seq, fmt, ...)				\
-do {								\
+({								\
 	char va_args[] = __stringify(__VA_ARGS__);		\
+	int _____ret;						\
 	if (sizeof(va_args) > 1)				\
-		seq_printf(seq, fmt, ##__VA_ARGS__);		\
+		_____ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
 	else							\
-		seq_puts(seq, fmt);				\
-} while (0)
+		_____ret = seq_puts(seq, fmt);			\
+	_____ret;						\
+})
 
 int seq_path(struct seq_file *, const struct path *, const char *);
 int seq_dentry(struct seq_file *, struct dentry *, const char *);

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Steven Rostedt]

On Sat, 2013-03-16 at 09:43 -0600, Bjorn Helgaas wrote:

> This is certainly a neat trick.

Thank you ;-)

> 
> But I don't really like the fact that it complicates things for every
> future code reader, especially when a trivial change in the caller
> would accomplish the same thing.  Do you have any idea how much
> performance we would gain in exchange for the complication?

Yeah, it does complicate things a little, but that's what comments are
for. With the comment above the code explaining *exactly* what is
happening, it shouldn't make it difficult for future readers. It may
even inspire them.

It is a trivial change to fix seq_printf("fmt") to seq_puts("fmt"), but
also a maintenance burden. I also find reading "printf" much easier to
read in code than seeing "puts". Imagine:

	seq_printf(m "Format this %s\n", fmt);
	seq_puts(m, "for the following line\n");
	seq_printf(m, "followed by this %d\n", cnt);

It makes it rather ugly to read in the code. Having all users just
default to seq_printf() actually makes the code *easier* to read and to
understand.

Yes, it is complex in one little location that seldom ever changes, and
people seldom even look at. But the result of this change can make it
much more readable throughout the rest of the kernel. When you include
all of the kernel, I think the balance is towards the macro in making
the code easier to read and review. 

I always say, add more complexity in a location that is self contained
and seldom changes, to make things that are all over the place and
changes constantly, less complex.

That was my philosophy for TRACE_EVENT() and my NMI handling. Two very
complex pieces of code, but both self contained, and seldom change.

Also, gcc does this conversion too in userspace. Compile the following
program:

#include <stdio.h>
int main() {
	printf("hello world\n");
	return 0;
}

and do an objdump on it:

  4004c4:       55                      push   %rbp
  4004c5:       48 89 e5                mov    %rsp,%rbp
  4004c8:       48 83 ec 10             sub    $0x10,%rsp
  4004cc:       89 7d fc                mov    %edi,-0x4(%rbp)
  4004cf:       48 89 75 f0             mov    %rsi,-0x10(%rbp)
  4004d3:       bf e8 05 40 00          mov    $0x4005e8,%edi
  4004d8:       e8 db fe ff ff          callq  4003b8 <puts@plt>
  4004dd:       b8 00 00 00 00          mov    $0x0,%eax
  4004e2:       c9                      leaveq 
  4004e3:       c3                      retq   

It converts it to puts!

-- Steve

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Joe Perches]

On Sat, 2013-03-16 at 11:57 -0400, Steven Rostedt wrote:
> My macro nastiness is contagious ;-)

True.

> On Sat, 2013-03-16 at 06:50 -0700, Joe Perches wrote:

> > +int (seq_printf)(struct seq_file *m, const char *f, ...)
> 
> That's rather ugly. Why not just #undef seq_printf before defining it?

The whole thing is ugly, nasty and hackish.
I kinda like it.

But I don't like unnecessary undefs.
The preprocessor doesn't expand (funcname).

> Anyway, not making va_args a whacky name is dangerous. This is why I add
> those crazy underscores. If someone does:
> 
> 	var = 1;
> 	va_args[] = "abc";
> 	seq_printf(m, "%d %s", var, va_args);

The same could be true of fmt and it's
used in lots of macros no?

> What will be printed is:
> 
> 	1 var, va_args
> 
> That will be very confusing to people.

And so be fixed very quickly.

> > +	if (sizeof(va_args) > 1)				\
> > +		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> > +	else							\
> > +		seq_puts(seq, fmt);				\
> > +} while (0)
> 
> BTW, you need to return a value.

Oh, yeah, thanks.

>  #define seq_printf(seq, fmt, ...)				\
> -do {								\
> +({								\
>  	char va_args[] = __stringify(__VA_ARGS__);		\
> +	int _____ret;						\
>  	if (sizeof(va_args) > 1)				\
> -		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> +		_____ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
>  	else							\
> -		seq_puts(seq, fmt);				\
> -} while (0)
> +		_____ret = seq_puts(seq, fmt);			\
> +	_____ret;						\
> +})

It's certainly better as a statement expression,
but I think the underscores are really ugly and
not necessary as ret is locally scoped.

Checkpatch doesn't generally parse strings.
checking strings for % could be done though
I suppose.

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Steven Rostedt]

On Sat, 2013-03-16 at 09:15 -0700, Joe Perches wrote:

> > > +int (seq_printf)(struct seq_file *m, const char *f, ...)
> > 
> > That's rather ugly. Why not just #undef seq_printf before defining it?
> 
> The whole thing is ugly, nasty and hackish.
> I kinda like it.
> 
> But I don't like unnecessary undefs.
> The preprocessor doesn't expand (funcname).

Either way, as long as it's commented.

> 
> > Anyway, not making va_args a whacky name is dangerous. This is why I add
> > those crazy underscores. If someone does:
> > 
> > 	var = 1;
> > 	va_args[] = "abc";
> > 	seq_printf(m, "%d %s", var, va_args);
> 
> The same could be true of fmt and it's
> used in lots of macros no?

No, not at all. Try it. The difference is that 'fmt' is the macro arg
and gets replaced by the pre-processor, where as va_args is what gets
placed into the C code by the C pre-processor. Any variables that's not
a macro argument MUST BE UNIQUE!

> 
> > What will be printed is:
> > 
> > 	1 var, va_args
> > 
> > That will be very confusing to people.
> 
> And so be fixed very quickly.

By changing the caller, it's a bug in the implementation of the macro,
not the user of the macro.

> 
> > > +	if (sizeof(va_args) > 1)				\
> > > +		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> > > +	else							\
> > > +		seq_puts(seq, fmt);				\
> > > +} while (0)
> > 
> > BTW, you need to return a value.
> 
> Oh, yeah, thanks.
> 
> >  #define seq_printf(seq, fmt, ...)				\
> > -do {								\
> > +({								\
> >  	char va_args[] = __stringify(__VA_ARGS__);		\
> > +	int _____ret;						\
> >  	if (sizeof(va_args) > 1)				\
> > -		seq_printf(seq, fmt, ##__VA_ARGS__);		\
> > +		_____ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
> >  	else							\
> > -		seq_puts(seq, fmt);				\
> > -} while (0)
> > +		_____ret = seq_puts(seq, fmt);			\
> > +	_____ret;						\
> > +})
> 
> It's certainly better as a statement expression,
> but I think the underscores are really ugly and
> not necessary as ret is locally scoped.

Let me show you the issue:

#define seq_printf(seq, fmt, ...)			\
({							\
	char va_args[] = __stringify(__VA_ARGS__);		\
	int ret;					\
	if (sizeof(va_args) > 1)			\
		ret = seq_printf(seq, fmt, ##__VA_ARGS__);	\
	else
		ret = seq_puts(seq, fmt);		\
	ret;						\
})

Now lets look at the usage of in the code:

	ret = 1;
	va_args = 5;
	fmt = "hello world\n";
	seq_print(m, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);

Notice that here we have va_args as a integer.

Here's what cpp does to it (adding newlines to make it readable):

	ret = 1;
	va_args = 5;
	fmt = "hello world";
	({
		char va_args[] = "fmt, va_args, ret";
		int ret;
		if (sizeof(va_args) > 1)
			ret = seq_printf(seq, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);
		else
			ret = seq_puts(seq, "my fmt=%s args=%d ret=%d\n");
		ret;
	})

A couple of things here. One, you'll get a gcc warning about ret being
used uninitialized. That should confuse the hell out of the developer.

Then you'll also get a warning about %d expecting a number but the
argument is a string. Another damn confusing thing for developers to
see.

Let me stress it one more time... Any variable names added by a macro,
that's not replaced by the parameters of the macro MUST BE UNIQUE!!!!
Otherwise you will get very difficult hard to debug bugs.

I'm also thinking, as macros may include macros, we may need to define a
macro variable name space policy. Something like:

	char __seq_printf__va_args[] = __stringify(__VA_ARGS__);
	int __seq_printf__ret;

It may make the macro ugly, but it helps keep conflicts in name spacing.

-- Steve

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Joe Perches]

On Sat, 2013-03-16 at 09:43 -0600, Bjorn Helgaas wrote:
> Checkpatch could look for additions of seq_printf() with constant formats.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Joe Perches <joe@perches.com>
---

I don't know what perl version introduced $-[0] and $+[0]
so this may not work with older perl.

 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b28cc38..e5b50c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -628,6 +628,13 @@ sub sanitise_line {
 	return $res;
 }
 
+sub get_quoted_string {
+	my ($line, $rawline) = @_;
+
+	return "" if ($line !~ m/(\"[X]+\")/g);
+	return substr($rawline, $-[0], $+[0] - $-[0]);
+}
+
 sub ctx_statement_block {
 	my ($linenr, $remain, $off) = @_;
 	my $line = $linenr - 1;
@@ -3372,6 +3379,15 @@ sub process {
 			     "struct spinlock should be spinlock_t\n" . $herecurr);
 		}
 
+# check for seq_printf uses that could be seq_puts
+		if ($line =~ /\bseq_printf\s*\(/) {
+			my $fmt = get_quoted_string($line, $rawline);
+			if ($fmt !~ /[^\\]\%/) {
+				WARN("PREFER_SEQ_PUTS",
+				     "Prefer seq_puts to seq_printf\n" . $herecurr);
+			}
+		}
+
 # Check for misused memsets
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Joe Perches]

On Sat, 2013-03-16 at 09:43 -0600, Bjorn Helgaas wrote:
> On Sat, Mar 16, 2013 at 7:50 AM, Joe Perches <joe@perches.com> wrote:
> > Instead of converting the 800 or so uses of seq_printf with
> > a constant format (without a % substitution) to seq_puts,
> > maybe there's another way to slightly speed up these outputs.
> >
> > Taking a similar approach to commit abd84d60eb
> > ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> > use the preprocessor to convert seq_printf(seq, "string constant")
> > to seq_puts(seq, "string constant")
> >
> > By stringifying __VA_ARGS__, we can, at compile time, determine
> > the number of args that are being passed to seq_printf() and
> > call seq_puts or seq_printf appropriately.
> >
> > The actual function definition for seq_printf must now
> > be enclosed in parenthesis to avoid further macro expansion.
> 
> This is certainly a neat trick.
> 
> But I don't really like the fact that it complicates things for every
> future code reader, especially when a trivial change in the caller
> would accomplish the same thing.  Do you have any idea how much
> performance we would gain in exchange for the complication?

Nope.  I believe it's trivial in any case.
I just saw Steven's trace hack and thought of seq_printk.

Is there a real performance sensitive seq_printf anywhere?

It's trivial to replace seq_printf("constant") with
seq_puts but there are over a thousand of them.

It may be better to just leave everything as-is.

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Al Viro]

On Sat, Mar 16, 2013 at 06:50:44AM -0700, Joe Perches wrote:
> Instead of converting the 800 or so uses of seq_printf with
> a constant format (without a % substitution) to seq_puts,
> maybe there's another way to slightly speed up these outputs.
> 
> Taking a similar approach to commit abd84d60eb
> ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> use the preprocessor to convert seq_printf(seq, "string constant")
> to seq_puts(seq, "string constant")
> 
> By stringifying __VA_ARGS__, we can, at compile time, determine
> the number of args that are being passed to seq_printf() and
> call seq_puts or seq_printf appropriately.
> 
> The actual function definition for seq_printf must now
> be enclosed in parenthesis to avoid further macro expansion.

Joe, would you mind showing me a single real-world case where that
"optimization" would really matter?  Adding a module that would
produce a seq_file in procfs, with contents generated by something like
	for(i = 0; i < 4000; i++)
		seq_printf(m, "a");
and application that would keep reading that file in a loop does not count,
TYVM.  Until then,
NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
NAKed-because: GAFL

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Al Viro]

On Sat, Mar 16, 2013 at 10:51:18AM -0700, Joe Perches wrote:
> > This is certainly a neat trick.
> > 
> > But I don't really like the fact that it complicates things for every
> > future code reader, especially when a trivial change in the caller
> > would accomplish the same thing.  Do you have any idea how much
> > performance we would gain in exchange for the complication?
> 
> Nope.  I believe it's trivial in any case.
> I just saw Steven's trace hack and thought of seq_printk.
> 
> Is there a real performance sensitive seq_printf anywhere?

... and _that_ is the question that should've been asked first.

> It's trivial to replace seq_printf("constant") with
> seq_puts but there are over a thousand of them.
> 
> It may be better to just leave everything as-is.

Quite.  Note that it's not equivalent to gcc treatment of printf/puts -
there we have cases when it *is* a real hotpath (and I seriously suspect
that it's in part driven by desire to discourage people from uglifying
source by manual equivalents of that micro-optimization).  Moreover,
glibc printf at least used to be heavy; kernel-side we are nowhere near
that bad.

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Steven Rostedt]

On Sat, 2013-03-16 at 18:01 +0000, Al Viro wrote:
> On Sat, Mar 16, 2013 at 10:51:18AM -0700, Joe Perches wrote:
> > > This is certainly a neat trick.
> > > 
> > > But I don't really like the fact that it complicates things for every
> > > future code reader, especially when a trivial change in the caller
> > > would accomplish the same thing.  Do you have any idea how much
> > > performance we would gain in exchange for the complication?
> > 
> > Nope.  I believe it's trivial in any case.
> > I just saw Steven's trace hack and thought of seq_printk.
> > 
> > Is there a real performance sensitive seq_printf anywhere?
> 
> ... and _that_ is the question that should've been asked first.

I totally agree with you. I've been avoiding the "performance
sensitivity" question because of my narcissistic enjoyment of my macro
cleverness ;-)
And also because I have no "F"'n life.

I just figured someone else will point out the lack of clothes the
Emperor has on.

> 
> > It's trivial to replace seq_printf("constant") with
> > seq_puts but there are over a thousand of them.
> > 
> > It may be better to just leave everything as-is.
> 
> Quite.  Note that it's not equivalent to gcc treatment of printf/puts -
> there we have cases when it *is* a real hotpath (and I seriously suspect
> that it's in part driven by desire to discourage people from uglifying
> source by manual equivalents of that micro-optimization).  Moreover,
> glibc printf at least used to be heavy; kernel-side we are nowhere near
> that bad.

It's also a very hot path in tracing. One reason I only implemented the
macro trick with trace_printk() and not printk() nor seq_printk() is
because I knew those were not hot paths. The reason I created
trace_puts() in the first place, is because I had a bug I was trying to
debug where a trace_printk() would actually make the bug go away. It
added too much of an impact to get the race to trigger. But the
trace_puts() was able to do the trace and still have the bug trigger,
and I was able to debug the problem.

But I enjoyed this conversation while it lasted. Sorry it took up your
time. But it did call to attention that these macros that create
variables should probably have a naming policy to avoid macro traps.

-- Steve

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Andrew Morton]

On Sat, 16 Mar 2013 17:54:47 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sat, Mar 16, 2013 at 06:50:44AM -0700, Joe Perches wrote:
> > Instead of converting the 800 or so uses of seq_printf with
> > a constant format (without a % substitution) to seq_puts,
> > maybe there's another way to slightly speed up these outputs.
> > 
> > Taking a similar approach to commit abd84d60eb
> > ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> > use the preprocessor to convert seq_printf(seq, "string constant")
> > to seq_puts(seq, "string constant")
> > 
> > By stringifying __VA_ARGS__, we can, at compile time, determine
> > the number of args that are being passed to seq_printf() and
> > call seq_puts or seq_printf appropriately.
> > 
> > The actual function definition for seq_printf must now
> > be enclosed in parenthesis to avoid further macro expansion.
> 
> Joe, would you mind showing me a single real-world case where that
> "optimization" would really matter?

There's also a small reduction in code footprint.  We merge less
significant things all day ;)

> NAKed-because: GAFL

Yeah, that macro thing should be nuked from orbit.  Please let's add the
checkpatch rule (resend when convenient with clean changelog, please)
and if people want to send "convert seq_printk to seq_puts" patches to
Jan and he takes them into the trivial tree then all is good.

Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args

[Joe Perches]

On Mon, 2013-03-18 at 13:59 -0700, Andrew Morton wrote:
> On Sat, 16 Mar 2013 17:54:47 +0000 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Sat, Mar 16, 2013 at 06:50:44AM -0700, Joe Perches wrote:
> > > Instead of converting the 800 or so uses of seq_printf with
> > > a constant format (without a % substitution) to seq_puts,
> > > maybe there's another way to slightly speed up these outputs.
> > > 
> > > Taking a similar approach to commit abd84d60eb
> > > ("tracing: Optimize trace_printk() with one arg to use trace_puts()")
> > > use the preprocessor to convert seq_printf(seq, "string constant")
> > > to seq_puts(seq, "string constant")
> > > 
> > > By stringifying __VA_ARGS__, we can, at compile time, determine
> > > the number of args that are being passed to seq_printf() and
> > > call seq_puts or seq_printf appropriately.
> > > 
> > > The actual function definition for seq_printf must now
> > > be enclosed in parenthesis to avoid further macro expansion.
> > 
> > Joe, would you mind showing me a single real-world case where that
> > "optimization" would really matter?
> 
> There's also a small reduction in code footprint.  We merge less
> significant things all day ;)

It's small.  ~1kb x86/32 defconfig
$ size def/vmlinux.o*
   text	   data	    bss	    dec	    hex	filename
9296433  661732 1822196 11780361 b3c109	def/vmlinux.o.new
9297361  661732 1822196 11781289 b3c4a9	def/vmlinux.o.old

> > NAKed-because: GAFL

I'm delighted to have a ViroGAFL score of 2.

> Yeah, that macro thing should be nuked from orbit.

It is kinda cute though.

Perhaps it could become something more generic like:

#define __VA_ARGS__EXIST(...)					\
({								\
	int ret;						\
	char __va_args__exist[] = __stringify(__VA_ARGS__);	\
	ret = sizeof(__va_args__exist) != 1;			\
	ret;							\
})

so the hacky ugliness is confined.

> Please let's add the
> checkpatch rule (resend when convenient with clean changelog, please)
> and if people want to send "convert seq_printk to seq_puts" patches to
> Jan and he takes them into the trivial tree then all is good.

I'll resend it.

[PATCH] checkpatch: Prefer seq_puts to seq_printf

[Joe Perches]

Add a check for seq_printf use with a constant format
without additional arguments.  Suggest seq_puts instead.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b28cc38..e5b50c5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -628,6 +628,13 @@ sub sanitise_line {
 	return $res;
 }
 
+sub get_quoted_string {
+	my ($line, $rawline) = @_;
+
+	return "" if ($line !~ m/(\"[X]+\")/g);
+	return substr($rawline, $-[0], $+[0] - $-[0]);
+}
+
 sub ctx_statement_block {
 	my ($linenr, $remain, $off) = @_;
 	my $line = $linenr - 1;
@@ -3372,6 +3379,15 @@ sub process {
 			     "struct spinlock should be spinlock_t\n" . $herecurr);
 		}
 
+# check for seq_printf uses that could be seq_puts
+		if ($line =~ /\bseq_printf\s*\(/) {
+			my $fmt = get_quoted_string($line, $rawline);
+			if ($fmt !~ /[^\\]\%/) {
+				WARN("PREFER_SEQ_PUTS",
+				     "Prefer seq_puts to seq_printf\n" . $herecurr);
+			}
+		}
+
 # Check for misused memsets
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&