[PATCH] fs: Cleanup string initializations (char[] instead of char *)

[PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Manuel Schölling]

Initializations like 'char *foo = "bar"' will create two variables: a static
string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
will declare a single variable and will end up in shorter
assembly (according to Jeff Garzik on the KernelJanitor's TODO list).

Signed-off-by: Manuel Schölling <manuel.schoelling@gmx.de>
---
 fs/binfmt_misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index b605003..2a10529 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -419,7 +419,7 @@ static void entry_status(Node *e, char *page)
 {
 	char *dp;
 	char *status = "disabled";
-	const char * flags = "flags: ";
+	const char flags[] = "flags: ";
 
 	if (test_bit(Enabled, &e->flags))
 		status = "enabled";
-- 
1.7.10.4

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Mateusz Guzik]

On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Schölling wrote:
> Initializations like 'char *foo = "bar"' will create two variables: a static
> string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> will declare a single variable and will end up in shorter
> assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> 

This is a greatly oversimplifying things, this may or may not happen.

Out of curiosity I checked my kernel on x86-64 and it has this
optimized:

0xffffffffa00a9629 <bm_entry_read+121>: movabs $0x203a7367616c66,%rcx
crash> ascii 0x203a7367616c66
00203a7367616c66: flags: <NUL>


>  fs/binfmt_misc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index b605003..2a10529 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -419,7 +419,7 @@ static void entry_status(Node *e, char *page)
>  {
>  	char *dp;
>  	char *status = "disabled";
> -	const char * flags = "flags: ";
> +	const char flags[] = "flags: ";
>  
>  	if (test_bit(Enabled, &e->flags))
>  		status = "enabled";

This particular function would be better of with removing this variable
and replacing all pairs like:
sprintf(dp, ...);
dp += strlen(...)

with:
dp += sprintf(dp, ...);

-- 
Mateusz Guzik

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Al Viro]

On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Schölling wrote:
> Initializations like 'char *foo = "bar"' will create two variables: a static
> string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> will declare a single variable and will end up in shorter
> assembly (according to Jeff Garzik on the KernelJanitor's TODO list).

The hell it will.  Compare assembler generated e.g. for 32bit x86 before
and after.

>  {
>  	char *dp;
>  	char *status = "disabled";
> -	const char * flags = "flags: ";
> +	const char flags[] = "flags: ";

The first variant puts address of constant array into local variable
(on stack or in a register).  The second one fills local _array_ - the
string itself goes on stack.

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Al Viro]

On Sat, May 17, 2014 at 05:44:28PM +0200, Mateusz Guzik wrote:
> On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Schölling wrote:
> > Initializations like 'char *foo = "bar"' will create two variables: a static
> > string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> > will declare a single variable and will end up in shorter
> > assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> > 
> 
> This is a greatly oversimplifying things, this may or may not happen.
> 
> Out of curiosity I checked my kernel on x86-64 and it has this
> optimized:
> 
> 0xffffffffa00a9629 <bm_entry_read+121>: movabs $0x203a7367616c66,%rcx
> crash> ascii 0x203a7367616c66
> 00203a7367616c66: flags: <NUL>
> 
> 
> >  fs/binfmt_misc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> > index b605003..2a10529 100644
> > --- a/fs/binfmt_misc.c
> > +++ b/fs/binfmt_misc.c
> > @@ -419,7 +419,7 @@ static void entry_status(Node *e, char *page)
> >  {
> >  	char *dp;
> >  	char *status = "disabled";
> > -	const char * flags = "flags: ";
> > +	const char flags[] = "flags: ";
> >  
> >  	if (test_bit(Enabled, &e->flags))
> >  		status = "enabled";
> 
> This particular function would be better of with removing this variable
> and replacing all pairs like:
> sprintf(dp, ...);
> dp += strlen(...)
> 
> with:
> dp += sprintf(dp, ...);

Sigh...  Premature optimisation and all such...  Folks, seriously, if you
want to do something with it, just switch to single_open().  Something like
this (completely untested):

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index b605003..357e421 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -30,6 +30,7 @@
 #include <linux/mount.h>
 #include <linux/syscalls.h>
 #include <linux/fs.h>
+#include <linux/seq_file.h>
 
 #include <asm/uaccess.h>
 
@@ -415,60 +416,47 @@ static int parse_command(const char __user *buffer, size_t count)
 
 /* generic stuff */
 
-static void entry_status(Node *e, char *page)
+static int entry_status(struct seq_file *m, void *v)
 {
-	char *dp;
-	char *status = "disabled";
-	const char * flags = "flags: ";
+	Node *e = m->private;
 
 	if (test_bit(Enabled, &e->flags))
-		status = "enabled";
+		seq_puts(m, "enabled\n");
+	else
+		seq_puts(m, "disabled\n");
 
-	if (!VERBOSE_STATUS) {
-		sprintf(page, "%s\n", status);
-		return;
-	}
+	if (!VERBOSE_STATUS)
+		return 0;
 
-	sprintf(page, "%s\ninterpreter %s\n", status, e->interpreter);
-	dp = page + strlen(page);
+	seq_printf(m, "interpreter %s\n", e->interpreter);
 
 	/* print the special flags */
-	sprintf (dp, "%s", flags);
-	dp += strlen (flags);
-	if (e->flags & MISC_FMT_PRESERVE_ARGV0) {
-		*dp ++ = 'P';
-	}
-	if (e->flags & MISC_FMT_OPEN_BINARY) {
-		*dp ++ = 'O';
-	}
-	if (e->flags & MISC_FMT_CREDENTIALS) {
-		*dp ++ = 'C';
-	}
-	*dp ++ = '\n';
+	seq_puts(m, "flags: ");
+	if (e->flags & MISC_FMT_PRESERVE_ARGV0)
+		seq_putc(m, 'P');
+	if (e->flags & MISC_FMT_OPEN_BINARY)
+		seq_putc(m, 'O');
+	if (e->flags & MISC_FMT_CREDENTIALS)
+		seq_putc(m, 'C');
+	seq_putc(m, '\n');
 
 
 	if (!test_bit(Magic, &e->flags)) {
-		sprintf(dp, "extension .%s\n", e->magic);
+		seq_printf(m, "extension .%s\n", e->magic);
 	} else {
 		int i;
 
-		sprintf(dp, "offset %i\nmagic ", e->offset);
-		dp = page + strlen(page);
-		for (i = 0; i < e->size; i++) {
-			sprintf(dp, "%02x", 0xff & (int) (e->magic[i]));
-			dp += 2;
-		}
+		seq_printf(m, "offset %i\nmagic ", e->offset);
+		for (i = 0; i < e->size; i++)
+			seq_printf(m, "%02x", (__u8)e->magic[i]);
 		if (e->mask) {
-			sprintf(dp, "\nmask ");
-			dp += 6;
-			for (i = 0; i < e->size; i++) {
-				sprintf(dp, "%02x", 0xff & (int) (e->mask[i]));
-				dp += 2;
-			}
+			seq_puts(m, "\nmask ");
+			for (i = 0; i < e->size; i++)
+				seq_printf(m, "%02x", (__u8)e->mask[i]);
 		}
-		*dp++ = '\n';
-		*dp = '\0';
+		seq_putc(m, '\n');
 	}
+	return 0;
 }
 
 static struct inode *bm_get_inode(struct super_block *sb, int mode)
@@ -512,22 +500,9 @@ static void kill_node(Node *e)
 
 /* /<entry> */
 
-static ssize_t
-bm_entry_read(struct file * file, char __user * buf, size_t nbytes, loff_t *ppos)
+static int bm_entry_open(struct inode *inode, struct file *file)
 {
-	Node *e = file_inode(file)->i_private;
-	ssize_t res;
-	char *page;
-
-	if (!(page = (char*) __get_free_page(GFP_KERNEL)))
-		return -ENOMEM;
-
-	entry_status(e, page);
-
-	res = simple_read_from_buffer(buf, nbytes, ppos, page, strlen(page));
-
-	free_page((unsigned long) page);
-	return res;
+	return single_open(file, entry_status, file_inode(file)->i_private);
 }
 
 static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
@@ -556,9 +531,11 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
 }
 
 static const struct file_operations bm_entry_operations = {
-	.read		= bm_entry_read,
+	.open		= bm_entry_open,
+	.release	= single_release,
+	.read		= seq_read,
 	.write		= bm_entry_write,
-	.llseek		= default_llseek,
+	.llseek		= seq_lseek,
 };
 
 /* /register */

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Mateusz Guzik]

On Sat, May 17, 2014 at 06:21:09PM +0100, Al Viro wrote:
> On Sat, May 17, 2014 at 05:44:28PM +0200, Mateusz Guzik wrote:
> > This particular function would be better of with removing this variable
> > and replacing all pairs like:
> > sprintf(dp, ...);
> > dp += strlen(...)
> > 
> > with:
> > dp += sprintf(dp, ...);
> 
> Sigh...  Premature optimisation and all such... (..)

Well, I was interested in getting rid of this error-prone style, which
results in stuff like:
	sprintf(dp, "\nmask ");
	dp += 6;

... and cleaning up the rest for consistency, will note next time.

I'm new to linux and didn't know about seq_ thingy, will grep some more
next time.

-- 
Mateusz Guzik

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Manuel Schoelling]

