svn commit: r326218 - head/sys/kern

[John Baldwin]

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?

>  }
> 
> 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?

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

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

>  		pc = pcpu_find(id);
>  		if (pc != NULL) {
>  			show_pcpu(pc);
> 


-- 
John Baldwin