Removing Giant asserts from geom

[Konstantin Belousov]

I propose to remove asserts
mtx_assert(&Giant, MA_NOTOWNED);
from the geom KPI. The asserts do not serve any purposes, at least not
in the current kernel. They might provided some agenda in the 5.x days,
but now they only force filesystems to add cruft to satisfy GEOM KPI
requirements.

As an example, consider DROP_GIANT/PICKUP_GIANT in the ffs code. VFS is
currently Giant-free, including the mount and unmount path, UFS/FFS are
also Giant-free, but mount and unmount paths have to do the Giant dance
to allow root mount to proceed. This is because startup is executed with
the Giant owned by the thread0.

Giant asserts are even more ridiculous in the geom code since geom cdev
geom.ctl is marked as needing Giant. And there are vestiges of Giant
around calls to add geom kthreads, which is not need by KPI. Not to note
that Giant is already held there due to startup protocol.

Any objections ?

diff --git a/sys/geom/eli/g_eli.c b/sys/geom/eli/g_eli.c
index 403d0b6..422883d 100644
--- a/sys/geom/eli/g_eli.c
+++ b/sys/geom/eli/g_eli.c
@@ -1229,7 +1229,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
int error;

mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
sc = gp->softc;
@@ -1245,7 +1244,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
}
}
g_topology_unlock();
- PICKUP_GIANT();
}

static void
diff --git a/sys/geom/geom.h b/sys/geom/geom.h
index bf70d0b..7c1d714 100644
--- a/sys/geom/geom.h
+++ b/sys/geom/geom.h
@@ -369,7 +369,6 @@ g_free(void *ptr)

#define g_topology_lock() \
do { \
- mtx_assert(&Giant, MA_NOTOWNED); \
sx_xlock(&topology_lock); \
} while (0)

diff --git a/sys/geom/geom_event.c b/sys/geom/geom_event.c
index 2ded638..3c2ee49 100644
--- a/sys/geom/geom_event.c
+++ b/sys/geom/geom_event.c
@@ -83,7 +83,6 @@ g_waitidle(void)
{

g_topology_assert_not();
- mtx_assert(&Giant, MA_NOTOWNED);

mtx_lock(&g_eventlock);
while (!TAILQ_EMPTY(&g_events))
diff --git a/sys/geom/geom_kern.c b/sys/geom/geom_kern.c
index dbced0f..9f3f120 100644
--- a/sys/geom/geom_kern.c
+++ b/sys/geom/geom_kern.c
@@ -90,7 +90,6 @@ static void
g_up_procbody(void *arg)
{

- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_up_td);
sched_prio(g_up_td, PRIBIO);
thread_unlock(g_up_td);
@@ -103,7 +102,6 @@ static void
g_down_procbody(void *arg)
{

- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_down_td);
sched_prio(g_down_td, PRIBIO);
thread_unlock(g_down_td);
@@ -116,7 +114,6 @@ static void
g_event_procbody(void *arg)
{

- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_event_td);
sched_prio(g_event_td, PRIBIO);
thread_unlock(g_event_td);
@@ -147,14 +144,12 @@ g_init(void)
g_io_init();
g_event_init();
g_ctl_init();
- mtx_lock(&Giant);
kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td,
RFHIGHPID, 0, "geom", "g_event");
kproc_kthread_add(g_up_procbody, NULL, &g_proc, &g_up_td,
RFHIGHPID, 0, "geom", "g_up");
kproc_kthread_add(g_down_procbody, NULL, &g_proc, &g_down_td,
RFHIGHPID, 0, "geom", "g_down");
- mtx_unlock(&Giant);
EVENTHANDLER_REGISTER(shutdown_pre_sync, geom_shutdown, NULL,
SHUTDOWN_PRI_FIRST);
}
diff --git a/sys/geom/geom_mbr.c b/sys/geom/geom_mbr.c
index 86ee860..a811e35 100644
--- a/sys/geom/geom_mbr.c
+++ b/sys/geom/geom_mbr.c
@@ -190,7 +190,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
case DIOCSMBR: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -205,7 +204,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
default:
diff --git a/sys/geom/geom_pc98.c b/sys/geom/geom_pc98.c
index 42c9962..f4435cb 100644
--- a/sys/geom/geom_pc98.c
+++ b/sys/geom/geom_pc98.c
@@ -176,7 +176,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
case DIOCSPC98: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -191,7 +190,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
default:
diff --git a/sys/geom/geom_subr.c b/sys/geom/geom_subr.c
index 54a99bf..8517991 100644
--- a/sys/geom/geom_subr.c
+++ b/sys/geom/geom_subr.c
@@ -247,9 +247,7 @@ g_modevent(module_t mod, int type, void *data)
break;
case MOD_UNLOAD:
g_trace(G_T_TOPOLOGY, "g_modevent(%s, UNLOAD)", mp->name);
- DROP_GIANT();
error = g_unload_class(mp);
- PICKUP_GIANT();
if (error == 0) {
KASSERT(LIST_EMPTY(&mp->geom),
("Unloaded class (%s) still has geom", mp->name));
diff --git a/sys/geom/journal/g_journal.c b/sys/geom/journal/g_journal.c
index 871bd8e4..0678003 100644
--- a/sys/geom/journal/g_journal.c
+++ b/sys/geom/journal/g_journal.c
@@ -2697,7 +2697,6 @@ g_journal_shutdown(void *arg, int howto __unused)
if (panicstr != NULL)
return;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
if (gp->softc == NULL)
@@ -2706,7 +2705,6 @@ g_journal_shutdown(void *arg, int howto __unused)
g_journal_destroy(gp->softc);
}
g_topology_unlock();
- PICKUP_GIANT();
}

