Date: Fri, 6 Apr 2018 22:15:01 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <royger@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r332092 - in head/sys: amd64/amd64 sys x86/x86 Message-ID: <20180406213913.G2230@besplex.bde.org> In-Reply-To: <201804061120.w36BK6s6074635@repo.freebsd.org> References: <201804061120.w36BK6s6074635@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 6 Apr 2018, [UTF-8] Roger Pau Monn=C3=A9 wrote:
> Log:
> remove GiB/MiB macros from param.h
>
> And instead define them in the files where they are used.
>
> Requested by: bde
Thanks, but these files have a negative need for the macros.
> Modified: head/sys/amd64/amd64/mp_machdep.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- head/sys/amd64/amd64/mp_machdep.c=09Fri Apr 6 09:25:08 2018=09(r3320=
91)
> +++ head/sys/amd64/amd64/mp_machdep.c=09Fri Apr 6 11:20:06 2018=09(r3320=
92)
> @@ -83,6 +83,8 @@ __FBSDID("$FreeBSD$");
> #define BIOS_RESET=09=09(0x0f)
> #define BIOS_WARM=09=09(0x0a)
>
> +#define GiB(v)=09=09=09(v ## ULL << 30)
> +
In this file, the macro is used only once. It takes about 4 times as
much code to define and use the macro once as to write
(vm_paddr_t)4 << 30. Much more than 4 times longer to read, since some
searching is needed to find the macro and some decoding is needed to
understand it. More to see that the wrong type returned by the macro
is not a problem. The value can be written more consisely as 4L << 30
after doing a similar type analysis. 1G is normally written as
1024 * 1024 * 1024 since this is a bit clearer than 1 << 30. This depends
n a similar type analysis -- the multipliction and the shift don't overflow
32-bit ints. But care must be taken with multiplication by another 4 or
even 2.
> Modified: head/sys/x86/x86/mp_x86.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- head/sys/x86/x86/mp_x86.c=09Fri Apr 6 09:25:08 2018=09(r332091)
> +++ head/sys/x86/x86/mp_x86.c=09Fri Apr 6 11:20:06 2018=09(r332092)
> @@ -160,6 +160,8 @@ struct cache_info {
>
> unsigned int boot_address;
>
> +#define MiB(v)=09(v ## ULL << 20)
> +
In this file, the macro is used twice with v =3D 1. Defining and using it
takes only about twice as much code and time to read as (vm_paddr_t)1 << 20=
=2E
Here it is more important to use vm_paddr_t since this code is shared by
amd64, i386 and i386-PAE so the size of vm_paddr_t is variable. However,
since 1MB is far below INT_MAX, it doesn't take much type analysis to see
than the shorter 1 << 20 is safe. 2 copies of the clearer 1024 * 1024
is also shorter than the macro and 2 calls to it.
The macro name doesn't match the comment. The comment still says 1MB.
The fix is not to break the comment.
Later in the file, basemem is converted from K to bytes by multiplying
by 1024. Now 1024 is shorter and clearer than 1 << 10 or 0x400 or a
macro with many undocmented details. The type analysis to show that
multiplying by 1024 doesn't overflow is slightly more complicated since
basemem is a variable. It is only easy to see that this doesn't
overflow because basemem is an old real-mode value. 640K was large
enough for anyone, and basemem in bytes is less than that. 640K was
20 times INT_MAX, but is now 1/3276 of INT_MAX.
Bruce
From owner-svn-src-all@freebsd.org Fri Apr 6 12:24:01 2018
Return-Path: <owner-svn-src-all@freebsd.org>
Delivered-To: svn-src-all@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 B88D1F806FA;
Fri, 6 Apr 2018 12:24:00 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org
[IPv6:2610:1c1:1:606c::19:3])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mxrelay.nyi.freebsd.org",
Issuer "Let's Encrypt Authority X3" (verified OK))
by mx1.freebsd.org (Postfix) with ESMTPS id 6AF4C781EE;
Fri, 6 Apr 2018 12:24:00 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from repo.freebsd.org (repo.freebsd.org
[IPv6:2610:1c1:1:6068::e6a:0])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client did not present a certificate)
by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 65CF511BD5;
Fri, 6 Apr 2018 12:24:00 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from repo.freebsd.org ([127.0.1.37])
by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w36CO0B5009952;
Fri, 6 Apr 2018 12:24:00 GMT (envelope-from avg@FreeBSD.org)
Received: (from avg@localhost)
by repo.freebsd.org (8.15.2/8.15.2/Submit) id w36CO0me009950;
Fri, 6 Apr 2018 12:24:00 GMT (envelope-from avg@FreeBSD.org)
Message-Id: <201804061224.w36CO0me009950@repo.freebsd.org>
X-Authentication-Warning: repo.freebsd.org: avg set sender to avg@FreeBSD.org
using -f
From: Andriy Gapon <avg@FreeBSD.org>
Date: Fri, 6 Apr 2018 12:24:00 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org
Subject: svn commit: r332096 - stable/10/sys/geom
X-SVN-Group: stable-10
X-SVN-Commit-Author: avg
X-SVN-Commit-Paths: stable/10/sys/geom
X-SVN-Commit-Revision: 332096
X-SVN-Commit-Repository: base
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.25
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for "
user" and " projects" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 06 Apr 2018 12:24:01 -0000
Author: avg
Date: Fri Apr 6 12:23:59 2018
New Revision: 332096
URL: https://svnweb.freebsd.org/changeset/base/332096
Log:
MFC r330977: g_access: deal with races created by geoms that drop the topology lock
PR: 225960
Modified:
stable/10/sys/geom/geom.h
stable/10/sys/geom/geom_subr.c
Directory Properties:
stable/10/ (props changed)
Modified: stable/10/sys/geom/geom.h
==============================================================================
--- stable/10/sys/geom/geom.h Fri Apr 6 12:13:32 2018 (r332095)
+++ stable/10/sys/geom/geom.h Fri Apr 6 12:23:59 2018 (r332096)
@@ -147,8 +147,10 @@ struct g_geom {
void *spare1;
void *softc;
unsigned flags;
-#define G_GEOM_WITHER 1
-#define G_GEOM_VOLATILE_BIO 2
+#define G_GEOM_WITHER 0x01
+#define G_GEOM_VOLATILE_BIO 0x02
+#define G_GEOM_IN_ACCESS 0x04
+#define G_GEOM_ACCESS_WAIT 0x08
};
/*
Modified: stable/10/sys/geom/geom_subr.c
==============================================================================
--- stable/10/sys/geom/geom_subr.c Fri Apr 6 12:13:32 2018 (r332095)
+++ stable/10/sys/geom/geom_subr.c Fri Apr 6 12:23:59 2018 (r332096)
@@ -859,7 +859,11 @@ int
g_access(struct g_consumer *cp, int dcr, int dcw, int dce)
{
struct g_provider *pp;
- int pr,pw,pe;
+ struct g_geom *gp;
+ int pw, pe;
+#ifdef INVARIANTS
+ int sr, sw, se;
+#endif
int error;
g_topology_assert();
@@ -867,6 +871,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
pp = cp->provider;
KASSERT(pp != NULL, ("access but not attached"));
G_VALID_PROVIDER(pp);
+ gp = pp->geom;
g_trace(G_T_ACCESS, "g_access(%p(%s), %d, %d, %d)",
cp, pp->name, dcr, dcw, dce);
@@ -875,7 +880,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
KASSERT(cp->acw + dcw >= 0, ("access resulting in negative acw"));
KASSERT(cp->ace + dce >= 0, ("access resulting in negative ace"));
KASSERT(dcr != 0 || dcw != 0 || dce != 0, ("NOP access request"));
- KASSERT(pp->geom->access != NULL, ("NULL geom->access"));
+ KASSERT(gp->access != NULL, ("NULL geom->access"));
/*
* If our class cares about being spoiled, and we have been, we
@@ -887,10 +892,30 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
return (ENXIO);
/*
+ * A number of GEOM classes either need to perform an I/O on the first
+ * open or to acquire a different subsystem's lock. To do that they
+ * may have to drop the topology lock.
+ * Other GEOM classes perform special actions when opening a lower rank
+ * geom for the first time. As a result, more than one thread may
+ * end up performing the special actions.
+ * So, we prevent concurrent "first" opens by marking the consumer with
+ * special flag.
+ *
+ * Note that if the geom's access method never drops the topology lock,
+ * then we will never see G_GEOM_IN_ACCESS here.
+ */
+ while ((gp->flags & G_GEOM_IN_ACCESS) != 0) {
+ g_trace(G_T_ACCESS,
+ "%s: race on geom %s via provider %s and consumer of %s",
+ __func__, gp->name, pp->name, cp->geom->name);
+ gp->flags |= G_GEOM_ACCESS_WAIT;
+ g_topology_sleep(gp, 0);
+ }
+
+ /*
* Figure out what counts the provider would have had, if this
* consumer had (r0w0e0) at this time.
*/
- pr = pp->acr - cp->acr;
pw = pp->acw - cp->acw;
pe = pp->ace - cp->ace;
@@ -902,7 +927,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
pp, pp->name);
/* If foot-shooting is enabled, any open on rank#1 is OK */
- if ((g_debugflags & 16) && pp->geom->rank == 1)
+ if ((g_debugflags & 16) && gp->rank == 1)
;
/* If we try exclusive but already write: fail */
else if (dce > 0 && pw > 0)
@@ -916,11 +941,27 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
/* Ok then... */
- error = pp->geom->access(pp, dcr, dcw, dce);
+#ifdef INVARIANTS
+ sr = cp->acr;
+ sw = cp->acw;
+ se = cp->ace;
+#endif
+ gp->flags |= G_GEOM_IN_ACCESS;
+ error = gp->access(pp, dcr, dcw, dce);
KASSERT(dcr > 0 || dcw > 0 || dce > 0 || error == 0,
("Geom provider %s::%s dcr=%d dcw=%d dce=%d error=%d failed "
- "closing ->access()", pp->geom->class->name, pp->name, dcr, dcw,
+ "closing ->access()", gp->class->name, pp->name, dcr, dcw,
dce, error));
+
+ g_topology_assert();
+ gp->flags &= ~G_GEOM_IN_ACCESS;
+ KASSERT(cp->acr == sr && cp->acw == sw && cp->ace == se,
+ ("Access counts changed during geom->access"));
+ if ((gp->flags & G_GEOM_ACCESS_WAIT) != 0) {
+ gp->flags &= ~G_GEOM_ACCESS_WAIT;
+ wakeup(gp);
+ }
+
if (!error) {
/*
* If we open first write, spoil any partner consumers.
@@ -930,7 +971,7 @@ g_access(struct g_consumer *cp, int dcr, int dcw, int
if (pp->acw == 0 && dcw != 0)
g_spoil(pp, cp);
else if (pp->acw != 0 && pp->acw == -dcw && pp->error == 0 &&
- !(pp->geom->flags & G_GEOM_WITHER))
+ !(gp->flags & G_GEOM_WITHER))
g_post_event(g_new_provider_event, pp, M_WAITOK,
pp, NULL);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180406213913.G2230>