svn commit: r326218 - head/sys/kern

[Nathan Whitehorn]

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