/*
@@ -2725,7 +2723,6 @@ g_journal_lowmem(void *arg, int howto __unused)

g_journal_stats_low_mem++;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &mp->geom, geom) {
sc = gp->softc;
@@ -2756,7 +2753,6 @@ g_journal_lowmem(void *arg, int howto __unused)
break;
}
g_topology_unlock();
- PICKUP_GIANT();
}

static void g_journal_switcher(void *arg);
@@ -2871,7 +2867,6 @@ g_journal_do_switch(struct g_class *classp)
char *mountpoint;
int error, save;

- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &classp->geom, geom) {
sc = gp->softc;
@@ -2886,7 +2881,6 @@ g_journal_do_switch(struct g_class *classp)
mtx_unlock(&sc->sc_mtx);
}
g_topology_unlock();
- PICKUP_GIANT();

mtx_lock(&mountlist_mtx);
TAILQ_FOREACH(mp, &mountlist, mnt_list) {
@@ -2901,11 +2895,9 @@ g_journal_do_switch(struct g_class *classp)
continue;
/* mtx_unlock(&mountlist_mtx) was done inside vfs_busy() */

- DROP_GIANT();
g_topology_lock();
sc = g_journal_find_device(classp, mp->mnt_gjprovider);
g_topology_unlock();
- PICKUP_GIANT();

if (sc == NULL) {
GJ_DEBUG(0, "Cannot find journal geom for %s.",
@@ -2984,7 +2976,6 @@ next:

sc = NULL;
for (;;) {
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &g_journal_class.geom, geom) {
sc = gp->softc;
@@ -3000,7 +2991,6 @@ next:
sc = NULL;
}
g_topology_unlock();
- PICKUP_GIANT();
if (sc == NULL)
break;
mtx_assert(&sc->sc_mtx, MA_OWNED);
diff --git a/sys/geom/mirror/g_mirror.c b/sys/geom/mirror/g_mirror.c
index 91f1367..379f615 100644
--- a/sys/geom/mirror/g_mirror.c
+++ b/sys/geom/mirror/g_mirror.c
@@ -3310,7 +3310,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
int error;

mp = arg;
- DROP_GIANT();
g_topology_lock();
g_mirror_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3329,7 +3328,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}

static void
diff --git a/sys/geom/mountver/g_mountver.c b/sys/geom/mountver/g_mountver.c
index eafccc8..61375ef 100644
--- a/sys/geom/mountver/g_mountver.c
+++ b/sys/geom/mountver/g_mountver.c
@@ -611,12 +611,10 @@ g_mountver_shutdown_pre_sync(void *arg, int howto)
struct g_geom *gp, *gp2;

mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2)
g_mountver_destroy(gp, 1);
g_topology_unlock();
- PICKUP_GIANT();
}

