On 11/27/17 11:31, John Baldwin wrote:
On Sunday, November 26, 2017 10:06:56 PM Nathan Whitehorn wrote:
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).
Given the large number of bugs caused by NULL pointers, having NULL pointers
"work" doesn't seem like a long-term tenable solution. You'd have to go add
explicit KASSERT()'s to every pointer indirection in the kernel which seems
unworkable. Also, adding the KASSERT() here has broken other places that were
depending on the existing behavior of pcpu_find() (see markj@'s commit to dtrace
today).
Unfortunately, it's unfixable on ppc64. Apologies for breaking dtrace!
Would you like me to remove the KASSERT() here? I'm happy to do that in
a few hours (unless you beat me to it first) -- although I do think that
explicitly checking for CPU_ABSENT is a much better behavior in client
code than checking the return value of pcpu_find().
-Nathan
_______________________________________________
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"
