Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 2 Apr 2015 16:23:18 +0200
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Ian Lepore <ian@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, Gleb Smirnoff <glebius@FreeBSD.org>, src-committers@freebsd.org
Subject:   Re: svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf
Message-ID:  <20150402142318.GC549@dft-labs.eu>
In-Reply-To: <1427983109.82583.115.camel@freebsd.org>
References:  <201504012226.t31MQedN044443@svn.freebsd.org> <1427929676.82583.103.camel@freebsd.org> <20150402123522.GC64665@FreeBSD.org> <20150402133751.GA549@dft-labs.eu> <20150402134217.GG64665@FreeBSD.org> <20150402135157.GB549@dft-labs.eu> <1427983109.82583.115.camel@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Apr 02, 2015 at 07:58:29AM -0600, Ian Lepore wrote:
> On Thu, 2015-04-02 at 15:51 +0200, Mateusz Guzik wrote:
> > On Thu, Apr 02, 2015 at 04:42:17PM +0300, Gleb Smirnoff wrote:
> > > On Thu, Apr 02, 2015 at 03:37:51PM +0200, Mateusz Guzik wrote:
> > > M> For this particular use-case you never care what CPU you are executing
> > > M> on, you only want to obtain a unique number.
> > > M> 
> > > M> per-cpu counters can serve this purpose no problem, just provide an
> > > M> operation which guarantees to return the new value of the counter it
> > > M> incremented. Should be easily achieved with e.g. just pinning curthread
> > > M> to the cpu it executes on for the duration of inc + fetch.
> > > 
> > > I'd ask to pay attention to this particular email:
> > > 
> > > https://lists.freebsd.org/pipermail/svn-src-head/2015-March/069966.html
> > > 
> > > Just to justify probabilities, risks and countermeasures.
> > > 
> > > For those, who don't believe in theory and prefers practice:
> > > 
> > > https://lists.freebsd.org/pipermail/svn-src-head/2015-March/070091.html
> > > 
> > > Note that Emeric was the one who observed collisions for the ip_id++
> > > code, that we used before.
> > > 
> > 
> > Well in that case this at least deserves a comment in the code. Everyone
> > spotting that counter_u64_add + zpcpu_get will think it's a bug.
> > 
> 
> Because it IS a bug.  That isn't changed by the fact that it works
> reliably on one platform due to what should be an opque implementation
> detail, and works by accident on other platforms (at least until the
> details of their implementations change in the future).
> 
> As soon as somebody sees this code, thinks it's a good way to do things,
> and cut and pastes it into another venue and removes the & 0xffff, it
> just turns into a bug on every platform except amd64.
> 

Well, a thread migrating to another cpu is the standard thing everyone
sees.

Are you referring to the fact that the counter is 64-bit, so 32-bit
arches must perform 2 reads and the thread can be migrated in-between?

Indeed that would look like a solid problem, mitigated 'by accident'
with 0xffff.

I don't feel that strongly about changing the code. If it stays as it
is, it definitely needs the comment I mentioned explaining why migration
is fine.

If the code was to be changed a machdep counter_u64_fetchadd seems like
the only course of action.

-- 
Mateusz Guzik <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150402142318.GC549>