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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
