From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756647AbdJKBYX (ORCPT ); Tue, 10 Oct 2017 21:24:23 -0400 Received: from shelob.surriel.com ([96.67.55.147]:44586 "EHLO shelob.surriel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752099AbdJKBYW (ORCPT ); Tue, 10 Oct 2017 21:24:22 -0400 Message-ID: <1507685048.21121.181.camel@surriel.com> Subject: Re: [PATCH v5 1/2] pid: Replace pid bitmap implementation with IDR API From: Rik van Riel To: Gargi Sharma , linux-kernel@vger.kernel.org Cc: julia.lawall@lip6.fr, akpm@linux-foundation.org, mingo@kernel.org, pasha.tatashin@oracle.com, ktkhai@virtuozzo.com, oleg@redhat.com, ebiederm@xmission.com, hch@infradead.org Date: Tue, 10 Oct 2017 21:24:08 -0400 In-Reply-To: <1507677761-15006-2-git-send-email-gs051095@gmail.com> References: <1507677761-15006-1-git-send-email-gs051095@gmail.com> <1507677761-15006-2-git-send-email-gs051095@gmail.com> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-rFzGD6Jl0dQJg7sJk1x8" X-Mailer: Evolution 3.22.6 (3.22.6-2.fc25) Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-rFzGD6Jl0dQJg7sJk1x8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2017-10-10 at 19:22 -0400, Gargi Sharma wrote: > This patch replaces the current bitmap implemetation for > Process ID allocation. Functions that are no longer required, > for example, free_pidmap(), alloc_pidmap(), etc. are removed. > The rest of the functions are modified to use the IDR API. > The change was made to make the PID allocation less complex by > replacing custom code with calls to generic API. >=20 > Signed-off-by: Gargi Sharma The patch looks mostly good, just a few style nitpicks remaining. > @@ -308,8 +165,28 @@ struct pid *alloc_pid(struct pid_namespace *ns) > =C2=A0 > =C2=A0 tmp =3D ns; > =C2=A0 pid->level =3D ns->level; > + > =C2=A0 for (i =3D ns->level; i >=3D 0; i--) { > - nr =3D alloc_pidmap(tmp); > + int pid_min =3D 1; > + > + idr_preload(GFP_KERNEL); > + spin_lock_irq(&pidmap_lock); > + > + /* > + =C2=A0* init really needs pid 1, but after reaching the > maximum > + =C2=A0* wrap back to RESERVED_PIDS > + =C2=A0*/ > + if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS) > + pid_min =3D RESERVED_PIDS; > + > + /* Store a null pointer so find_pid_ns does not find > + =C2=A0* a partially initialized PID (see below). > + =C2=A0*/ This comment should start with an empty line, just like the one above: /* * Store a null pointer so find_pid_ns does not find a * partially initialized PID (see below). */ > @@ -240,18 +230,15 @@ void zap_pid_ns_processes(struct pid_namespace > *pid_ns) > =C2=A0 =C2=A0* > =C2=A0 =C2=A0*/ > =C2=A0 read_lock(&tasklist_lock); > - nr =3D next_pidmap(pid_ns, 1); > - while (nr > 0) { > - rcu_read_lock(); > + nr =3D 2; > =C2=A0 > - task =3D pid_task(find_vpid(nr), PIDTYPE_PID); > + rcu_read_lock(); I would place the rcu_read_lock() on the line above the read_lock(&tasklist_lock) and the rcu_read_unlock() below the read_unlock(&tasklist_lock). Why? No real reason, except the rcu lock is "a larger lock" than the read-write lock, and it seems to be how things are done commonly... I may be wrong here, and I hope somebody else corrects me if I am wrong :) > + idr_for_each_entry_continue(&pid_ns->idr, pid, nr) { > + task =3D pid_task(pid, PIDTYPE_PID); > =C2=A0 if (task && !__fatal_signal_pending(task)) > =C2=A0 send_sig_info(SIGKILL, SEND_SIG_FORCED, > task); > - > - rcu_read_unlock(); > - > - nr =3D next_pidmap(pid_ns, nr); > =C2=A0 } > + rcu_read_unlock(); > =C2=A0 read_unlock(&tasklist_lock); > =C2=A0 > =C2=A0 /* --=20 All Rights Reversed. --=-rFzGD6Jl0dQJg7sJk1x8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJZ3XK5AAoJEM553pKExN6Dd4kH/2TIo0XjdXANLMIiatdkG8T8 vqQFtq9eAZGbDO2Ql/sgoA7gnsmmIRTHqDRtj0OqUxTFXcv7noAgEHz6/CYlwZUW sHQlDS0Sdn7ibUbIr10dEPi/zCYm/akhXvZYLAneBSZky/azgH8vCC8AuxCWs5rc AumEsvkT2RLGVDlRvNnQsytL3IKJTrpGI3EJINdw0/dtWxDxt9/8J2VwVfS9ZhWD QKHL5h5MK+8SXPngPKJJNTsVEf2poOtIq1JA7QR+oEZ66xwlzb9j26izHxhU+sTx uOvPEysqjSsgtAQmSI+ogL3pP54gp+7aFuxMyzlYhCy+dRAjXR9GS5wlLmEVkzU= =nVP0 -----END PGP SIGNATURE----- --=-rFzGD6Jl0dQJg7sJk1x8--