Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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:
>
> 0xa00a9629 : movabs $0x203a7367616c66,%rcx
> crash> ascii 0x203a7367616c66
> 00203a7367616c66: flags:
>
>
> > 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
#include
#include
+#include
#include
@@ -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)
/* / */
-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;
-
Re: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH] fs: Cleanup string initializations (char[] instead of char *)
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:
0xa00a9629 : movabs $0x203a7367616c66,%rcx
crash> ascii 0x203a7367616c66
00203a7367616c66: flags:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fs: Cleanup string initializations (char[] instead of char *)
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
