svn commit: r326218 - head/sys/kern

[John Baldwin]

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).

-- 
John Baldwin