From owner-svn-src-head@freebsd.org Mon Nov 27 20:43:42 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 16469DE7AA4; Mon, 27 Nov 2017 20:43:42 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DD91763850; Mon, 27 Nov 2017 20:43:41 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id E940610A8BA; Mon, 27 Nov 2017 15:43:40 -0500 (EST) From: John Baldwin To: Nathan Whitehorn Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326218 - head/sys/kern Date: Mon, 27 Nov 2017 11:31:46 -0800 Message-ID: <14322447.103fKFTi3y@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.1-STABLE; KDE/4.14.30; amd64; ; ) In-Reply-To: References: <201711252341.vAPNf5Qx001464@repo.freebsd.org> <3170692.kvv90QqB0X@ralph.baldwin.cx> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Mon, 27 Nov 2017 15:43:41 -0500 (EST) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2017 20:43:42 -0000 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