On Sun, Mar 29, 2020 at 11:16:47AM -0700, James Bottomley wrote:
> On Sun, 2020-03-29 at 17:50 +0000, George Spelvin wrote:
>> On Sun, Mar 29, 2020 at 07:13:33PM +0200, Takashi Iwai wrote:
>>> Fair enough. But it's still a question whether an open code X * HZ
>>> is good at all...
>>
>> I'm sorry, I don't understand what you mean by "good at all" here.
>> The value computed is exactly the same.
>
> I think he means what the compiler does with it.
>
> We all assume that msecs_to_jiffies is properly optimized so there
> should be no need to open code it like you're proposing. So firstly
> can you produce the assembly that shows the worse output from
> msecs_to_jiffies? If there is a problem, then we should be fixing it
> in msecs_to_jiffies, not adding open coding.
Fair enough. For the two alternative functions:
#include <linux/jiffies.h>
unsigned long timeout1(unsigned seconds)
{
return jiffies + msecs_to_jiffies(seconds * 1000);
}
unsigned long timeout2(unsigned seconds)
{
return jiffies + seconds * HZ;
}
compiled with Kbuild, the object code produced is:
Disassembly of section .text:
0000000000000000 <timeout1>:
0: 69 ff e8 03 00 00 imul $0x3e8,%edi,%edi
6: e8 00 00 00 00 callq b <timeout1+0xb>
7: R_X86_64_PLT32 __msecs_to_jiffies-0x4
b: 49 89 c0 mov %rax,%r8
e: 48 8b 05 00 00 00 00 mov 0x0(%rip),%rax # 15
<timeout1+0x15>
11: R_X86_64_PC32 jiffies-0x4
15: 4c 01 c0 add %r8,%rax
18: c3 retq
19: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
0000000000000020 <timeout2>:
20: 48 8b 15 00 00 00 00 mov 0x0(%rip),%rdx # 27
<timeout2+0x7>
23: R_X86_64_PC32 jiffies-0x4
27: 69 c7 2c 01 00 00 imul $0x12c,%edi,%eax
2d: 48 01 d0 add %rdx,%rax
30: c3 retq
This is the type of code I replaced: code where the number of seconds
is not known at compile time. Notice how the first multiplies by 1000
and then calls __msecs_to_jiffies. The second multiplies by 300 and
makes no function call.
__msecs_to_jiffies (from kernel/time/time.o) is:
0000000000000100 <__msecs_to_jiffies>:
100: 48 b8 fe ff ff ff ff movabs $0x3ffffffffffffffe,%rax
107: ff ff 3f
10a: 85 ff test %edi,%edi
10c: 78 1c js 12a
<__msecs_to_jiffies+0x2a>
10e: b8 9a 99 99 99 mov $0x9999999a,%eax
113: 89 ff mov %edi,%edi
115: 48 0f af f8 imul %rax,%rdi
119: 48 b8 cc cc cc cc 01 movabs $0x1cccccccc,%rax
120: 00 00 00
123: 48 01 f8 add %rdi,%rax
126: 48 c1 e8 21 shr $0x21,%rax
12a: c3 retq
12b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
I didn't try to replace code that uses compile-time constant arguments
such as include/linux/ceph/libceph.h.