Thanks for the detailed review of my patches, guys!

I had a look at the assembler code now, too and you are right about
this.
I was misguided by the KernelJanitor's TODO list [1]. If there is
consensus the corresponding paragraph from that list should be removed.

[1] http://kernelnewbies.org/KernelJanitors/Todo

On Sa, 2014-05-17 at 17:53 +0100, Al Viro wrote:
> On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Schölling wrote:
> > Initializations like 'char *foo = "bar"' will create two variables: a static
> > string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> > will declare a single variable and will end up in shorter
> > assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> 
> The hell it will.  Compare assembler generated e.g. for 32bit x86 before
> and after.
> 
> >  {
> >  	char *dp;
> >  	char *status = "disabled";
> > -	const char * flags = "flags: ";
> > +	const char flags[] = "flags: ";
> 
> The first variant puts address of constant array into local variable
> (on stack or in a register).  The second one fills local _array_ - the
> string itself goes on stack.

Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)

[Kevin Easton]

On Sat, May 17, 2014 at 05:53:45PM +0100, Al Viro wrote:
> On Sat, May 17, 2014 at 05:00:18PM +0200, Manuel Sch?lling wrote:
> > Initializations like 'char *foo = "bar"' will create two variables: a static
> > string and a pointer (foo) to that static string. Instead 'char foo[] = "bar"'
> > will declare a single variable and will end up in shorter
> > assembly (according to Jeff Garzik on the KernelJanitor's TODO list).
> 
> The hell it will.  Compare assembler generated e.g. for 32bit x86 before
> and after.
> 
> >  {
> >  	char *dp;
> >  	char *status = "disabled";
> > -	const char * flags = "flags: ";
> > +	const char flags[] = "flags: ";
> 
> The first variant puts address of constant array into local variable
> (on stack or in a register).  The second one fills local _array_ - the
> string itself goes on stack.

Right - if you're going to do this, you ireally want to make the array
static as well:

	static const char flags[] = "flags: ";

(it's very unlikely that you would want a const array that isn't static).

As Al showed though, usually the optimiser will figure things out on its
own, and the pointer variable in the original variant gets optimised away.
There's also optimisations that apply to string literals but don't apply to
arrays, like string literal merging, which mean that the original could
well still be preferred.

    - Kevin