From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83529C47095 for ; Mon, 7 Jun 2021 15:41:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6D01160231 for ; Mon, 7 Jun 2021 15:41:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231624AbhFGPnm (ORCPT ); Mon, 7 Jun 2021 11:43:42 -0400 Received: from mail.kernel.org ([198.145.29.99]:50952 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231488AbhFGPna (ORCPT ); Mon, 7 Jun 2021 11:43:30 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 3A90F6100B; Mon, 7 Jun 2021 15:41:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1623080499; bh=HirXEOeTJC8OXEqZozB9U77eq3qf8a0FWCrZd8Q9AKs=; h=Date:From:To:Cc:Subject:Reply-To:References:In-Reply-To:From; b=fSXmWgzO2Hz+MaCRW7VyB3tXJvC8q0DCRoxX3R3pVkT4vRriI8Gpqx1BRVrwy0ugZ c8DbQxy+7rA73hfTW0eOyglludJJ/t4nTXl0HQEgEEsuvgXq4Na9X5H9yeGCzQhSL/ NEEvsS0ePGGJE5ji3inhIuBWuANVQInniQuSyCIxpUtC2Nr0uH3M8PUhj600cu4VvO ioBRU5iARgl5gmsSNXtdT9qjow2WacDVAmuyZ+gdX3fnbA6B4EHKfSvyTLGfSaKVUl FLgb/esdyWdZgi6onIytKB4UF8oOmN6WZJgzf8p0Pl4zZbSDU0fG0eieqJhqFjBaZz u3P1AazMZbncg== Received: by paulmck-ThinkPad-P17-Gen-1.home (Postfix, from userid 1000) id F37D65C0395; Mon, 7 Jun 2021 08:41:38 -0700 (PDT) Date: Mon, 7 Jun 2021 08:41:38 -0700 From: "Paul E. McKenney" To: "Zhang, Qiang" Cc: "Rafael J. Wysocki" , Rafael Wysocki , Len Brown , Pavel Machek , Linux PM , Linux Kernel Mailing List Subject: Re: [PATCH] PM: sleep: Replace read_lock/unlock(tasklist_lock) with rcu_read_lock/unlock() Message-ID: <20210607154138.GU4397@paulmck-ThinkPad-P17-Gen-1> Reply-To: paulmck@kernel.org References: <20210607065743.1596-1-qiang.zhang@windriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 07, 2021 at 02:11:37PM +0000, Zhang, Qiang wrote: > ________________________________________ > From: Rafael J. Wysocki > Sent: Monday, 7 June 2021 19:45 > To: Zhang, Qiang > Cc: Rafael Wysocki; Len Brown; Pavel Machek; Paul E. McKenney; Linux PM; Linux Kernel Mailing List > Subject: Re: [PATCH] PM: sleep: Replace read_lock/unlock(tasklist_lock) with rcu_read_lock/unlock() > > [Please note: This e-mail is from an EXTERNAL e-mail address] > > On Mon, Jun 7, 2021 at 8:57 AM wrote: > > > > From: Zqiang > > > > Using rcu_read_lock/unlock() instead of read_lock/unlock(tasklist_lock), > > the task list can be traversed in parallel to any list additions or > > removals, improve concurrency. > > > > Signed-off-by: Zqiang > > >This changes the reader side only AFAICS, but what about the >writer side? > > Hello rafael > > In the case of holding read_lock(tasklist_lock) > if there are a lot of tasks in the system that need to be frozen, > the read_lock(tasklist_lock) has not been released for a short time, > when clone task, we will acquire write_lock(tasklist_lock) and insert task to list, if Ifcan't get the write lock all the time. the hung task > may be trigger and report warning. > > for example: > > copy_process() > {........ > write_lock_irq(&tasklist_lock); > ........ > list_add_tail_rcu(&p->tasks, &init_task.tasks); > } > > > > >What exactly is there to ensure that the updates of the list will > >remain safe after this change? > > The RCU can guarantee that the list can be traversed and added at the same time, and in the read critical area, it is guaranteed that the task structure will not be released. > > In ./Documentation/RCU/listRCU.rst , it is also explained. > > Maybe we can ask Paul E, Paul E what's your opinion ? Although RCU does guarantee that the task structures are not freed while an RCU reader references them, that is only one of the questions that must be answered. Changing from read_lock(&tasklist_lock) to rcu_read_lock() also allows all of the code currently under write_lock(&tasklist_lock) to execute concurrently with these newly created RCU readers. Is that safe? If it is safe, why? For example, the first pair of hunks in the diff -might- be safe because "todo" is updated while read-holding tasklist_lock, and then that value is used after releasing that lock. But I do not know this code well, and it is quite possible that something else is going on. Thanx, Paul > Thanks > Qiang > > > --- > > kernel/power/process.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/power/process.c b/kernel/power/process.c > > index 50cc63534486..0f8dee9ee097 100644 > > --- a/kernel/power/process.c > > +++ b/kernel/power/process.c > > @@ -48,7 +48,7 @@ static int try_to_freeze_tasks(bool user_only) > > > > while (true) { > > todo = 0; > > - read_lock(&tasklist_lock); > > + rcu_read_lock(); > > for_each_process_thread(g, p) { > > if (p == current || !freeze_task(p)) > > continue; > > @@ -56,7 +56,7 @@ static int try_to_freeze_tasks(bool user_only) > > if (!freezer_should_skip(p)) > > todo++; > > } > > - read_unlock(&tasklist_lock); > > + rcu_read_unlock(); > > > > if (!user_only) { > > wq_busy = freeze_workqueues_busy(); > > @@ -97,13 +97,13 @@ static int try_to_freeze_tasks(bool user_only) > > show_workqueue_state(); > > > > if (!wakeup || pm_debug_messages_on) { > > - read_lock(&tasklist_lock); > > + rcu_read_lock(); > > for_each_process_thread(g, p) { > > if (p != current && !freezer_should_skip(p) > > && freezing(p) && !frozen(p)) > > sched_show_task(p); > > } > > - read_unlock(&tasklist_lock); > > + rcu_read_unlock(); > > } > > } else { > > pr_cont("(elapsed %d.%03d seconds) ", elapsed_msecs / 1000, > > @@ -206,13 +206,13 @@ void thaw_processes(void) > > > > cpuset_wait_for_hotplug(); > > > > - read_lock(&tasklist_lock); > > + rcu_read_lock(); > > for_each_process_thread(g, p) { > > /* No other threads should have PF_SUSPEND_TASK set */ > > WARN_ON((p != curr) && (p->flags & PF_SUSPEND_TASK)); > > __thaw_task(p); > > } > > - read_unlock(&tasklist_lock); > > + rcu_read_unlock(); > > > > WARN_ON(!(curr->flags & PF_SUSPEND_TASK)); > > curr->flags &= ~PF_SUSPEND_TASK; > > @@ -233,12 +233,12 @@ void thaw_kernel_threads(void) > > > > thaw_workqueues(); > > > > - read_lock(&tasklist_lock); > > + rcu_read_lock(); > > for_each_process_thread(g, p) { > > if (p->flags & PF_KTHREAD) > > __thaw_task(p); > > } > > - read_unlock(&tasklist_lock); > > + rcu_read_unlock(); > > > > schedule(); > > pr_cont("done.\n"); > > -- > > 2.17.1 > >