Re: [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup

[Steven Rostedt]

blktrace is maintained by Jens Axboe (added to the Cc), although I also
maintain it for cleanups such as this. That's the reason it doesn't
have a "trace_" in its name like the other files do (a bit of trivia for
you ;-)

kernel/trace/blktrace.c should probably be included in the MAINTAINERS
file under BLOCK LAYER.


On Thu,  4 Jun 2015 11:37:10 +0200
Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:

> Part of the disassembly of do_blk_trace_setup:
> 
>     231b:       e8 00 00 00 00          callq  2320 <do_blk_trace_setup+0x50>
>                         231c: R_X86_64_PC32     strlen+0xfffffffffffffffc
>     2320:       eb 0a                   jmp    232c <do_blk_trace_setup+0x5c>
>     2322:       66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)
>     2328:       48 83 c3 01             add    $0x1,%rbx
>     232c:       48 39 d8                cmp    %rbx,%rax
>     232f:       76 47                   jbe    2378 <do_blk_trace_setup+0xa8>
>     2331:       41 80 3c 1c 2f          cmpb   $0x2f,(%r12,%rbx,1)
>     2336:       75 f0                   jne    2328 <do_blk_trace_setup+0x58>
>     2338:       41 c6 04 1c 5f          movb   $0x5f,(%r12,%rbx,1)
>     233d:       4c 89 e7                mov    %r12,%rdi
>     2340:       e8 00 00 00 00          callq  2345 <do_blk_trace_setup+0x75>
>                         2341: R_X86_64_PC32     strlen+0xfffffffffffffffc
>     2345:       eb e1                   jmp    2328 <do_blk_trace_setup+0x58>
> 
> Yep, that's right: gcc isn't smart enough to realize that replacing
> '/' by '_' cannot change the strlen(), so we call it again and again
> (at least when a '/' is found). Even if gcc were that smart, this
> construction would still loop over the string twice, once for the
> initial strlen() call and then the open-coded loop.
> 
> Let's simply use strreplace() instead.
> 
> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> ---
>  kernel/trace/blktrace.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 483cecfa5c17..4eeae4674b5a 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -439,7 +439,7 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
>  {
>  	struct blk_trace *old_bt, *bt = NULL;
>  	struct dentry *dir = NULL;
> -	int ret, i;
> +	int ret;
>  
>  	if (!buts->buf_size || !buts->buf_nr)
>  		return -EINVAL;
> @@ -451,9 +451,7 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
>  	 * some device names have larger paths - convert the slashes
>  	 * to underscores for this to work as expected
>  	 */
> -	for (i = 0; i < strlen(buts->name); i++)

Wow! I'm surprised this wasn't:

	for (i = 0; buts->name[i]; i++)

> -		if (buts->name[i] == '/')
> -			buts->name[i] = '_';
> +	strreplace(buts->name, '/', '_');
>  
>  	bt = kzalloc(sizeof(*bt), GFP_KERNEL);
>  	if (!bt)

Acked-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/