From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756190AbdJJMf4 (ORCPT ); Tue, 10 Oct 2017 08:35:56 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:36596 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755349AbdJJMfy (ORCPT ); Tue, 10 Oct 2017 08:35:54 -0400 X-Google-Smtp-Source: AOwi7QCMhScWpdMtzJYb5JsIhscgdAa4de986mU8JGbovrj7pL/BgtWhxa5dmmlAfmnA+TA324XnttdWdJV2qpnn3Zs= MIME-Version: 1.0 In-Reply-To: <20171010115034.GA28545@redhat.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> From: Gargi Sharma Date: Tue, 10 Oct 2017 13:35:23 +0100 Message-ID: Subject: Re: [PATCH v4 1/2] pid: Replace pid bitmap implementation with IDR API To: Oleg Nesterov Cc: Andrew Morton , linux-kernel@vger.kernel.org, Rik van Riel , 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 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? Thanks! Gargi > > Oleg. >