static void
diff --git a/sys/geom/raid/g_raid.c b/sys/geom/raid/g_raid.c
index 4885319..e590e35 100644
--- a/sys/geom/raid/g_raid.c
+++ b/sys/geom/raid/g_raid.c
@@ -2462,7 +2462,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
struct g_raid_volume *vol;

mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -2477,7 +2476,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}

static void
diff --git a/sys/geom/raid3/g_raid3.c b/sys/geom/raid3/g_raid3.c
index a2ffe53..9b3c483 100644
--- a/sys/geom/raid3/g_raid3.c
+++ b/sys/geom/raid3/g_raid3.c
@@ -3543,7 +3543,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
int error;

mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid3_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3562,7 +3561,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}

static void

[Poul-Henning Kamp]

--------

Post by Konstantin Belousov
I propose to remove asserts
mtx_assert(&Giant, MA_NOTOWNED);

By all means.

They were necessary because GEOM happened in the early days of SMP
where things were strange and locks unpredictable.

[Alfred Perlstein]

It seems like it should be the opposite, the DROP_GIANTs should be
turned into mtx_assert(&Giant, MA_NOTOWNED) as giant is removed from the
tree.

Meaning Giant should be pushed further back until it is eliminated.
Doing as this patch proposes hides that we still have callers holding
Giant which is not good.

-Alfred

