Date: Thu, 24 May 2018 06:44:20 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: "Jonathan T. Looney" <jtl@freebsd.org> Cc: Matthew Macy <mmacy@freebsd.org>, John Baldwin <jhb@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r334104 - in head/sys: netinet sys Message-ID: <CAGudoHFi6T3tNCy8NUq=oF6h_4=i0cc3peiD%2BE5-NRYKQZX9Tg@mail.gmail.com> In-Reply-To: <CADrOrmtmSYtMt4vrqdFHrLqAArBaws8bAeynPa8X_sz7ui86uw@mail.gmail.com> References: <201805231700.w4NH05hs047395@repo.freebsd.org> <2281830.zrSQodBeDb@ralph.baldwin.cx> <CAPrugNo8_h5jnn2Yt250ZH1crwxHhK46QK1vfdyWssYjuuSAqQ@mail.gmail.com> <CADrOrmtmSYtMt4vrqdFHrLqAArBaws8bAeynPa8X_sz7ui86uw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 24, 2018 at 2:40 AM, Jonathan T. Looney <jtl@freebsd.org> wrote: > On Wed, May 23, 2018 at 7:13 PM, Matthew Macy <mmacy@freebsd.org> wrote: > > > > On Wed, May 23, 2018 at 11:52 AM, John Baldwin <jhb@freebsd.org> wrote: > > > On Wednesday, May 23, 2018 05:00:05 PM Matt Macy wrote: > > >> Author: mmacy > > >> Date: Wed May 23 17:00:05 2018 > > >> New Revision: 334104 > > >> URL: https://svnweb.freebsd.org/changeset/base/334104 > > >> > > >> Log: > > >> epoch: allow for conditionally asserting that the epoch context > fields > > >> are unused by zeroing on INVARIANTS builds > > > > > > Is M_ZERO really so bad that you need to make it conditional? > > > > In this case not at all. It's only exercised by sysctl handlers. I'm > > mostly responding to an inquiry by jtl. However, gratuitous M_ZERO > > usage does have a cumulative adverse performance impact. > > I appreciate you making this change. And, I do think it is worth avoiding > M_ZERO where it is unnecessary, for the reason you state. > > I agree that M_ZERO for no reason should be avoided, especially so with the current implementation of said zeroing (mandatory call to bzero, which is not fast either). > > > > I would probably have preferred something like 'M_ZERO_INVARIANTS' > > > instead perhaps (or M_ZERO_EPOCH) that only controls M_ZERO and is > > > still or'd with M_WAITOK or M_NOWAIT. > > > > Yes. I like that better too. Thanks. > > Yes, that does seem better. > > I fundamentally disagree with this part. If a known value of a given field is needed for assertion purposes, you can add (possibly conditional) code setting this specific value. It probably should not be zero if it can be helped. Conditional zeroing of the *whole* struct depending on invariants will *hide* uninitialized memory read bugs - production kernel will have whatever it happens to find, while *debug* kernel will guarantee to have all the values zeroed. In fact the flag actively combats redzoning. if the resulting allocation is zeroed, poisoning is actively neutered. But only if debug is enabled. That said, I find the change harmful. #define epoch_debug_init or similar can be used here instead. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHFi6T3tNCy8NUq=oF6h_4=i0cc3peiD%2BE5-NRYKQZX9Tg>