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.
> >
> > Fixes: 316580b69d0a ("u64_stats: provide u64_stats_t type")
> > Signed-off-by: Julian Anastasov <ja@xxxxxx>
> > ---
> > include/net/ip_vs.h | 10 +++++-----
> > net/netfilter/ipvs/ip_vs_core.c | 30 +++++++++++++++---------------
> > net/netfilter/ipvs/ip_vs_ctl.c | 10 +++++-----
> > net/netfilter/ipvs/ip_vs_est.c | 20 ++++++++++----------
> > 4 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> > index e5582c01a4a3..a4d44138c2a8 100644
> > --- a/include/net/ip_vs.h
> > +++ b/include/net/ip_vs.h
> > @@ -351,11 +351,11 @@ struct ip_vs_seq {
> >
> > /* counters per cpu */
> > struct ip_vs_counters {
> > - __u64 conns; /* connections scheduled */
> > - __u64 inpkts; /* incoming packets */
> > - __u64 outpkts; /* outgoing packets */
> > - __u64 inbytes; /* incoming bytes */
> > - __u64 outbytes; /* outgoing bytes */
> > + u64_stats_t conns; /* connections scheduled */
> > + u64_stats_t inpkts; /* incoming packets */
> > + u64_stats_t outpkts; /* outgoing packets */
> > + u64_stats_t inbytes; /* incoming bytes */
> > + u64_stats_t outbytes; /* outgoing bytes */
> > };
> > /* Stats per cpu */
> > struct ip_vs_cpu_stats {
>
> 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.
--
Jiri Wiesner
SUSE Labs
