From owner-freebsd-geom@freebsd.org Thu May 19 10:56:44 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8948BB423AC for ; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 75302147B for ; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id 6DF73B423AA; Thu, 19 May 2016 10:56:44 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6D328B423A8; Thu, 19 May 2016 10:56:44 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id F2C791478; Thu, 19 May 2016 10:56:40 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u4JAuYfr059664 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Thu, 19 May 2016 13:56:35 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u4JAuYfr059664 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u4JAuYhq059663; Thu, 19 May 2016 13:56:34 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 19 May 2016 13:56:34 +0300 From: Konstantin Belousov To: geom@freebsd.org Cc: arch@freebsd.org Subject: Removing Giant asserts from geom Message-ID: <20160519105634.GO89104@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 10:56:44 -0000 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 From owner-freebsd-geom@freebsd.org Thu May 19 11:04:30 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 339FEB4277E for ; Thu, 19 May 2016 11:04:30 +0000 (UTC) (envelope-from phk@phk.freebsd.dk) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 240C21E33 for ; Thu, 19 May 2016 11:04:30 +0000 (UTC) (envelope-from phk@phk.freebsd.dk) Received: by mailman.ysv.freebsd.org (Postfix) id 23727B4277C; Thu, 19 May 2016 11:04:30 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 23034B4277B; Thu, 19 May 2016 11:04:30 +0000 (UTC) (envelope-from phk@phk.freebsd.dk) Received: from phk.freebsd.dk (phk.freebsd.dk [130.225.244.222]) by mx1.freebsd.org (Postfix) with ESMTP id E55901E32; Thu, 19 May 2016 11:04:29 +0000 (UTC) (envelope-from phk@phk.freebsd.dk) Received: from critter.freebsd.dk (unknown [192.168.55.3]) by phk.freebsd.dk (Postfix) with ESMTP id 58CEE4FB1D; Thu, 19 May 2016 11:04:22 +0000 (UTC) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.15.2/8.15.2) with ESMTP id u4JB4Lx2085765; Thu, 19 May 2016 11:04:21 GMT (envelope-from phk@phk.freebsd.dk) To: Konstantin Belousov cc: geom@freebsd.org, arch@freebsd.org Subject: Re: Removing Giant asserts from geom In-reply-to: <20160519105634.GO89104@kib.kiev.ua> From: "Poul-Henning Kamp" References: <20160519105634.GO89104@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <85763.1463655861.1@critter.freebsd.dk> Date: Thu, 19 May 2016 11:04:21 +0000 Message-ID: <85764.1463655861@critter.freebsd.dk> X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 11:04:30 -0000 -------- In message <20160519105634.GO89104@kib.kiev.ua>, Konstantin Belousov writes: >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. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@FreeBSD.ORG | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. From owner-freebsd-geom@freebsd.org Thu May 19 16:31:54 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 847AFB427A7 for ; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 750021F26 for ; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: by mailman.ysv.freebsd.org (Postfix) id 745C8B427A5; Thu, 19 May 2016 16:31:54 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 73D89B427A4; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [IPv6:2001:470:1f05:b76::196]) by mx1.freebsd.org (Postfix) with ESMTP id 644B91F25; Thu, 19 May 2016 16:31:54 +0000 (UTC) (envelope-from bright@mu.org) Received: from AlfredMacbookAir.local (unknown [IPv6:2601:645:8003:a4d6:1000:9833:bf6a:cc6b]) by elvis.mu.org (Postfix) with ESMTPSA id 9F159346DDE2; Thu, 19 May 2016 09:31:48 -0700 (PDT) Subject: Re: Removing Giant asserts from geom To: Konstantin Belousov , geom@freebsd.org References: <20160519105634.GO89104@kib.kiev.ua> Cc: arch@freebsd.org From: Alfred Perlstein Message-ID: <573DEA73.4080408@mu.org> Date: Thu, 19 May 2016 09:31:47 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160519105634.GO89104@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 16:31:54 -0000 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 On 5/19/16 3:56 AM, Konstantin Belousov wrote: > 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 > _______________________________________________ > freebsd-arch@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" > From owner-freebsd-geom@freebsd.org Thu May 19 19:12:56 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0514DB3F411 for ; Thu, 19 May 2016 19:12:56 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id E40DC1125 for ; Thu, 19 May 2016 19:12:55 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id DFEC8B3F40E; Thu, 19 May 2016 19:12:55 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DCDB6B3F40C; Thu, 19 May 2016 19:12:55 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 6EF4E1122; Thu, 19 May 2016 19:12:55 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u4JJClL9084388 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Thu, 19 May 2016 22:12:48 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u4JJClL9084388 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u4JJCliR084387; Thu, 19 May 2016 22:12:47 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 19 May 2016 22:12:47 +0300 From: Konstantin Belousov To: Alfred Perlstein Cc: geom@freebsd.org, arch@freebsd.org Subject: Re: Removing Giant asserts from geom Message-ID: <20160519191247.GQ89104@kib.kiev.ua> References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <573DEA73.4080408@mu.org> User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 19:12:56 -0000 On Thu, May 19, 2016 at 09:31:47AM -0700, Alfred Perlstein wrote: > 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. > > -Alfred > > > On 5/19/16 3:56 AM, Konstantin Belousov wrote: > > 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 > > _______________________________________________ > > freebsd-arch@freebsd.org mailing list > > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org" > > From owner-freebsd-geom@freebsd.org Thu May 19 19:58:56 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9B537B42020 for ; Thu, 19 May 2016 19:58:56 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 7D3171A4C for ; Thu, 19 May 2016 19:58:56 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mailman.ysv.freebsd.org (Postfix) id 79037B4201E; Thu, 19 May 2016 19:58:56 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 78A55B4201C for ; Thu, 19 May 2016 19:58:56 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (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 4E6521A4A for ; Thu, 19 May 2016 19:58:56 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io0-x229.google.com with SMTP id t40so29307515ioi.0 for ; Thu, 19 May 2016 12:58:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc; bh=ni3XQ2Fv3KiXMKEwYiQC0scsA+fc4g2ZSpxyNEfEYew=; b=is5fWV3f7CF88CMLN7O0TRptLkJJ6TMdjj2OVPiaSSrqVRc9AwFSAWv/etQhIR3bH1 /aTzKk3e6r2sptOMjxnkocY+C7QlIab9aFl7UIaKU0Mm9a3T31j+xVFTI+ZtinRgtT/L QFZQlKhqIpAes3JMbv1ObXPqOwJs5CLouAu5g1xkQY+Z3/de/cz9uZ0apA5p18ylPkvo 8uzl3mTx4wBH/aeYTTvdjlw+BDJvEuxkRJPslEiX3SqLyYMZishd2T7kFRCSYhelHaih 0fQw1ElMFKXfTcFHe9yMRhe8u4V7oLB1sFx1fhj4pZ+fEA2M7a8j7P2FmJkoUG+uBne2 3hxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc; bh=ni3XQ2Fv3KiXMKEwYiQC0scsA+fc4g2ZSpxyNEfEYew=; b=cxD5plNGq7shpaanWOqbEfvFwwif0JY5sHpvziBHuSgdW4EtcDe5QWsIm2tcJ4FNgX UbEMrjGo2F3Z8npKQ1OLDdqvTrP+kDaxRIIdjxlOogGmq626uw7xPdpeXWB50T1pec+f 01fdyE6rjtN0J5FjV+wPxR4iA1VnSnRauAFLwkElo6gcnZpFPkW+QPbaFETZ0AHSkCN3 tahTh8pucgmaEWbgnkKVOpWgdaGjEN1prbOE6yXrPJ3qSX/GytJQP3WWCKzzkVGLeaXV dR4svFNPYXsZMYxF1E848Gzby93ddo/H4Qc0YTiTBD1yUeGWuNbxb5ZuvuVOzOM7F3g4 7Zvg== X-Gm-Message-State: AOPr4FXQLt5hLwurGOcwY9/A/uXqBs4xPHJgISZd6Ug91m/GKWj2KHegknjkgql6mxo0VNOsDCwuwn2+7B5hZQ== MIME-Version: 1.0 X-Received: by 10.36.78.67 with SMTP id r64mr4519237ita.72.1463687935609; Thu, 19 May 2016 12:58:55 -0700 (PDT) Sender: wlosh@bsdimp.com Received: by 10.79.75.68 with HTTP; Thu, 19 May 2016 12:58:55 -0700 (PDT) X-Originating-IP: [50.253.99.174] In-Reply-To: <20160519105634.GO89104@kib.kiev.ua> References: <20160519105634.GO89104@kib.kiev.ua> Date: Thu, 19 May 2016 13:58:55 -0600 X-Google-Sender-Auth: 1rl7IH8SaZS3VPQGythPq5cvSsk Message-ID: Subject: Re: Removing Giant asserts from geom From: Warner Losh To: Konstantin Belousov Cc: geom@freebsd.org, "freebsd-arch@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 19:58:56 -0000 On Thu, May 19, 2016 at 4:56 AM, Konstantin Belousov wrote: > 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 From owner-freebsd-geom@freebsd.org Thu May 19 20:56:44 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B27FFB41182 for ; Thu, 19 May 2016 20:56:44 +0000 (UTC) (envelope-from bright@mu.org) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id A35511E8C for ; Thu, 19 May 2016 20:56:44 +0000 (UTC) (envelope-from bright@mu.org) Received: by mailman.ysv.freebsd.org (Postfix) id 9B4A6B4117B; Thu, 19 May 2016 20:56:44 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9AAB6B41176; Thu, 19 May 2016 20:56:44 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 8EEA91E8A; Thu, 19 May 2016 20:56:44 +0000 (UTC) (envelope-from bright@mu.org) Received: from Alfreds-MacBook-Pro-2.local (c-76-21-10-192.hsd1.ca.comcast.net [76.21.10.192]) by elvis.mu.org (Postfix) with ESMTPSA id B15C0346DDE2; Thu, 19 May 2016 13:56:43 -0700 (PDT) Subject: Re: Removing Giant asserts from geom To: Konstantin Belousov References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org> <20160519191247.GQ89104@kib.kiev.ua> Cc: geom@freebsd.org, arch@freebsd.org From: Alfred Perlstein Message-ID: <53397f3f-1056-ceb7-ce3a-5269ac1d29e2@mu.org> Date: Thu, 19 May 2016 13:57:53 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160519191247.GQ89104@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 20:56:44 -0000 On 5/19/16 12:12 PM, Konstantin Belousov wrote: > On Thu, May 19, 2016 at 09:31:47AM -0700, Alfred Perlstein wrote: >> 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? > 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 From owner-freebsd-geom@freebsd.org Thu May 19 21:31:34 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 46C02B41DAD for ; Thu, 19 May 2016 21:31:34 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 3147F1724 for ; Thu, 19 May 2016 21:31:34 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id 304DCB41DAB; Thu, 19 May 2016 21:31:34 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2FD37B41DAA; Thu, 19 May 2016 21:31:34 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id A45EB1723; Thu, 19 May 2016 21:31:33 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u4JLVSL7017475 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Fri, 20 May 2016 00:31:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u4JLVSL7017475 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u4JLVSbX017474; Fri, 20 May 2016 00:31:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 20 May 2016 00:31:28 +0300 From: Konstantin Belousov To: Alfred Perlstein Cc: geom@freebsd.org, arch@freebsd.org Subject: Re: Removing Giant asserts from geom Message-ID: <20160519213128.GT89104@kib.kiev.ua> References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org> <20160519191247.GQ89104@kib.kiev.ua> <53397f3f-1056-ceb7-ce3a-5269ac1d29e2@mu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53397f3f-1056-ceb7-ce3a-5269ac1d29e2@mu.org> User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 21:31:34 -0000 On Thu, May 19, 2016 at 01:57:53PM -0700, Alfred Perlstein wrote: > 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. From owner-freebsd-geom@freebsd.org Thu May 19 21:35:36 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 28781B41EEE for ; Thu, 19 May 2016 21:35:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 0910C1A17 for ; Thu, 19 May 2016 21:35:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mailman.ysv.freebsd.org (Postfix) id 04847B41EEB; Thu, 19 May 2016 21:35:36 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 042E7B41EEA for ; Thu, 19 May 2016 21:35:36 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (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 C28231A15 for ; Thu, 19 May 2016 21:35:35 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io0-x22e.google.com with SMTP id p64so33144552ioi.2 for ; Thu, 19 May 2016 14:35:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc; bh=56NIbtNbLlzDvThqDkhfRCWD+zx79zvQY9sMgH1gmTI=; b=ZhiS6hp5LKRKzgyLEg3G+yuyqXIvPpn9V/u0LWFGslG4LDefGmfvZDY5hk3CqYAIKn faoo8SkMFjGQsjwCn6Hmgd42T4Z4W+QxPXey8eiVOWv+rf3QcKljEl7uhV+n4u0G7ZPJ GPz8W0J1iZhQJoJJAIwoY0hKeW9taPj/E0Gd5lWcYS6jvLo81Fzi5y29+m6pU82VNXIr ejvI7/wAMNjt/NZcx9I2a8KZK5VAy+0gOFJU6GvNrSp0AHMndMktuPAyRD75GzvRgQm3 jYhPXgKaIdltE/FH7PlWWF29UjQxJVf7O9McOoumaRl8Hc2jlG/QvWJFoxscBgAvkK7/ NKew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc; bh=56NIbtNbLlzDvThqDkhfRCWD+zx79zvQY9sMgH1gmTI=; b=lNSpdSgk5+rQ+TMaW18y22LPQT0SWX6EmHNqIEAGbtovMb1fbxAXJCW5qi7OyN6cyv y1pAufuRS8kxMb/J0WetJOrXp6HrVVPOggiCKxowqErCICsnrKX9aJInmnXKQGouzOVB E/u80dijYA7ZfST6P1vBhsJovWGMFWgXXSxtPUCRiun/6H6xDxl84trijdzXkHv5b/f5 tq98yQE4jRhPN38DbO713uSpI9o+yKfcQyR1bWKNnLyjxS2gVG3aRexchGm7wTNQv+aB yCtyVqRgkvsQNpyZBIcY6AqlJVohME0SCdQ9trPDmFEErT3uCcVps3hUGGtHTGIHuVQl 6Vig== X-Gm-Message-State: AOPr4FWGRR28LfstWUexmAcrh30AiqMnrZVUZ9a4kgI6u49buFF1pFdQcjOILUBt4s2UKhTBWIttmq/ODqtkqA== MIME-Version: 1.0 X-Received: by 10.36.70.7 with SMTP id j7mr4806886itb.72.1463693735230; Thu, 19 May 2016 14:35:35 -0700 (PDT) Sender: wlosh@bsdimp.com Received: by 10.79.75.68 with HTTP; Thu, 19 May 2016 14:35:35 -0700 (PDT) X-Originating-IP: [50.253.99.174] In-Reply-To: <20160519213128.GT89104@kib.kiev.ua> References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org> <20160519191247.GQ89104@kib.kiev.ua> <53397f3f-1056-ceb7-ce3a-5269ac1d29e2@mu.org> <20160519213128.GT89104@kib.kiev.ua> Date: Thu, 19 May 2016 15:35:35 -0600 X-Google-Sender-Auth: O3xZTgY3zSYoqJ9xkwhh0htQZ7U Message-ID: Subject: Re: Removing Giant asserts from geom From: Warner Losh To: Konstantin Belousov Cc: Alfred Perlstein , geom@freebsd.org, "freebsd-arch@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 21:35:36 -0000 On Thu, May 19, 2016 at 3:31 PM, Konstantin Belousov wrote: > On Thu, May 19, 2016 at 01:57:53PM -0700, Alfred Perlstein wrote: >> 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 From owner-freebsd-geom@freebsd.org Thu May 19 21:42:22 2016 Return-Path: Delivered-To: freebsd-geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 10C92B42251 for ; Thu, 19 May 2016 21:42:22 +0000 (UTC) (envelope-from bright@mu.org) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 00BD41046 for ; Thu, 19 May 2016 21:42:22 +0000 (UTC) (envelope-from bright@mu.org) Received: by mailman.ysv.freebsd.org (Postfix) id EFEE9B4224D; Thu, 19 May 2016 21:42:21 +0000 (UTC) Delivered-To: geom@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id EF579B42249; Thu, 19 May 2016 21:42:21 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [IPv6:2001:470:1f05:b76::196]) by mx1.freebsd.org (Postfix) with ESMTP id E25311043; Thu, 19 May 2016 21:42:21 +0000 (UTC) (envelope-from bright@mu.org) Received: from Alfreds-MacBook-Pro-2.local (c-76-21-10-192.hsd1.ca.comcast.net [76.21.10.192]) by elvis.mu.org (Postfix) with ESMTPSA id 98943346DE2F; Thu, 19 May 2016 14:42:21 -0700 (PDT) Subject: Re: Removing Giant asserts from geom To: Konstantin Belousov References: <20160519105634.GO89104@kib.kiev.ua> <573DEA73.4080408@mu.org> <20160519191247.GQ89104@kib.kiev.ua> <53397f3f-1056-ceb7-ce3a-5269ac1d29e2@mu.org> <20160519213128.GT89104@kib.kiev.ua> Cc: geom@freebsd.org, arch@freebsd.org From: Alfred Perlstein Message-ID: Date: Thu, 19 May 2016 14:43:31 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160519213128.GT89104@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 21:42:22 -0000 On 5/19/16 2:31 PM, Konstantin Belousov wrote: > On Thu, May 19, 2016 at 01:57:53PM -0700, Alfred Perlstein wrote: >> 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