Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752942AbdI0NKN (ORCPT ); Wed, 27 Sep 2017 09:10:13 -0400 Received: from shelob.surriel.com ([96.67.55.147]:40472 "EHLO shelob.surriel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752882AbdI0NKL (ORCPT ); Wed, 27 Sep 2017 09:10:11 -0400 Message-ID: <1506517794.21121.93.camel@surriel.com> Subject: Re: [PATCH v2 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 Date: Wed, 27 Sep 2017 09:09:54 -0400 In-Reply-To: References: Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-DwrTbK4kpJn65BxS9rRH" 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 Content-Length: 3084 Lines: 90 --=-DwrTbK4kpJn65BxS9rRH Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2017-09-27 at 01:06 -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. I like where this is going. Just a few last comments from me :) > --- a/arch/powerpc/platforms/cell/spufs/sched.c > +++ b/arch/powerpc/platforms/cell/spufs/sched.c > @@ -1093,7 +1093,7 @@ static int show_spu_loadavg(struct seq_file *s, > void *private) > =C2=A0 LOAD_INT(c), LOAD_FRAC(c), > =C2=A0 count_active_contexts(), > =C2=A0 atomic_read(&nr_spu_contexts), > - task_active_pid_ns(current)->last_pid); > + task_active_pid_ns(current)->idr.idr_next-1); > =C2=A0 return 0; > =C2=A0} Everywhere you use idr.idr_next or idr->idr_next directly, you could use idr_get_cursor(idr) instead, exposing less of the IDR internals to the rest of the code. > @@ -240,17 +230,18 @@ 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(); > + pid =3D idr_get_next(&pid_ns->idr, &nr); > + while (pid) { > =C2=A0 > - task =3D pid_task(find_vpid(nr), PIDTYPE_PID); > - if (task && !__fatal_signal_pending(task)) > - send_sig_info(SIGKILL, SEND_SIG_FORCED, > task); > + rcu_read_lock(); > =C2=A0 > + idr_for_each_entry_continue(&pid_ns->idr, pid, nr) { > + task =3D pid_task(pid, PIDTYPE_PID); > + if (task && !__fatal_signal_pending(task)) > + send_sig_info(SIGKILL, > SEND_SIG_FORCED, task); > + } > =C2=A0 rcu_read_unlock(); > =C2=A0 > - nr =3D next_pidmap(pid_ns, nr); > =C2=A0 } > =C2=A0 read_unlock(&tasklist_lock); > =C2=A0 I believe we should be fine with just the idr_for_each_entry_continue() surrounding the loop, and not need the while (pid) around that. That should still iterate over all the pids in the namespace, and simplify the code even more. You have done a great job understanding some complicated code, and simplifying it during your Outreachy internship. --=20 All Rights Reversed. --=-DwrTbK4kpJn65BxS9rRH 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 iQEcBAABCAAGBQJZy6MiAAoJEM553pKExN6DKMUH/0o4gFqmmkWkc/CzRO3wWJrb emhivpbUH5xYxU+w/rkVYle+3TJvg4+SFiBtvLLaAg9gVxwKoxHi0qKh75KpbbIX hY3Y3V4Yn2vltcDbrXj/C6i5Zsd8VOA4XOASrZzFzsVl4iZM6No+4JuHhPFRSXGb JPi/e0f1SUajEeNhNKFORfkaEHtsXnfG2+7aivY/g83J44Um3pLAZu8lPrIIrKFl +FQAmuj64MEExWl/hMVDg/t/jCPnmXAjO813zD03Gk73EY8R2CUocFkF1X0DtIES h/3qdXfBNtwd8m0qO0vV2+hztceVwRdaapiMhYdbzhpexwxEoPvaCAi+wwCq/R4= =5IIv -----END PGP SIGNATURE----- --=-DwrTbK4kpJn65BxS9rRH--