From owner-svn-src-head@freebsd.org Tue Jan 17 01:54:16 2017 Return-Path: Delivered-To: svn-src-head@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 E3E9DCB39AA; Tue, 17 Jan 2017 01:54:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 787CE1B32; Tue, 17 Jan 2017 01:54:16 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 454D1D45F9C; Tue, 17 Jan 2017 12:54:08 +1100 (AEDT) Date: Tue, 17 Jan 2017 12:54:08 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Sean Bruno cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312293 - in head/sys: kern sys In-Reply-To: <201701161658.v0GGwD0Z050066@repo.freebsd.org> Message-ID: <20170117120313.K1043@besplex.bde.org> References: <201701161658.v0GGwD0Z050066@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=BKLDlBYG c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=_hRaNZH9cyDBuD_93R8A:9 a=V7xSkV4eWzfH1oLK:21 a=wwmg7bA4uAeZwAJW:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2017 01:54:17 -0000 On Mon, 16 Jan 2017, Sean Bruno wrote: > Log: > Change startup order for the no EARLY_AP_STARTUP case to initialize > gtaskqueue bits at SI_SUB_INIT_IF instead of waiting until SI_SUB_SMP > which is far too late. > > Add an assertion in taskqgroup_attach() to catch startup initialization > failures in the future. > > Reported by: kib bde I don't see how this can work. The purpose of the EARLY_AP_STARTUP ifdef is to split up the initialization so that part of it is much later in the !EARLY_AP_STARTUP case. The second part was too late to work, except possibly in the UP case where the split isn't needed. This commit turns the split into almost a non-split and does the second part too early to work. Also, I don't see how dynamic management of CPUs can work. The adjustment function was delayed so that it can see the correct number of CPUs. I think this number can change dynamically later still, but the adjustment is static so it doesn't run again. > Modified: head/sys/kern/subr_gtaskqueue.c > ============================================================================== > --- head/sys/kern/subr_gtaskqueue.c Mon Jan 16 16:44:13 2017 (r312292) > +++ head/sys/kern/subr_gtaskqueue.c Mon Jan 16 16:58:12 2017 (r312293) > @@ -646,6 +646,7 @@ taskqgroup_attach(struct taskqgroup *qgr > qid = taskqgroup_find(qgroup, uniq); > qgroup->tqg_queue[qid].tgc_cnt++; > LIST_INSERT_HEAD(&qgroup->tqg_queue[qid].tgc_tasks, gtask, gt_list); > + MPASS(qgroup->tqg_queue[qid].tgc_taskq != NULL); This was removed in the next commit. Failure to run the adjustment function early enough to work (or at all) results in this assertion failing. I don't see how the pointer can become non-null later if this assertion fails, so this shouldn't be removed. > Modified: head/sys/sys/gtaskqueue.h > ============================================================================== > --- head/sys/sys/gtaskqueue.h Mon Jan 16 16:44:13 2017 (r312292) > +++ head/sys/sys/gtaskqueue.h Mon Jan 16 16:58:12 2017 (r312293) > @@ -115,7 +115,7 @@ taskqgroup_adjust_##name(void *arg) > taskqgroup_adjust(qgroup_##name, (cnt), (stride)); \ > } \ > \ > -SYSINIT(taskqgroup_adj_##name, SI_SUB_SMP, SI_ORDER_ANY, \ > +SYSINIT(taskqgroup_adj_##name, SI_SUB_INIT_IF, SI_ORDER_ANY, \ > taskqgroup_adjust_##name, NULL); \ > \ > struct __hack Note that in the EARLY_AP_STARTUP case, this operation is done sequentially at SI_SUB_INIT_IF:SI_ORDER_FIRST in the taskgroup_define* method. In the !EARLY_AP_STARTUP case, the order was significantly different, but now it is just: taskqgroup_define_##name, SI_SUB_INIT_IF, SI_ORDER_FIRST, taskqgroup_adj_##name, SI_SUB_INIT_IF, SI_ORDER_ANY, (or even the reverse, if ANY really means "any", but according to its comment it means "last". It is last numerically, and I think it really is last). So split is almost null -- only a few other SI_SUB_INIT_IF's can run before the last one, and the APs are never started before this. The ifdef is still on EARLY_AP_STARTUP, so makes no sense now. Testing shows that this works as predicted: with !EARLY_AP_STARTUP: - UP case now works (because taskqgroup_adj_##name now runs early enough to work) - SMP case still gives null pointer panic (because taskqgroup_adj_##name now runs too early to work (cnt = 1, stride = 1, smp_started = 0, mp_ncpus = 8). After this commit, smp_started is always 0 at attach time for obvious reasons, but this only causes _taskqgroup_adj_##name to do nothing in the SMP case (mp_ncpus != 1). Bruce