From owner-svn-src-all@FreeBSD.ORG Thu Apr 2 13:52:02 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B6496548; Thu, 2 Apr 2015 13:52:02 +0000 (UTC) Received: from mail-wi0-x22d.google.com (mail-wi0-x22d.google.com [IPv6:2a00:1450:400c:c05::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 54209869; Thu, 2 Apr 2015 13:52:02 +0000 (UTC) Received: by widdi4 with SMTP id di4so79522196wid.0; Thu, 02 Apr 2015 06:52:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=RNyWOZSwijIboKTJHnIkRVzyeAS0xT7qHS0wNRSITGk=; b=DdadLy7esbYI3q2NID3E6choz9XiakEriJiNwh9FdtOkyTGnlgHDL1OrIIn1KCmFyT pJK3xwdj2WgRJB8RnN7CosZnAFnL1w/JHC5vgwUBNkfLlLCsvcIJ9TcavtwDnu4u4fnP N1FjViBRe6Q8pSMRGLv4sBLrH1yKMNTBQ5X4yMhI1OV6IySbqsAxAIuqVVuzFfOajftu wMYfu+Gga+QyJn8C7jvXG7RAC8zyA9mpk7b2vgmoLKLwZDcUv2G8YyPxFzMqzXSiLGlG XmI1G1fQq4jp9IRohJ900QMTYgxmguOmVG5rzHLpfrz9ykdFkUsl0/84MLDmVlssjjCF hmrw== X-Received: by 10.180.107.5 with SMTP id gy5mr25348043wib.0.1427982720720; Thu, 02 Apr 2015 06:52:00 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id cf12sm7368600wjb.10.2015.04.02.06.51.59 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 02 Apr 2015 06:51:59 -0700 (PDT) Date: Thu, 2 Apr 2015 15:51:57 +0200 From: Mateusz Guzik To: Gleb Smirnoff 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: <20150402135157.GB549@dft-labs.eu> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150402134217.GG64665@FreeBSD.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Ian Lepore X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Apr 2015 13:52:02 -0000 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> On Thu, Apr 02, 2015 at 03:35:22PM +0300, Gleb Smirnoff wrote: > M> > On Wed, Apr 01, 2015 at 05:07:56PM -0600, Ian Lepore wrote: > M> > I> > Author: glebius > M> > I> > Date: Wed Apr 1 22:26:39 2015 > M> > I> > New Revision: 280971 > M> > I> > URL: https://svnweb.freebsd.org/changeset/base/280971 > M> > I> > > M> > I> > Log: > M> > I> > o Use new function ip_fillid() in all places throughout the kernel, > M> > I> > where we want to create a new IP datagram. > M> > I> > o Add support for RFC6864, which allows to set IP ID for atomic IP > M> > I> > datagrams to any value, to improve performance. The behaviour is > M> > I> > controlled by net.inet.ip.rfc6864 sysctl knob, which is enabled by > M> > I> > default. > M> > I> > o In case if we generate IP ID, use counter(9) to improve performance. > M> > I> > o Gather all code related to IP ID into ip_id.c. > M> > I> > > M> > I> > Differential Revision: https://reviews.freebsd.org/D2177 > M> > I> > Reviewed by: adrian, cy, rpaulo > M> > I> > Tested by: Emeric POUPON > M> > I> > Sponsored by: Netflix > M> > I> > Sponsored by: Nginx, Inc. > M> > I> > Relnotes: yes > M> > I> > > M> > I> [...] > M> > I> > +void > M> > I> > +ip_fillid(struct ip *ip) > M> > I> > +{ > M> > I> > + > M> > I> > + /* > M> > I> > + * Per RFC6864 Section 4 > M> > I> > + * > M> > I> > + * o Atomic datagrams: (DF==1) && (MF==0) && (frag_offset==0) > M> > I> > + * o Non-atomic datagrams: (DF==0) || (MF==1) || (frag_offset>0) > M> > I> > + */ > M> > I> > + if (V_ip_rfc6864 && (ip->ip_off & htons(IP_DF)) == htons(IP_DF)) > M> > I> > + ip->ip_id = 0; > M> > I> > + else if (V_ip_do_randomid) > M> > I> > + ip->ip_id = ip_randomid(); > M> > I> > + else { > M> > I> > + counter_u64_add(V_ip_id, 1); > M> > I> > + ip->ip_id = htons((*(uint64_t *)zpcpu_get(V_ip_id)) & 0xffff); > M> > I> > + } > M> > I> > +} > M> > I> > + > M> > I> > M> > I> This is completely bogus. It's a big opacity violation (it relies on > M> > I> what should be opaque private internal implementation details of > M> > I> counter(9)). The fact that the counter api doesn't provide a function > M> > I> for retrieving one cpu's counter value should be a big clue there -- the > M> > I> fact that you know the internals doesn't make it okay to reach behind > M> > I> the counter and grab a value like that. It may not even be safe to do > M> > I> so on any given architecture; it certainly isn't safe on arm, and that > M> > I> line of code above will work only by accident because you're throwing > M> > I> way all but 16 bits. > M> > > M> > I though about providing that API, but since it isn't safe in general, > M> > I decided to not do that. > M> > > M> > I> But even more importantly, this WILL result in multiple threads using > M> > I> the same value at the same time... > M> > I> > M> > I> - Thread A on CPU 1 and thread B on CPU 2 both begin executing here at > M> > I> the same time, and both get through counter_u64_add(). > M> > I> - Thread A keeps running and uses CPU 1's new value, call it 27. > M> > I> - Thread B gets prempted between counter_u64_add() and zpcpu_get(). > M> > I> When it resumes it's now on CPU 1, so it retrieves value 27 as well. > M> > > M> > This was already discussed in this thread: > M> > > M> > https://lists.freebsd.org/pipermail/svn-src-head/2015-March/069864.html > M> > > M> > 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. -- Mateusz Guzik