svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf

[Mateusz Guzik]

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>