Date: Tue, 17 Jan 2017 12:54:08 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Sean Bruno <sbruno@freebsd.org> 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 Message-ID: <20170117120313.K1043@besplex.bde.org> In-Reply-To: <201701161658.v0GGwD0Z050066@repo.freebsd.org> References: <201701161658.v0GGwD0Z050066@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170117120313.K1043>