Post by Konstantin Belousov
I propose to remove asserts
mtx_assert(&Giant, MA_NOTOWNED);
from the geom KPI. The asserts do not serve any purposes, at least not
in the current kernel. They might provided some agenda in the 5.x days,
but now they only force filesystems to add cruft to satisfy GEOM KPI
requirements.
As an example, consider DROP_GIANT/PICKUP_GIANT in the ffs code. VFS is
currently Giant-free, including the mount and unmount path, UFS/FFS are
also Giant-free, but mount and unmount paths have to do the Giant dance
to allow root mount to proceed. This is because startup is executed with
the Giant owned by the thread0.
Giant asserts are even more ridiculous in the geom code since geom cdev
geom.ctl is marked as needing Giant. And there are vestiges of Giant
around calls to add geom kthreads, which is not need by KPI. Not to note
that Giant is already held there due to startup protocol.
Any objections ?
diff --git a/sys/geom/eli/g_eli.c b/sys/geom/eli/g_eli.c
index 403d0b6..422883d 100644
--- a/sys/geom/eli/g_eli.c
+++ b/sys/geom/eli/g_eli.c
@@ -1229,7 +1229,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
sc = gp->softc;
@@ -1245,7 +1244,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
}
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/geom.h b/sys/geom/geom.h
index bf70d0b..7c1d714 100644
--- a/sys/geom/geom.h
+++ b/sys/geom/geom.h
@@ -369,7 +369,6 @@ g_free(void *ptr)
#define g_topology_lock() \
do { \
- mtx_assert(&Giant, MA_NOTOWNED); \
sx_xlock(&topology_lock); \
} while (0)
diff --git a/sys/geom/geom_event.c b/sys/geom/geom_event.c
index 2ded638..3c2ee49 100644
--- a/sys/geom/geom_event.c
+++ b/sys/geom/geom_event.c
@@ -83,7 +83,6 @@ g_waitidle(void)
{
g_topology_assert_not();
- mtx_assert(&Giant, MA_NOTOWNED);
mtx_lock(&g_eventlock);
while (!TAILQ_EMPTY(&g_events))
diff --git a/sys/geom/geom_kern.c b/sys/geom/geom_kern.c
index dbced0f..9f3f120 100644
--- a/sys/geom/geom_kern.c
+++ b/sys/geom/geom_kern.c
@@ -90,7 +90,6 @@ static void
g_up_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_up_td);
sched_prio(g_up_td, PRIBIO);
thread_unlock(g_up_td);
@@ -103,7 +102,6 @@ static void
g_down_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_down_td);
sched_prio(g_down_td, PRIBIO);
thread_unlock(g_down_td);
@@ -116,7 +114,6 @@ static void
g_event_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_event_td);
sched_prio(g_event_td, PRIBIO);
thread_unlock(g_event_td);
@@ -147,14 +144,12 @@ g_init(void)
g_io_init();
g_event_init();
g_ctl_init();
- mtx_lock(&Giant);
kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td,
RFHIGHPID, 0, "geom", "g_event");
kproc_kthread_add(g_up_procbody, NULL, &g_proc, &g_up_td,
RFHIGHPID, 0, "geom", "g_up");
kproc_kthread_add(g_down_procbody, NULL, &g_proc, &g_down_td,
RFHIGHPID, 0, "geom", "g_down");
- mtx_unlock(&Giant);
EVENTHANDLER_REGISTER(shutdown_pre_sync, geom_shutdown, NULL,
SHUTDOWN_PRI_FIRST);
}
diff --git a/sys/geom/geom_mbr.c b/sys/geom/geom_mbr.c
index 86ee860..a811e35 100644
--- a/sys/geom/geom_mbr.c
+++ b/sys/geom/geom_mbr.c
@@ -190,7 +190,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
case DIOCSMBR: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -205,7 +204,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
diff --git a/sys/geom/geom_pc98.c b/sys/geom/geom_pc98.c
index 42c9962..f4435cb 100644
--- a/sys/geom/geom_pc98.c
+++ b/sys/geom/geom_pc98.c
@@ -176,7 +176,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
case DIOCSPC98: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -191,7 +190,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
diff --git a/sys/geom/geom_subr.c b/sys/geom/geom_subr.c
index 54a99bf..8517991 100644
--- a/sys/geom/geom_subr.c
+++ b/sys/geom/geom_subr.c
@@ -247,9 +247,7 @@ g_modevent(module_t mod, int type, void *data)
break;
g_trace(G_T_TOPOLOGY, "g_modevent(%s, UNLOAD)", mp->name);
- DROP_GIANT();
error = g_unload_class(mp);
- PICKUP_GIANT();
if (error == 0) {
KASSERT(LIST_EMPTY(&mp->geom),
("Unloaded class (%s) still has geom", mp->name));
diff --git a/sys/geom/journal/g_journal.c b/sys/geom/journal/g_journal.c
index 871bd8e4..0678003 100644
--- a/sys/geom/journal/g_journal.c
+++ b/sys/geom/journal/g_journal.c
@@ -2697,7 +2697,6 @@ g_journal_shutdown(void *arg, int howto __unused)
if (panicstr != NULL)
return;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
if (gp->softc == NULL)
@@ -2706,7 +2705,6 @@ g_journal_shutdown(void *arg, int howto __unused)
g_journal_destroy(gp->softc);
}
g_topology_unlock();
- PICKUP_GIANT();
}
/*
@@ -2725,7 +2723,6 @@ g_journal_lowmem(void *arg, int howto __unused)
g_journal_stats_low_mem++;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &mp->geom, geom) {
sc = gp->softc;
@@ -2756,7 +2753,6 @@ g_journal_lowmem(void *arg, int howto __unused)
break;
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void g_journal_switcher(void *arg);
@@ -2871,7 +2867,6 @@ g_journal_do_switch(struct g_class *classp)
char *mountpoint;
int error, save;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &classp->geom, geom) {
sc = gp->softc;
@@ -2886,7 +2881,6 @@ g_journal_do_switch(struct g_class *classp)
mtx_unlock(&sc->sc_mtx);
}
g_topology_unlock();
- PICKUP_GIANT();
mtx_lock(&mountlist_mtx);
TAILQ_FOREACH(mp, &mountlist, mnt_list) {
@@ -2901,11 +2895,9 @@ g_journal_do_switch(struct g_class *classp)
continue;
/* mtx_unlock(&mountlist_mtx) was done inside vfs_busy() */
- DROP_GIANT();
g_topology_lock();
sc = g_journal_find_device(classp, mp->mnt_gjprovider);
g_topology_unlock();
- PICKUP_GIANT();
if (sc == NULL) {
GJ_DEBUG(0, "Cannot find journal geom for %s.",
sc = NULL;
for (;;) {
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &g_journal_class.geom, geom) {
sc = gp->softc;
sc = NULL;
}
g_topology_unlock();
- PICKUP_GIANT();
if (sc == NULL)
break;
mtx_assert(&sc->sc_mtx, MA_OWNED);
diff --git a/sys/geom/mirror/g_mirror.c b/sys/geom/mirror/g_mirror.c
index 91f1367..379f615 100644
--- a/sys/geom/mirror/g_mirror.c
+++ b/sys/geom/mirror/g_mirror.c
@@ -3310,7 +3310,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_mirror_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3329,7 +3328,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/mountver/g_mountver.c b/sys/geom/mountver/g_mountver.c
index eafccc8..61375ef 100644
--- a/sys/geom/mountver/g_mountver.c
+++ b/sys/geom/mountver/g_mountver.c
@@ -611,12 +611,10 @@ g_mountver_shutdown_pre_sync(void *arg, int howto)
struct g_geom *gp, *gp2;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2)
g_mountver_destroy(gp, 1);
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/raid/g_raid.c b/sys/geom/raid/g_raid.c
index 4885319..e590e35 100644
--- a/sys/geom/raid/g_raid.c
+++ b/sys/geom/raid/g_raid.c
@@ -2462,7 +2462,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
struct g_raid_volume *vol;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -2477,7 +2476,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/raid3/g_raid3.c b/sys/geom/raid3/g_raid3.c
index a2ffe53..9b3c483 100644
--- a/sys/geom/raid3/g_raid3.c
+++ b/sys/geom/raid3/g_raid3.c
@@ -3543,7 +3543,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid3_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3562,7 +3561,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch

[Konstantin Belousov]

Post by Alfred Perlstein
It seems like it should be the opposite, the DROP_GIANTs should be
turned into mtx_assert(&Giant, MA_NOTOWNED) as giant is removed from the
tree.
Meaning Giant should be pushed further back until it is eliminated.
Doing as this patch proposes hides that we still have callers holding
Giant which is not good.

Did you read the third paragraph of my email ?

FWIW, the assumed model of the kernel locking which must be in somebody
mind when talking about 'pushing back Giant' is not true for last 5-6
years for our kernel in general, and for the VFS in particular.

Post by Alfred Perlstein
-Alfred

Post by Konstantin Belousov
I propose to remove asserts
mtx_assert(&Giant, MA_NOTOWNED);
from the geom KPI. The asserts do not serve any purposes, at least not
in the current kernel. They might provided some agenda in the 5.x days,
but now they only force filesystems to add cruft to satisfy GEOM KPI
requirements.
As an example, consider DROP_GIANT/PICKUP_GIANT in the ffs code. VFS is
currently Giant-free, including the mount and unmount path, UFS/FFS are
also Giant-free, but mount and unmount paths have to do the Giant dance
to allow root mount to proceed. This is because startup is executed with
the Giant owned by the thread0.

^^^^

Post by Alfred Perlstein

Post by Konstantin Belousov
Giant asserts are even more ridiculous in the geom code since geom cdev
geom.ctl is marked as needing Giant. And there are vestiges of Giant
around calls to add geom kthreads, which is not need by KPI. Not to note
that Giant is already held there due to startup protocol.
Any objections ?
diff --git a/sys/geom/eli/g_eli.c b/sys/geom/eli/g_eli.c
index 403d0b6..422883d 100644
--- a/sys/geom/eli/g_eli.c
+++ b/sys/geom/eli/g_eli.c
@@ -1229,7 +1229,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
sc = gp->softc;
@@ -1245,7 +1244,6 @@ g_eli_shutdown_pre_sync(void *arg, int howto)
}
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/geom.h b/sys/geom/geom.h
index bf70d0b..7c1d714 100644
--- a/sys/geom/geom.h
+++ b/sys/geom/geom.h
@@ -369,7 +369,6 @@ g_free(void *ptr)
#define g_topology_lock() \
do { \
- mtx_assert(&Giant, MA_NOTOWNED); \
sx_xlock(&topology_lock); \
} while (0)
diff --git a/sys/geom/geom_event.c b/sys/geom/geom_event.c
index 2ded638..3c2ee49 100644
--- a/sys/geom/geom_event.c
+++ b/sys/geom/geom_event.c
@@ -83,7 +83,6 @@ g_waitidle(void)
{
g_topology_assert_not();
- mtx_assert(&Giant, MA_NOTOWNED);
mtx_lock(&g_eventlock);
while (!TAILQ_EMPTY(&g_events))
diff --git a/sys/geom/geom_kern.c b/sys/geom/geom_kern.c
index dbced0f..9f3f120 100644
--- a/sys/geom/geom_kern.c
+++ b/sys/geom/geom_kern.c
@@ -90,7 +90,6 @@ static void
g_up_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_up_td);
sched_prio(g_up_td, PRIBIO);
thread_unlock(g_up_td);
@@ -103,7 +102,6 @@ static void
g_down_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_down_td);
sched_prio(g_down_td, PRIBIO);
thread_unlock(g_down_td);
@@ -116,7 +114,6 @@ static void
g_event_procbody(void *arg)
{
- mtx_assert(&Giant, MA_NOTOWNED);
thread_lock(g_event_td);
sched_prio(g_event_td, PRIBIO);
thread_unlock(g_event_td);
@@ -147,14 +144,12 @@ g_init(void)
g_io_init();
g_event_init();
g_ctl_init();
- mtx_lock(&Giant);
kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td,
RFHIGHPID, 0, "geom", "g_event");
kproc_kthread_add(g_up_procbody, NULL, &g_proc, &g_up_td,
RFHIGHPID, 0, "geom", "g_up");
kproc_kthread_add(g_down_procbody, NULL, &g_proc, &g_down_td,
RFHIGHPID, 0, "geom", "g_down");
- mtx_unlock(&Giant);
EVENTHANDLER_REGISTER(shutdown_pre_sync, geom_shutdown, NULL,
SHUTDOWN_PRI_FIRST);
}
diff --git a/sys/geom/geom_mbr.c b/sys/geom/geom_mbr.c
index 86ee860..a811e35 100644
--- a/sys/geom/geom_mbr.c
+++ b/sys/geom/geom_mbr.c
@@ -190,7 +190,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
case DIOCSMBR: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -205,7 +204,6 @@ g_mbr_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct thr
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
diff --git a/sys/geom/geom_pc98.c b/sys/geom/geom_pc98.c
index 42c9962..f4435cb 100644
--- a/sys/geom/geom_pc98.c
+++ b/sys/geom/geom_pc98.c
@@ -176,7 +176,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
case DIOCSPC98: {
if (!(fflag & FWRITE))
return (EPERM);
- DROP_GIANT();
g_topology_lock();
cp = LIST_FIRST(&gp->consumer);
if (cp->acw == 0) {
@@ -191,7 +190,6 @@ g_pc98_ioctl(struct g_provider *pp, u_long cmd, void *data, int fflag, struct th
if (opened)
g_access(cp, 0, -1 , 0);
g_topology_unlock();
- PICKUP_GIANT();
return(error);
}
diff --git a/sys/geom/geom_subr.c b/sys/geom/geom_subr.c
index 54a99bf..8517991 100644
--- a/sys/geom/geom_subr.c
+++ b/sys/geom/geom_subr.c
@@ -247,9 +247,7 @@ g_modevent(module_t mod, int type, void *data)
break;
g_trace(G_T_TOPOLOGY, "g_modevent(%s, UNLOAD)", mp->name);
- DROP_GIANT();
error = g_unload_class(mp);
- PICKUP_GIANT();
if (error == 0) {
KASSERT(LIST_EMPTY(&mp->geom),
("Unloaded class (%s) still has geom", mp->name));
diff --git a/sys/geom/journal/g_journal.c b/sys/geom/journal/g_journal.c
index 871bd8e4..0678003 100644
--- a/sys/geom/journal/g_journal.c
+++ b/sys/geom/journal/g_journal.c
@@ -2697,7 +2697,6 @@ g_journal_shutdown(void *arg, int howto __unused)
if (panicstr != NULL)
return;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
if (gp->softc == NULL)
@@ -2706,7 +2705,6 @@ g_journal_shutdown(void *arg, int howto __unused)
g_journal_destroy(gp->softc);
}
g_topology_unlock();
- PICKUP_GIANT();
}
/*
@@ -2725,7 +2723,6 @@ g_journal_lowmem(void *arg, int howto __unused)
g_journal_stats_low_mem++;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &mp->geom, geom) {
sc = gp->softc;
@@ -2756,7 +2753,6 @@ g_journal_lowmem(void *arg, int howto __unused)
break;
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void g_journal_switcher(void *arg);
@@ -2871,7 +2867,6 @@ g_journal_do_switch(struct g_class *classp)
char *mountpoint;
int error, save;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &classp->geom, geom) {
sc = gp->softc;
@@ -2886,7 +2881,6 @@ g_journal_do_switch(struct g_class *classp)
mtx_unlock(&sc->sc_mtx);
}
g_topology_unlock();
- PICKUP_GIANT();
mtx_lock(&mountlist_mtx);
TAILQ_FOREACH(mp, &mountlist, mnt_list) {
@@ -2901,11 +2895,9 @@ g_journal_do_switch(struct g_class *classp)
continue;
/* mtx_unlock(&mountlist_mtx) was done inside vfs_busy() */
- DROP_GIANT();
g_topology_lock();
sc = g_journal_find_device(classp, mp->mnt_gjprovider);
g_topology_unlock();
- PICKUP_GIANT();
if (sc == NULL) {
GJ_DEBUG(0, "Cannot find journal geom for %s.",
sc = NULL;
for (;;) {
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH(gp, &g_journal_class.geom, geom) {
sc = gp->softc;
sc = NULL;
}
g_topology_unlock();
- PICKUP_GIANT();
if (sc == NULL)
break;
mtx_assert(&sc->sc_mtx, MA_OWNED);
diff --git a/sys/geom/mirror/g_mirror.c b/sys/geom/mirror/g_mirror.c
index 91f1367..379f615 100644
--- a/sys/geom/mirror/g_mirror.c
+++ b/sys/geom/mirror/g_mirror.c
@@ -3310,7 +3310,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_mirror_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3329,7 +3328,6 @@ g_mirror_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/mountver/g_mountver.c b/sys/geom/mountver/g_mountver.c
index eafccc8..61375ef 100644
--- a/sys/geom/mountver/g_mountver.c
+++ b/sys/geom/mountver/g_mountver.c
@@ -611,12 +611,10 @@ g_mountver_shutdown_pre_sync(void *arg, int howto)
struct g_geom *gp, *gp2;
mp = arg;
- DROP_GIANT();
g_topology_lock();
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2)
g_mountver_destroy(gp, 1);
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/raid/g_raid.c b/sys/geom/raid/g_raid.c
index 4885319..e590e35 100644
--- a/sys/geom/raid/g_raid.c
+++ b/sys/geom/raid/g_raid.c
@@ -2462,7 +2462,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
struct g_raid_volume *vol;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -2477,7 +2476,6 @@ g_raid_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
diff --git a/sys/geom/raid3/g_raid3.c b/sys/geom/raid3/g_raid3.c
index a2ffe53..9b3c483 100644
--- a/sys/geom/raid3/g_raid3.c
+++ b/sys/geom/raid3/g_raid3.c
@@ -3543,7 +3543,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
int error;
mp = arg;
- DROP_GIANT();
g_topology_lock();
g_raid3_shutdown = 1;
LIST_FOREACH_SAFE(gp, &mp->geom, geom, gp2) {
@@ -3562,7 +3561,6 @@ g_raid3_shutdown_post_sync(void *arg, int howto)
g_topology_lock();
}
g_topology_unlock();
- PICKUP_GIANT();
}
static void
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch

[Alfred Perlstein]

Post by Konstantin Belousov

Post by Alfred Perlstein
It seems like it should be the opposite, the DROP_GIANTs should be
turned into mtx_assert(&Giant, MA_NOTOWNED) as giant is removed from the
tree.
Meaning Giant should be pushed further back until it is eliminated.
Doing as this patch proposes hides that we still have callers holding
Giant which is not good.

Did you read the third paragraph of my email ?

OK, and why is thread0 needing Giant for so long?

Post by Konstantin Belousov
FWIW, the assumed model of the kernel locking which must be in somebody
mind when talking about 'pushing back Giant' is not true for last 5-6
years for our kernel in general, and for the VFS in particular.

OK, makes sense, still would prefer to have assertions that don't allow
mistakes to creep in. FreeBSD's assertions on locking and VFS make it
much easier to develop under.

-Alfred

[Konstantin Belousov]

Post by Alfred Perlstein
OK, and why is thread0 needing Giant for so long?

[Below is my opinion]

It does not need Giant per se, it needs a work done to audit and turn it
off. Probably most high profile subsystem in the kernel still relying on
Giant is the newbus. Easy to see consequence is that handling thread0,
that spends most of the time in discovering and configuring devices,
actually needs to provide proper locking for the newbus.

At least one attempt to do the later failed. Troubles are both in the
strange recursiveness of newbus, where device method could call into
subr_bus.c, and in potential offloading of some work to other thread,
which means that naive surround of the subr_bus.c methods with some
global sleepable (and even recursive) lock would not work.

Since newbus activity and early startup are rare events, fine-grained
locking for them would result in, well, fine grained locking. There
would be no break-through performance improvements after really hard
work, including handling user reports for long time.

So, it is not a priority for most people.

[Warner Losh]

On Thu, May 19, 2016 at 3:31 PM, Konstantin Belousov

Post by Konstantin Belousov

Post by Alfred Perlstein
OK, and why is thread0 needing Giant for so long?

[Below is my opinion]
It does not need Giant per se, it needs a work done to audit and turn it
off. Probably most high profile subsystem in the kernel still relying on
Giant is the newbus. Easy to see consequence is that handling thread0,
that spends most of the time in discovering and configuring devices,
actually needs to provide proper locking for the newbus.
At least one attempt to do the later failed. Troubles are both in the
strange recursiveness of newbus, where device method could call into
subr_bus.c, and in potential offloading of some work to other thread,
which means that naive surround of the subr_bus.c methods with some
global sleepable (and even recursive) lock would not work.
Since newbus activity and early startup are rare events, fine-grained
locking for them would result in, well, fine grained locking. There
would be no break-through performance improvements after really hard
work, including handling user reports for long time.
So, it is not a priority for most people.

I know of three runs at the newbus locking issue that have failed.
It's not easy or trivial because it calls into so many different
subsystems, many in odd ways that aren't done elsewhere.

Warner

[Alfred Perlstein]

Post by Konstantin Belousov

Post by Alfred Perlstein
OK, and why is thread0 needing Giant for so long?

[Below is my opinion]
It does not need Giant per se, it needs a work done to audit and turn it
off. Probably most high profile subsystem in the kernel still relying on
Giant is the newbus. Easy to see consequence is that handling thread0,
that spends most of the time in discovering and configuring devices,
actually needs to provide proper locking for the newbus.
At least one attempt to do the later failed. Troubles are both in the
strange recursiveness of newbus, where device method could call into
subr_bus.c, and in potential offloading of some work to other thread,
which means that naive surround of the subr_bus.c methods with some
global sleepable (and even recursive) lock would not work.
Since newbus activity and early startup are rare events, fine-grained
locking for them would result in, well, fine grained locking. There
would be no break-through performance improvements after really hard
work, including handling user reports for long time.
So, it is not a priority for most people.

Thank you Konstantin, makes sense.

-Alfred

[Warner Losh]

On Thu, May 19, 2016 at 4:56 AM, Konstantin Belousov

Post by Konstantin Belousov
I propose to remove asserts
mtx_assert(&Giant, MA_NOTOWNED);
from the geom KPI.

I can't see any reason for the assertion in today's kernel.

I might split out the lock / unlock of giant around thread creation
since that's different.

Warner