From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932424AbdJJQLt (ORCPT ); Tue, 10 Oct 2017 12:11:49 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:34949 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932271AbdJJQLr (ORCPT ); Tue, 10 Oct 2017 12:11:47 -0400 X-Google-Smtp-Source: AOwi7QCCbkCY7kXEQ5tZXiGjcr8JnzWtopxNSgUA2mzqtugClXYUU+Vl75hKuuoz4ONbu8TnEUSlqmqZ/A1bhblnWDY= MIME-Version: 1.0 In-Reply-To: <1507650370.10046.41.camel@surriel.com> References: <1507583624-22146-1-git-send-email-gs051095@gmail.com> <1507583624-22146-2-git-send-email-gs051095@gmail.com> <20171009161737.ea8c62441cc12dfd909ee0b2@linux-foundation.org> <20171010115034.GA28545@redhat.com> <1507650370.10046.41.camel@surriel.com> From: Gargi Sharma Date: Tue, 10 Oct 2017 17:11:16 +0100 Message-ID: Subject: Re: [PATCH v4 1/2] pid: Replace pid bitmap implementation with IDR API To: Rik van Riel Cc: Oleg Nesterov , Andrew Morton , linux-kernel@vger.kernel.org, Julia Lawall , mingo@kernel.org, pasha.tatashin@oracle.com, ktkhai@virtuozzo.com, "Eric W. Biederman" , Christoph Hellwig Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 10, 2017 at 4:46 PM, Rik van Riel wrote: > On Tue, 2017-10-10 at 13:35 +0100, Gargi Sharma wrote: >> On Tue, Oct 10, 2017 at 12:50 PM, Oleg Nesterov >> wrote: >> > On 10/09, Andrew Morton wrote: >> > > >> > > > @@ -240,17 +230,11 @@ void zap_pid_ns_processes(struct >> > > > pid_namespace *pid_ns) >> > > > * >> > > > */ >> > > > read_lock(&tasklist_lock); >> > > > - nr = next_pidmap(pid_ns, 1); >> > > > - while (nr > 0) { >> > > > - rcu_read_lock(); >> > > > - >> > > > - task = pid_task(find_vpid(nr), PIDTYPE_PID); >> > > > + nr = 2; >> > > > + idr_for_each_entry_continue(&pid_ns->idr, pid, nr) { >> > > > + task = pid_task(pid, PIDTYPE_PID); >> > > > if (task && !__fatal_signal_pending(task)) >> > > > send_sig_info(SIGKILL, SEND_SIG_FORCED, >> > > > task); >> > > > - >> > > > - rcu_read_unlock(); >> > > > - >> > > > - nr = next_pidmap(pid_ns, nr); >> > > > } >> > > > read_unlock(&tasklist_lock); >> > > >> > > Especially here. I don't think pidmap_lock is held. Is that IDR >> > > iteration safe? >> > >> > Yes, this doesn't look right, we need rcu_read_lock() or >> > pidmap_lock. >> > >> > And, we also need rcu_read_lock() for another reason, to protect >> > "struct pid". >> >> Ah, I missed this. From what I understood idr_for_each_entry_continue >> should be safe because calls idr_get_next which in turn calls >> radix_tree_iter_find to find the next populated entry in the idr. If >> the pid that you are looking up the task for is deleted, task will >> get >> a NULL from pid_task and no signal to kill will be sent. >> > >> > Gargi, I suggested to use idr_for_each_entry_continue(), but now I >> > am wondering >> > if we should use idr_for_each() instead. IIUC this would be a bit >> > faster? Not >> > that I think this is really important... >> >> I can run benchmarks with idr_for_each to see how much speed up is >> achieved and then we can go with whatever we think is better. How >> does >> that sounds? > > I suspect this code will not be a hot path in any > conceivable "kill off hundreds of containers" > benchmark, since the overhead of having all of the > tasks in those containers exit will dwarf any > changes in this code. > > Simply making it safe for fully preemptible > kernels by adding rcu_read_lock() around the > section is what matters the most. > > The choice between idr_for_each_entry_continue() > and idr_for_each() is dictated more by which > of the two results in easier to read code. I have listed down the code for both idr_for_each and idr_for_each_entry. IMHO idr_for_each_entry is easier to read, but YMMV. :) void kill_task(int id, void *ptr, void *data) { struct *pid = ptr; struct task_struct *task = pid_task(pid, PIDTYPE_PID); if (task && !__fatal_signal_pending(task)) send_sig_info(SIGKILL, SEND_SIG_FORCED, task); } rcu_read_unlock(); idr_for_each(&pid_ns->idr, &kill_task, NULL); rcu_read_unlock(); VS idr_for_each_entry_continue(&pid_ns->idr, pid, nr) { task = pid_task(pid, PIDTYPE_PID); if (task && !__fatal_signal_pending(task)) send_sig_info(SIGKILL, SEND_SIG_FORCED, task); } Thanks! Gargi > > -- > All rights reversed