On 11/26/17 20:50, John Baldwin wrote:
On Saturday, November 25, 2017 11:41:05 PM Nathan Whitehorn wrote:
Author: nwhitehorn
Date: Sat Nov 25 23:41:05 2017
New Revision: 326218
URL: https://svnweb.freebsd.org/changeset/base/326218
Log:
Remove some, but not all, assumptions that the BSP is CPU 0 and that CPUs
are numbered densely from there to n_cpus.
MFC after: 1 month
Modified:
head/sys/kern/kern_clock.c
head/sys/kern/kern_clocksource.c
head/sys/kern/kern_shutdown.c
head/sys/kern/kern_timeout.c
head/sys/kern/sched_ule.c
head/sys/kern/subr_pcpu.c
Modified: head/sys/kern/kern_clock.c
==============================================================================
--- head/sys/kern/kern_clock.c Sat Nov 25 23:23:24 2017 (r326217)
+++ head/sys/kern/kern_clock.c Sat Nov 25 23:41:05 2017 (r326218)
@@ -573,7 +573,9 @@ hardclock_cnt(int cnt, int usermode)
void
hardclock_sync(int cpu)
{
- int *t = DPCPU_ID_PTR(cpu, pcputicks);
+ int *t;
+ KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu));
Blank line before the KASSERT() perhaps?
+ t = DPCPU_ID_PTR(cpu, pcputicks);
*t = ticks;
Probably don't need this blank line though?
Those are both good ideas.
}
Modified: head/sys/kern/sched_ule.c
==============================================================================
--- head/sys/kern/sched_ule.c Sat Nov 25 23:23:24 2017 (r326217)
+++ head/sys/kern/sched_ule.c Sat Nov 25 23:41:05 2017 (r326218)
@@ -2444,6 +2451,7 @@ sched_add(struct thread *td, int flags)
* Pick the destination cpu and if it isn't ours transfer to the
* target cpu.
*/
+ td_get_sched(td)->ts_cpu = curcpu; /* Pick something valid to start */
cpu = sched_pickcpu(td, flags);
It is not obvious why every sched_add() needs this once you've fixed thread0.
Shouldn't new threads just inherit from thread0's already-fixed value? If not,
perhaps fix thread0's value sooner?
That's a fair point. I don't remember the rationale for this now; the
changes are over a year old from the powernv branch. I do remember
setting thread0's CPU early not working, but have forgotten why. I will
try to remember...
tdq = sched_setcpu(td, cpu, flags);
tdq_add(tdq, td, flags);
Modified: head/sys/kern/subr_pcpu.c
==============================================================================
--- head/sys/kern/subr_pcpu.c Sat Nov 25 23:23:24 2017 (r326217)
+++ head/sys/kern/subr_pcpu.c Sat Nov 25 23:41:05 2017 (r326218)
@@ -279,6 +279,8 @@ pcpu_destroy(struct pcpu *pcpu)
struct pcpu *
pcpu_find(u_int cpuid)
{
+ KASSERT(cpuid_to_pcpu[cpuid] != NULL,
+ ("Getting uninitialized PCPU %d", cpuid));
This KASSERT seems unnecessary? If the caller uses an invalid one, it will
just fault anyway.
It won't necessarily fault. For example, on PowerPC, NULL is a valid
address that does not trigger faults. It's unfortunately quite
complicated to fix this in a general way. Even if it did fault, this
makes the fault more informative (and has found at least one bug on
arm64 already).
return (cpuid_to_pcpu[cpuid]);
}
@@ -409,7 +411,7 @@ DB_SHOW_ALL_COMMAND(pcpu, db_show_cpu_all)
int id;
db_printf("Current CPU: %d\n\n", PCPU_GET(cpuid));
- for (id = 0; id <= mp_maxid; id++) {
+ CPU_FOREACH(id) {
If you remove the KASSERT you don't need this change since it checks the return
value of pcpu_find() (which you didn't change). In particular, this DDB command
shows all valid pcpu structures safely even if that set is inconsistent with
the all_cpus mask (or the old version did at least). There is also nothing
about
this that assumes BSP == 0 either. CPU_FOREACH() is doing a loop from 0 to
mp_maxid under the covers as well.
True. CPU_FOREACH just seemed simpler here and future-proof if it ever
started doing something more complex.
-Nathan
pc = pcpu_find(id);
if (pc != NULL) {
show_pcpu(pc);
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
