netdev - Re: [PATCH 03/18] ipvs: zero percpu stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 06 Mar 2011 10:06:07 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
	netfilter@...r.kernel.org, lvs-devel@...r.kernel.org,
	Julian Anastasov <ja@....bg>,
	Hans Schillstrom <hans@...illstrom.com>
Subject: Re: [PATCH 03/18] ipvs: zero percpu stats

Le dimanche 06 mars 2011 à 08:45 +0900, Simon Horman a écrit :
> From: Julian Anastasov <ja@....bg>
> 
>  	Zero the new percpu stats because we copy from there.
> 
> Signed-off-by: Julian Anastasov <ja@....bg>
> Signed-off-by: Simon Horman <horms@...ge.net.au>
> ---
>  net/netfilter/ipvs/ip_vs_ctl.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index a2a67ad..fd74527 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -715,8 +715,25 @@ static void ip_vs_trash_cleanup(struct net *net)
>  static void
>  ip_vs_zero_stats(struct ip_vs_stats *stats)
>  {
> +	struct ip_vs_cpu_stats *cpustats = stats->cpustats;
> +	int i;
> +
>  	spin_lock_bh(&stats->lock);
>  
> +	for_each_possible_cpu(i) {
> +		struct ip_vs_cpu_stats *u = per_cpu_ptr(cpustats, i);
> +		unsigned int start;
> +
> +		/* Do not pretend to be writer, it is enough to
> +		 * sync with writers that modify the u64 counters
> +		 * because under stats->lock we are the only reader.
> +		 */
> +		do {
> +			start = u64_stats_fetch_begin(&u->syncp);
> +			memset(&u->ustats, 0, sizeof(u->ustats));
> +		} while (u64_stats_fetch_retry(&u->syncp, start));


Sorry this makes no sense to me.

This code _is_ a writer, and hardly a hot path.

Why try to pretend its a reader and confuse people ?

Either :

- Another writer can modify the counters in same time, and we must
synchronize with them (we are a writer after all)

- Another reader can read the counters in same time, and we must let
them catch we mihjt have cleared half of their values.

- No reader or writer can access data, no synch is needed, a pure
memset() is OK.


> +	}
> +
>  	memset(&stats->ustats, 0, sizeof(stats->ustats));
>  	ip_vs_zero_estimator(stats);
>  


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html