netdev - Re: [RFC net-next] net: sched: do not acquire qdisc spinlock in qdisc/class stats dump

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]

Message-ID: <1463723432.18194.274.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Thu, 19 May 2016 22:50:32 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
Cc:	netdev <netdev@...r.kernel.org>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	John Fastabend <john.fastabend@...il.com>,
	Kevin Athey <kda@...gle.com>,
	Xiaotian Pei <xiaotian@...gle.com>
Subject: Re: [RFC net-next] net: sched: do not acquire qdisc spinlock in
 qdisc/class stats dump

On Thu, 2016-05-19 at 22:23 -0700, Cong Wang wrote:
> On Thu, May 19, 2016 at 7:45 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> > On Thu, 2016-05-19 at 18:50 -0700, Cong Wang wrote:
> >> On Thu, May 19, 2016 at 5:35 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> >> >
> >> > These stats are using u64 or u32 fields, so reading integral values
> >> > should not prevent writers from doing concurrent updates if the kernel
> >> > arch is a 64bit one.
> >> >
> >> > Being able to atomically fetch all counters like packets and bytes sent
> >> > at the expense of interfering in fast path (queue and dequeue packets)
> >> > is simply not worth the pain, as the values are generally stale after 1
> >> > usec.
> >>
> >> I think one purpose of this lock is to make sure we have an atomic
> >> snapshot of these counters as a whole. IOW, we may need another
> >> lock rather than the qdisc root lock to guarantee this.
> >
> > Right, this was stated in the changelog.
> >
> > I played a bit at changing qdisc->__state to a seqcount.
> >
> > But this would add 2 additional smp_wmb() barriers.
> >
> > Not sure if any application would actually care if it reads
> >
> > Sent 22954160104777 bytes 1808585171 pkt (dropped 0, overlimits 0
> > requeues 26021)
> >
> > Instead of
> >
> > Sent 22954160104777 bytes 1808585172 pkt (dropped 0, overlimits 0
> > requeues 26021)
> >
> > ?
> 
> Sometimes I use bytes/pkts to valuate the average size of the packets.
> ;) It doesn't have to be that accurate but we don't know how inaccurate
> it is without lock?

It wont be inaccurate (no drift), as the writers hold the qdisc
spinlock.

It it the same with say IP/TCP SNMP counters :

When an incoming frame is handled, it might change many SNMP counters,
but an SNMP agent might fetch the whole set of SNMP values in the middle
of the changes.

IpInReceives
IpInDelivers 
TcpInSegs
IpExtInOctets
IpExtInNoECTPkts
...

The only 'problem' can be a off-by-one (or off-by-bytes-in-the-packet)
transient error, that all SNMP agents are normally handling just fine.