On Sat, Nov 12, 2022 at 06:01:36PM +0200, Julian Anastasov wrote: > > Hello, > > On Sat, 12 Nov 2022, Jiri Wiesner wrote: > > > On Sat, Nov 12, 2022 at 10:00:01AM +0100, Jiri Wiesner wrote: > > > On Mon, Oct 31, 2022 at 04:56:43PM +0200, Julian Anastasov wrote: > > > > Use the provided u64_stats_t type to avoid > > > > load/store tearing. > > > > I think only 32-bit machines have a problem storing a 64-bit counter atomically. u64_stats_fetch_begin() and u64_stats_fetch_retry() should take care of that. > > Some compilers for 64-bit can use two 32-bit > loads (load tearing) while we require atomic 64-bit read > in case reader is interrupted by updater. That is why READ_ONCE > is used now. I don't know which compilers can do this. That is > why I switched to u64_stats_t to correctly use the u64_stats > interface. And our data is 8-byte aligned. 32-bit are using > seqcount_t, so they are protected for this problem. All right. It is counter-intuitive but I guess those compiles have their reasons. I think it would not hurt to expand the description of the patch with the explanation above. > > > Converting the per cpu stat to u64_stats_t means that the compiler cannot optimize the memory access and addition on x86_64. Previously, the summation of per cpu counters in ip_vs_chain_estimation() looked like: > > > 15b65: add (%rsi),%r14 > > > 15b68: add 0x8(%rsi),%r15 > > > 15b6c: add 0x10(%rsi),%r13 > > > 15b70: add 0x18(%rsi),%r12 > > > 15b74: add 0x20(%rsi),%rbp > > > The u64_stats_read() calls in ip_vs_chain_estimation() turned it into: > > > 159d5: mov (%rcx),%r11 > > > 159d8: mov 0x8(%rcx),%r10 > > > 159dc: mov 0x10(%rcx),%r9 > > > 159e0: mov 0x18(%rcx),%rdi > > > 159e4: mov 0x20(%rcx),%rdx > > > 159e8: add %r11,%r14 > > > 159eb: add %r10,%r13 > > > 159ee: add %r9,%r12 > > > 159f1: add %rdi,%rbp > > > 159f4: add %rdx,%rbx > > > I guess that is not a big deal because the mov should be the instruction taking the most time on account of accessing per cpu regions of other CPUs. The add will be fast. > > > > Another concern is the number of registers needed for the summation. Previously, 5 registers were needed. Now, 10 registers are needed. This would matter mostly if the size of the stack frame of ip_vs_chain_estimation() and the number of callee-saved registers stored to the stack changed, but introducing u64_stats_read() in ip_vs_chain_estimation() did not change that. > > It happens in this way probably because compiler should not > reorder two or more successive instances of READ_ONCE (rwonce.h), > something that hurts u64_stats_read(). As result, it multiplies > up to 5 times the needed temp storage (registers or stack). > > Another option would be to use something like > this below, try on top of v6. It is rude to add ifdefs here but > we should avoid unneeded code for 64-bit. On 32-bit, code is > more but operations are same. On 64-bit it should order > read+add one by one which can run in parallel. In my compile > tests it uses 3 times movq(READ_ONCE)+addq(to sum) with same temp > register which defeats fully parallel operations and for the > last 2 counters uses movq,movq,addq,addq with different > registers which can run in parallel. It seems there are no free > registers to make it for all 5 counters. > > With this patch the benefit is that compiler should > not hold all 5 values before adding them but on > x86 this patch shows some problem: it does not allocate > 5 new temp registers to read and add the values in parallel. > Without this patch, as you show it, it somehow allocates 5+5 > registers which allows parallel operations. > > If you think we are better with such change, we > can include it in patch 4. It will be better in case one day > we add more counters and there are more available registers. > You can notice the difference probably by comparing the > chain_max value in performance mode. I have tested the change to ip_vs_chain_estimation() and ran profiling with bus-cycles, which are proportional to time spent executing code. The number of estimator per kthread was the same in both tests - 88800. It turns out the change made the code slower: > # Util1 Util2 Diff Command Shared Object Symbol CPU > 1,124,932,796 1,143,867,951 18,935,155 ( 1.7%) ipvs-e:0:0 [ip_vs] all all > 16,480,274 16,853,752 373,478 ( 2.3%) ipvs-e:0:1 [ip_vs] all all > 0 21,987 21,987 (100.0%) ipvs-e:0:0 [kvm] all all > 4,622,992 4,366,150 -256,842 ( 5.6%) ipvs-e:0:1 [kernel.kallsyms] all all > 180,226,857 164,665,271 -15,561,586 ( 8.6%) ipvs-e:0:0 [kernel.kallsyms] all all The most significant component was the time spent in ip_vs_chain_estimation(): > # Util1 Util2 Diff Command Shared Object Symbol CPU > 1,124,414,110 1,143,352,233 18,938,123 ( 1.7%) ipvs-e:0:0 [ip_vs] ip_vs_chain_estimation all > 16,291,044 16,574,498 283,454 ( 1.7%) ipvs-e:0:1 [ip_vs] ip_vs_chain_estimation all > 189,230 279,254 90,024 ( 47.6%) ipvs-e:0:1 [ip_vs] ip_vs_estimation_kthread all > 518,686 515,718 -2,968 ( 0.6%) ipvs-e:0:0 [ip_vs] ip_vs_estimation_kthread all > 1,562,512 1,377,609 -184,903 ( 11.8%) ipvs-e:0:0 [kernel.kallsyms] kthread_should_stop all > 2,435,803 2,138,404 -297,399 ( 12.2%) ipvs-e:0:1 [kernel.kallsyms] _find_next_bit all > 16,304,577 15,786,553 -518,024 ( 3.2%) ipvs-e:0:0 [kernel.kallsyms] _raw_spin_lock all > 151,110,969 137,172,160 -13,938,809 ( 9.2%) ipvs-e:0:0 [kernel.kallsyms] _find_next_bit all The disassembly shows it is the mov instructions (there is a skid of 1 instruction) what takes the most time: >Percent | Source code & Disassembly of kcore for bus-cycles (55354 samples, percent: local period) > : ffffffffc0bad980 <ip_vs_chain_estimation>: > v6 patchset v6 patchset + ip_vs_chain_estimation() change >------------------------------------------------------------------------------------------------------- > 0.00 : nopl 0x0(%rax,%rax,1) 0.00 : nopl 0x0(%rax,%rax,1) > 0.00 : push %r15 0.00 : push %r15 > 0.00 : push %r14 0.00 : push %r14 > 0.00 : push %r13 0.00 : push %r13 > 0.00 : push %r12 0.00 : push %r12 > 0.00 : push %rbp 0.00 : push %rbp > 0.00 : push %rbx 0.00 : push %rbx > 0.00 : sub $0x8,%rsp 0.00 : sub $0x8,%rsp > 0.00 : mov (%rdi),%r15 0.00 : mov (%rdi),%r15 > 0.00 : test %r15,%r15 0.00 : test %r15,%r15 > 0.00 : je 0xffffffffc0badaf1 0.00 : je 0xffffffffc0b7faf1 > 0.00 : call 0xffffffff8bcd33a0 0.00 : call 0xffffffffa1ed33a0 > 0.00 : test %al,%al 0.00 : test %al,%al > 0.00 : jne 0xffffffffc0badaf1 0.00 : jne 0xffffffffc0b7faf1 > 0.00 : mov -0x334ccb22(%rip),%esi 0.00 : mov -0x1d29eb22(%rip),%esi > 0.00 : xor %eax,%eax 0.00 : xor %eax,%eax > 0.00 : xor %ebx,%ebx 0.00 : xor %ebx,%ebx > 0.00 : xor %ebp,%ebp 0.00 : xor %ebp,%ebp > 0.00 : xor %r12d,%r12d 0.00 : xor %r12d,%r12d > 0.00 : xor %r13d,%r13d 0.00 : xor %r13d,%r13d > 0.05 : xor %r14d,%r14d 0.08 : xor %r14d,%r14d > 0.00 : jmp 0xffffffffc0bad9f7 0.00 : jmp 0xffffffffc0b7f9f7 > 0.14 : movslq %eax,%rdx 0.08 : movslq %eax,%rdx > 0.00 : mov 0x68(%r15),%rcx 0.00 : mov 0x68(%r15),%rcx > 11.92 : add $0x1,%eax 12.25 : add $0x1,%eax > 0.00 : add -0x72ead560(,%rdx,8),%rcx 0.00 : add -0x5ccad560(,%rdx,8),%rcx > 4.07 : mov (%rcx),%r11 3.84 : mov (%rcx),%rdx > 34.28 : mov 0x8(%rcx),%r10 34.17 : add %rdx,%r14 > 10.35 : mov 0x10(%rcx),%r9 2.62 : mov 0x8(%rcx),%rdx > 7.28 : mov 0x18(%rcx),%rdi 9.33 : add %rdx,%r13 > 7.70 : mov 0x20(%rcx),%rdx 1.82 : mov 0x10(%rcx),%rdx > 6.80 : add %r11,%r14 6.11 : add %rdx,%r12 > 0.27 : add %r10,%r13 1.26 : mov 0x18(%rcx),%rdx > 0.36 : add %r9,%r12 6.00 : add %rdx,%rbp > 2.24 : add %rdi,%rbp 2.97 : mov 0x20(%rcx),%rdx > 1.21 : add %rdx,%rbx 5.78 : add %rdx,%rbx > 0.07 : movslq %eax,%rdx 1.08 : movslq %eax,%rdx > 0.35 : mov $0xffffffff8d6e0860,%rdi 0.00 : mov $0xffffffffa38e0860,%rdi > 2.26 : call 0xffffffff8c16e270 2.62 : call 0xffffffffa236e270 > 0.17 : mov -0x334ccb7c(%rip),%esi 0.11 : mov -0x1d29eb7c(%rip),%esi > 0.00 : cmp %eax,%esi 0.00 : cmp %eax,%esi > 0.00 : ja 0xffffffffc0bad9c3 0.00 : ja 0xffffffffc0b7f9c3 > 0.00 : lea 0x70(%r15),%rdx 0.00 : lea 0x70(%r15),%rdx > 0.00 : mov %rdx,%rdi 0.00 : mov %rdx,%rdi > 0.04 : mov %rdx,(%rsp) 0.05 : mov %rdx,(%rsp) > 0.00 : call 0xffffffff8c74d800 0.00 : call 0xffffffffa294d800 > 0.06 : mov %r14,%rax 0.07 : mov %r14,%rax > 0.00 : sub 0x20(%r15),%rax 0.00 : sub 0x20(%r15),%rax > 9.55 : mov 0x38(%r15),%rcx 8.97 : mov 0x38(%r15),%rcx > 0.01 : mov (%rsp),%rdx 0.03 : mov (%rsp),%rdx The results indicate that the extra add instructions should not matter - memory accesses (the mov instructions) are the bottleneck. -- Jiri Wiesner SUSE Labs
