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>