From owner-svn-src-head@freebsd.org Fri Jul 6 17:57:00 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D60861040729 for ; Fri, 6 Jul 2018 17:56:59 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (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 6AC4F8E945 for ; Fri, 6 Jul 2018 17:56:59 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it0-x242.google.com with SMTP id j185-v6so17992224ite.1 for ; Fri, 06 Jul 2018 10:56:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=09fK4/RfFzq+P1jH+EuUBR3T4ZiLHFMRKvvtalx1G1U=; b=aEvqkOMft+NP+eRwGkKFmpOu47EgnTxz6QxYUR9sBcyaoRdq9TTdLjcAhKQunDonZ8 Ev/OWPxE9ZzkcXwLs0Tou/u5ykX8Gh79PS+EY4FCj0M5aoE5Ed03GMe7leYA1gVkMBv/ gaVs1gSkS3+8R4OVkHnmLYqOEovV1qbpdGVAN9bKechaKp3Y6qgXsBToiDdoYACKZCbK gwF6JuVc4MF+DipU/8dgUTneoqkh+PitS9cS2a9MA7FJeTk8aHSPDocd/9WGajMXkEvB OJrHnHOupLzovcP5U3x00hPsntBBHBwRNidSCVfA833pFJIY6/ethyiqSylUoh7JojIJ tReQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=09fK4/RfFzq+P1jH+EuUBR3T4ZiLHFMRKvvtalx1G1U=; b=kJGYv+fk5s5yvPDFExJOifigMUTp2hP/yBCzlhg0h63lt6UAcVJfG1mY42oMKJUSQt euY36DUzk/oFo+igvk8vPa4mzlHO+F7KKFQ5DVGHYrKWIitQMUFKxviA69rLvVNumpr8 ohziNiOVtcTlFRaU3qSwNnu+ibddD1DoxpEBlJN31eAA80yWs0Db6+u4TIzJDOXIKCzM s9D/HNMlAWlrViC3ELM+B5kMk7ri62jhytd/p9i2ODKERFzgLELvc/AL2gXN38H5h7ze e1nA71NKHw9sl/wq2qi/aXdOppmyR7qFcqXILaV2xC8lWm0jzapUKwAvoqrONhWSq2P4 fZkQ== X-Gm-Message-State: APt69E3ipo39kPe9W9dJKLOwhM0ZsZhkurX3l6zhk2k6WZlGLEpKmvIF A9OiOngp3GLGnUCg4pNUgjWYZ5fDd/RzTkTNCs6LTA== X-Google-Smtp-Source: AAOMgpfXkwPVPO0i1k4GPHEgPdchrCsi3cQeFK+ArcIVCdrMrLe3aBiLLyxAaGOrw90cfryTOCFdX7iPxJng1T+LFMg= X-Received: by 2002:a02:a701:: with SMTP id k1-v6mr9350663jam.140.1530899818657; Fri, 06 Jul 2018 10:56:58 -0700 (PDT) MIME-Version: 1.0 References: <201807061727.w66HR2wY053353@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <201807061727.w66HR2wY053353@pdx.rh.CN85.dnsmgr.net> From: Warner Losh Date: Fri, 6 Jul 2018 11:56:46 -0600 Message-ID: Subject: Re: svn commit: r336025 - in head/sys: amd64/include i386/include To: "Rodney W. Grimes" Cc: Hans Petter Selasky , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.27 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Jul 2018 17:57:00 -0000 On Fri, Jul 6, 2018, 12:27 PM Rodney W. Grimes < freebsd@pdx.rh.cn85.dnsmgr.net> wrote: > > On Fri, Jul 6, 2018 at 9:52 AM, Rodney W. Grimes < > > freebsd@pdx.rh.cn85.dnsmgr.net> wrote: > > > > > > On Fri, Jul 6, 2018 at 9:32 AM, Rodney W. Grimes < > > > > freebsd@pdx.rh.cn85.dnsmgr.net> wrote: > > > > > > > > > > Author: hselasky > > > > > > Date: Fri Jul 6 10:13:42 2018 > > > > > > New Revision: 336025 > > > > > > URL: https://svnweb.freebsd.org/changeset/base/336025 > > > > > > > > > > > > Log: > > > > > > Make sure kernel modules built by default are portable between > UP > > > and > > > > > > SMP systems by extending defined(SMP) to include > > > defined(KLD_MODULE). > > > > > > > > > > > > This is a regression issue after r335873 . > > > > > > > > > > > > Discussed with: mmacy@ > > > > > > Sponsored by: Mellanox Technologies > > > > > > > > > > Though this fixes the issue, it also means that now when > > > > > anyone intentionally builds a UP kernel with modules > > > > > they are getting SMP support in the modules and I am > > > > > not sure they would want that. I know I don't. > > > > > > > > > > > > > > > > > On UP systems, these additional opcodes are harmless. They take a few > > > extra > > > > cycles (since they lock an uncontested bus) and add a couple extra > memory > > > > barriers (which will be NOPs). On MP systems, atomics now work by > > > default. > > > > Had we not defaulted like this, all modules built outside of a kernel > > > build > > > > env would have broken atomics. Given that (a) the overwhelming > majority > > > > (99% or more) is SMP and (b) the MP code merely adds a few cycles to > > > what's > > > > already a not-too-expensive operation, this was the right choice. > > > > > > > > It simply doesn't matter for systems that are relevant to the project > > > > today. While one could try to optimize this a little (for example, by > > > > having SMP defined to be 0 or 1, say, and changing all the ifdef SMP > to > > > if > > > > (defined(SMP) && SMP != 0)), it's likely not going to matter enough > for > > > > anybody to make the effort. UP on x86 is simply not relevant enough > to > > > > optimize for it. Even in VMs, people run SMP kernels typically even > when > > > > they just allocate one CPU to the VM. > > > > > > > > So while we still support the UP config, and we'll let people build > > > > optimized kernels for x86, we've flipped the switch from pessimized > for > > > SMP > > > > modules to pessimized for UP modules, which seems like quite the > > > reasonable > > > > trade-off. > > > > > > > > Were it practical to do so, I'd suggest de-orbiting UP on x86. > However, > > > > it's a lot of work for not much benefit and we'd need to invent much > > > crazy > > > > to get there. > > > > > > Trivial to fix this with > > > +#if defined(SMP) || !defined(_KERNEL) || defined(KLD_MODULE) || > > > !defined(KLD_UP_MODULES) > > > > > > Nope. Not so trivial. Who defines KLD_UP_MODULES? > > Call it SMP_KLD_MODULES, and it gets defined the same place SMP does. > Not so simple. SMP is defined in the config file, and winds up in one of the option files. It will be absent for stand alone builds, though. These change tweak the default yo be inlined and to include the sequence that works everywhere. > > > And really, it's absolutely not worth it unless someone shows up with > > numbers to show the old 'function call to optimal routine' is actually > > faster than the new 'inline to slightly unoptimal code'. Since I think > the > > function call overhead is larger than the pessmizations, I'm not sure > what > > the fuss is about. > > I have no issues with the SMP converting from function calls to > inline locks, I just want to retain the exact same code I had > before any of these changes, and that was A UP built system > without any SMP locking. Is it too much to ask to keep what > already worked? > This doesn't enable or disable locks in the muted sense. It just changes the atomic ops for the kernel from a function call to an inlined function. The inlining is more efficient than the call, even with the overhead added by always inlining the same stuff. It still is faster than before. And userland has done this forever... So I honestly think even UP builds are better off, even if it's not hyper optimized for UP. The lock instruction prefix is minimal overhead (a cycle I think). This is different than the mutexes we optimize for the UP cases (and which aren't affected by this change). It's really not a big deal. Warner > > > > > > > > > Modified: > > > > > > head/sys/amd64/include/atomic.h > > > > > > head/sys/i386/include/atomic.h > > > > > > > > > > > > Modified: head/sys/amd64/include/atomic.h > > > > > > ============================================================ > > > > > ================== > > > > > > --- head/sys/amd64/include/atomic.h Fri Jul 6 10:10:00 2018 > > > > > (r336024) > > > > > > +++ head/sys/amd64/include/atomic.h Fri Jul 6 10:13:42 2018 > > > > > (r336025) > > > > > > @@ -132,7 +132,7 @@ void atomic_store_rel_##TYPE( > > > volatile > > > > > u_##TYPE *p, u_ > > > > > > * For userland, always use lock prefixes so that the binaries > will > > > run > > > > > > * on both SMP and !SMP systems. > > > > > > */ > > > > > > -#if defined(SMP) || !defined(_KERNEL) > > > > > > +#if defined(SMP) || !defined(_KERNEL) || defined(KLD_MODULE) > > > > > > #define MPLOCKED "lock ; " > > > > > > #else > > > > > > #define MPLOCKED > > > > > > @@ -354,7 +354,7 @@ atomic_testandclear_long(volatile u_long *p, > > > u_int > > > > > v) > > > > > > */ > > > > > > #define OFFSETOF_MONITORBUF 0x100 > > > > > > > > > > > > -#if defined(SMP) > > > > > > +#if defined(SMP) || defined(KLD_MODULE) > > > > > > static __inline void > > > > > > __storeload_barrier(void) > > > > > > { > > > > > > > > > > > > Modified: head/sys/i386/include/atomic.h > > > > > > ============================================================ > > > > > ================== > > > > > > --- head/sys/i386/include/atomic.h Fri Jul 6 10:10:00 2018 > > > > > (r336024) > > > > > > +++ head/sys/i386/include/atomic.h Fri Jul 6 10:13:42 2018 > > > > > (r336025) > > > > > > @@ -143,7 +143,7 @@ void atomic_subtract_64(volatile > > > > > uint64_t *, uint64_t > > > > > > * For userland, always use lock prefixes so that the binaries > will > > > run > > > > > > * on both SMP and !SMP systems. > > > > > > */ > > > > > > -#if defined(SMP) || !defined(_KERNEL) > > > > > > +#if defined(SMP) || !defined(_KERNEL) || defined(KLD_MODULE) > > > > > > #define MPLOCKED "lock ; " > > > > > > #else > > > > > > #define MPLOCKED > > > > > > @@ -302,7 +302,7 @@ atomic_testandclear_int(volatile u_int *p, > > > u_int v) > > > > > > */ > > > > > > > > > > > > #if defined(_KERNEL) > > > > > > -#if defined(SMP) > > > > > > +#if defined(SMP) || defined(KLD_MODULE) > > > > > > #define __storeload_barrier() __mbk() > > > > > > #else /* _KERNEL && UP */ > > > > > > #define __storeload_barrier() __compiler_membar() > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Rod Grimes > > > > > rgrimes@freebsd.org > > > > > > > > > > > > > > > > -- > > > Rod Grimes > > > rgrimes@freebsd.org > > > > > -- > Rod Grimes > rgrimes@freebsd.org >