Re: [RFC PATCHv6 3/7] ipvs: use u64_stats_t for the per-cpu counters

	Hello,

On Tue, 15 Nov 2022, Jiri Wiesner wrote:

> On Sat, Nov 12, 2022 at 06:01:36PM +0200, Julian Anastasov wrote:
> > 
> > 	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.

	OK. I relied on the mentioned commit to explain the problem
as other similar patches do.

> > 	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

	chain_max is probably 37 here and 1.7% is low
enough (below 1/37) not to be accounted in chain_max.

> >    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 :       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

	Bad, same register RDX used. I guess, this is
CONFIG_GENERIC_CPU=y kernel which is suitable for different
CPUs. My example was tested with CONFIG_MCORE2=y. But the
optimizations are compiler's job, there can be a cost
to free registers for our operations. Who knows, change
can be faster on other platforms with less available
registers to hold the 5 counters and their sum in registers.

	Probably, we can force it to add mem8 into reg by using
something like this:

static inline void u64_stats_read_add(u64_stats_t *p, u64 *sum)
{
	local64_add_to(&p->v, (long *) sum);
}

static inline void local_add_to(local_t *l, long *sum)
{
	asm(_ASM_ADD "%1,%0"
	    : "+r" (*sum)
	    : "m" (l->a.counter));
}

	But it deserves more testing.

> The results indicate that the extra add instructions should not matter - memory accesses (the mov instructions) are the bottleneck.

	Yep, then lets work without this change for now,
no need to deviate much from the general u64_stats usage
if difference is negligible.

Regards

--
Julian Anastasov <[email protected]>