Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 27 Nov 2017 14:04:59 -0800
From:      Nathan Whitehorn <nwhitehorn@freebsd.org>
To:        John Baldwin <jhb@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r326218 - head/sys/kern
Message-ID:  <3fc45d5f-22b9-0562-278b-c515e36f48e7@freebsd.org>
In-Reply-To: <14322447.103fKFTi3y@ralph.baldwin.cx>
References:  <201711252341.vAPNf5Qx001464@repo.freebsd.org> <3170692.kvv90QqB0X@ralph.baldwin.cx> <abfdf2b1-bb2f-4eb1-a448-bfee68158577@freebsd.org> <14322447.103fKFTi3y@ralph.baldwin.cx>

next in thread | previous in thread | raw e-mail | index | archive | help


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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3fc45d5f-22b9-0562-278b-c515e36f48e7>