From owner-freebsd-hackers@freebsd.org Tue May 30 01:48:37 2017 Return-Path: Delivered-To: freebsd-hackers@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 D5F68D89587 for ; Tue, 30 May 2017 01:48:37 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: from asp.reflexion.net (outbound-mail-210-63.reflexion.net [208.70.210.63]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 8B3936FE52 for ; Tue, 30 May 2017 01:48:36 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: (qmail 18216 invoked from network); 30 May 2017 01:49:49 -0000 Received: from unknown (HELO rtc-sm-01.app.dca.reflexion.local) (10.81.150.1) by 0 (rfx-qmail) with SMTP; 30 May 2017 01:49:49 -0000 Received: by rtc-sm-01.app.dca.reflexion.local (Reflexion email security v8.40.0) with SMTP; Mon, 29 May 2017 21:48:30 -0400 (EDT) Received: (qmail 9279 invoked from network); 30 May 2017 01:48:30 -0000 Received: from unknown (HELO iron2.pdx.net) (69.64.224.71) by 0 (rfx-qmail) with (AES256-SHA encrypted) SMTP; 30 May 2017 01:48:30 -0000 Received: from [192.168.1.114] (c-76-115-7-162.hsd1.or.comcast.net [76.115.7.162]) by iron2.pdx.net (Postfix) with ESMTPSA id 5AE35EC7652; Mon, 29 May 2017 18:48:29 -0700 (PDT) From: Mark Millard Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: cpuset_setithread and PROC_LOCK vs. thread_lock usage: Is this correct? Message-Id: <433AC404-E85D-4910-8638-4EBC3ACBBCDB@dsl-only.net> Date: Mon, 29 May 2017 18:48:28 -0700 Cc: FreeBSD PowerPC ML To: freebsd-hackers@freebsd.org X-Mailer: Apple Mail (2.3273) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 May 2017 01:48:37 -0000 [FYI: The source referenced is from head -r317820.] In the process of trying to get evidence related to a periodic/random kernel panic for TARGET_ARCH=3Dpowerpc I ran into what follows. I've no evidence that it is tied to the panics. It is just something that I read that looked a little odd to me. But it looked like a good thing to ask about, at least based on my level of ignorance of such things for the kernel. (The more I learn the better.) I normally see the likes of: thread_lock(td); tdset =3D td->td_cpuset; and: thread_lock(td); error =3D cpuset_shadow(td->td_cpuset, nset, mask); but in cpuset_setithread the comments read like a PROC_LOCK is in use instead: int cpuset_setithread(lwpid_t id, int cpu) { . . . struct proc *p; . . . error =3D cpuset_which(CPU_WHICH_TID, id, &p, &td, &old_set); if (error !=3D 0 || ((cs_id =3D alloc_unr(cpuset_unr)) =3D=3D = CPUSET_INVALID)) goto out; /* cpuset_which() returns with PROC_LOCK held. */ old_set =3D td->td_cpuset; . . . That seems true for !error but is a different lock than used elsewhere (those thread_lock's). cpuset_which does seem to end up with the PROC_LOCK as indicated: int cpuset_which(cpuwhich_t which, id_t id, struct proc **pp, struct thread = **tdp, struct cpuset **setp) { . . . case CPU_WHICH_TID: if (id =3D=3D -1) { PROC_LOCK(curproc); p =3D curproc; td =3D curthread; break; } td =3D tdfind(id, -1); if (td =3D=3D NULL) return (ESRCH); p =3D td->td_proc; break; . . . error =3D p_cansched(curthread, p); if (error) { PROC_UNLOCK(p); return (error); } if (td =3D=3D NULL) td =3D FIRST_THREAD_IN_PROC(p); *pp =3D p; *tdp =3D td; return (0); } (tdfind does rw_rlock on tidhash_lock, PROC_LOCK on td->td_proc. It only PROC_UNLOCK's for PRS_NEW and is comments to return with the proc lock held. No thread_lock's involved.) So it appears to me that in cpuset_setithread : /* cpuset_which() returns with PROC_LOCK held. */ old_set =3D td->td_cpuset; might happen with no thread_lock protecting the td use (say to avoid reading during updates). (This might also apply to what old_set points to.) Similarly for: cpuset_setithread(lwpid_t id, int cpu) { . . . struct cpuset *parent, *old_set; . . . error =3D cpuset_shadow(parent, nset, &mask); . . . it does not appear that a thread_lock is involved around the cpuset_shadow operation, unlike elsewhere. Is this lack of thread_lock involvement in fact okay for some reason in each case? =3D=3D=3D Mark Millard markmi at dsl-only.net