Re: [PATCH] cifs: perf improvement - use faster macros ALIGN() and round_up()

From: Enzo Matsumiya <ematsumiya@suse.de>
To: "Aurélien Aptel" <aurelien.aptel@gmail.com>
Cc: linux-cifs@vger.kernel.org, smfrench@gmail.com, pc@cjr.nz,
	ronniesahlberg@gmail.com, nspmangalore@gmail.com
Subject: Re: [PATCH] cifs: perf improvement - use faster macros ALIGN() and round_up()
Date: Wed, 7 Sep 2022 15:15:04 -0300	[thread overview]
Message-ID: <20220907181504.slrof5u42vijakje@cyberdelia> (raw)
In-Reply-To: <CA+5B0FNBJVFC6-SfVodctu0DkyyZ9DzJM8OJsDBbVb453Mvfsw@mail.gmail.com>

Hi Aurelien,

On 09/07, Aurélien Aptel wrote:
>Hi,
>
>Changes are good from a readability stand point but like the others
>I'm very skeptical about the perf improvement claims.

I also am/was. That's why I ran 5 times each test after sending the
patch. For each run, I made sure to have the mount point clean, both
server and client freshly booted, and aside from the patch, the build
env/compile options were exactly the same.

The server had no parallel workload at all as it's only a test server.

>Many of these compile to the same division-less instructions
>especially if any of the values are known at compile time.

<snip>

>> @@ -2826,9 +2819,7 @@ SMB2_open_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
>> -               copy_size = uni_path_len;
>> -               if (copy_size % 8 != 0)
>> -                       copy_size = roundup(copy_size, 8);
>> +               copy_size = round_up(uni_path_len, 8);
>> @@ -4090,7 +4081,7 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
>> -                       *total_len = DIV_ROUND_UP(*total_len, 8) * 8;
>> +                       *total_len = ALIGN(*total_len, 8);
>
>These 2 are also hot paths, but skeptical about the optimizations.

As you point out, SMB2_open_init() was indeed the function with greater
improvement, and as per my measurements, the one that actually impacted
the general latency.

>I've looked at those macros in Compiler Explorer and sure enough they
>compile to the same thing on x86_64.
>Even worse, the optimized versions compile with extra instructions for some:
>
>https://godbolt.org/z/z1xhhW9sj

I did the same comparison on a userspace program and got similar
results, but I didn't bother to check the kernel objects as testing it
was quicker to me. But I'll do it today.

>I suspect the improvements are more likely to be related to caches,
>system load, server load, ...

See above.

>You can try to use perf to make a flamegraph and compare.
>
>Cheers,

Not really used to perf, but will check it out, thanks!

Cheers,

Enzo