Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Zhang, Qiang" <Qiang.Zhang@windriver.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>,
        "paulmck@kernel.org" <paulmck@kernel.org>
CC:     Rafael Wysocki <rafael.j.wysocki@intel.com>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] PM: sleep: Replace read_lock/unlock(tasklist_lock) with
 rcu_read_lock/unlock()
Thread-Topic: [PATCH] PM: sleep: Replace read_lock/unlock(tasklist_lock) with
 rcu_read_lock/unlock()
Thread-Index: AQHXW2piNMZm7X449EajuzK9DUU2o6sIbnaAgAAiY3c=
Date:   Mon, 7 Jun 2021 14:11:37 +0000
Message-ID: <DM6PR11MB42027F9DA1B3E9B6BDFA5194FF389@DM6PR11MB4202.namprd11.prod.outlook.com>
References: <20210607065743.1596-1-qiang.zhang@windriver.com>,<CAJZ5v0iYtNP54mUFs5VbmHxuXYjTBMrbCZ5CqfaHShnGdd+3Jg@mail.gmail.com>
In-Reply-To: <CAJZ5v0iYtNP54mUFs5VbmHxuXYjTBMrbCZ5CqfaHShnGdd+3Jg@mail.gmail.com>
Accept-Language: en-001, zh-CN, en-US
Content-Language: aa
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9d2922d1-9934-4a37-9e56-08d929be2a2a
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jun 2021 14:11:37.5987
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ddb2873-a1ad-4a18-ae4e-4644631433be
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: B7+Qdb80n3FpcSoiOYbWUJk1KDZ2zjf4xucwkBnB1jZTxmM4U0e0GbEnB/ojDOEmUITRbiWSLTxH75aI6GeWKh7zsUoWe6C5NRp6ieMwJuU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1433
X-Proofpoint-ORIG-GUID: SiUI-DkTPCsSNm1wmGLtS1DE47pj_oMP
X-Proofpoint-GUID: SiUI-DkTPCsSNm1wmGLtS1DE47pj_oMP
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-06-07_11:2021-06-04,2021-06-07 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1011 adultscore=0
 suspectscore=0 phishscore=0 mlxlogscore=999 lowpriorityscore=0 spamscore=0
 malwarescore=0 mlxscore=0 impostorscore=0 priorityscore=1501 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000
 definitions=main-2106070105
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

=0A=
=0A=
________________________________________=0A=
From: Rafael J. Wysocki <rafael@kernel.org>=0A=
Sent: Monday, 7 June 2021 19:45=0A=
To: Zhang, Qiang=0A=
Cc: Rafael Wysocki; Len Brown; Pavel Machek; Paul E. McKenney; Linux PM; Li=
nux Kernel Mailing List=0A=
Subject: Re: [PATCH] PM: sleep: Replace read_lock/unlock(tasklist_lock) wit=
h rcu_read_lock/unlock()=0A=
=0A=
[Please note: This e-mail is from an EXTERNAL e-mail address]=0A=
=0A=
On Mon, Jun 7, 2021 at 8:57 AM <qiang.zhang@windriver.com> wrote:=0A=
>=0A=
> From: Zqiang <qiang.zhang@windriver.com>=0A=
>=0A=
> Using rcu_read_lock/unlock() instead of read_lock/unlock(tasklist_lock),=
=0A=
> the task list can be traversed in parallel to any list additions or=0A=
> removals, improve concurrency.=0A=
>=0A=
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>=0A=
=0A=
>This changes the reader side only AFAICS, but what about the >writer side?=
=0A=
=0A=
Hello  rafael=0A=
=0A=
In the case of holding read_lock(tasklist_lock) =0A=
if there are a lot of tasks in the system that need to be frozen,=0A=
the read_lock(tasklist_lock)  has not been released for a short time,=0A=
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=0A=
may be trigger and report warning.=0A=
=0A=
for example:=0A=
=0A=
copy_process()=0A=
{........=0A=
write_lock_irq(&tasklist_lock);=0A=
........=0A=
list_add_tail_rcu(&p->tasks, &init_task.tasks);=0A=
}=0A=
=0A=
>=0A=
>What exactly is there to ensure that the updates of the list will=0A=
>remain safe after this change?=0A=
=0A=
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 struc=
ture will not be released.=0A=
=0A=
In  ./Documentation/RCU/listRCU.rst , it is also explained.=0A=
=0A=
 Maybe we can ask  Paul E,  Paul E what's your opinion ?=0A=
=0A=
Thanks=0A=
Qiang=0A=
=0A=
> ---=0A=
>  kernel/power/process.c | 16 ++++++++--------=0A=
>  1 file changed, 8 insertions(+), 8 deletions(-)=0A=
>=0A=
> diff --git a/kernel/power/process.c b/kernel/power/process.c=0A=
> index 50cc63534486..0f8dee9ee097 100644=0A=
> --- a/kernel/power/process.c=0A=
> +++ b/kernel/power/process.c=0A=
> @@ -48,7 +48,7 @@ static int try_to_freeze_tasks(bool user_only)=0A=
>=0A=
>         while (true) {=0A=
>                 todo =3D 0;=0A=
> -               read_lock(&tasklist_lock);=0A=
> +               rcu_read_lock();=0A=
>                 for_each_process_thread(g, p) {=0A=
>                         if (p =3D=3D current || !freeze_task(p))=0A=
>                                 continue;=0A=
> @@ -56,7 +56,7 @@ static int try_to_freeze_tasks(bool user_only)=0A=
>                         if (!freezer_should_skip(p))=0A=
>                                 todo++;=0A=
>                 }=0A=
> -               read_unlock(&tasklist_lock);=0A=
> +               rcu_read_unlock();=0A=
>=0A=
>                 if (!user_only) {=0A=
>                         wq_busy =3D freeze_workqueues_busy();=0A=
> @@ -97,13 +97,13 @@ static int try_to_freeze_tasks(bool user_only)=0A=
>                         show_workqueue_state();=0A=
>=0A=
>                 if (!wakeup || pm_debug_messages_on) {=0A=
> -                       read_lock(&tasklist_lock);=0A=
> +                       rcu_read_lock();=0A=
>                         for_each_process_thread(g, p) {=0A=
>                                 if (p !=3D current && !freezer_should_ski=
p(p)=0A=
>                                     && freezing(p) && !frozen(p))=0A=
>                                         sched_show_task(p);=0A=
>                         }=0A=
> -                       read_unlock(&tasklist_lock);=0A=
> +                       rcu_read_unlock();=0A=
>                 }=0A=
>         } else {=0A=
>                 pr_cont("(elapsed %d.%03d seconds) ", elapsed_msecs / 100=
0,=0A=
> @@ -206,13 +206,13 @@ void thaw_processes(void)=0A=
>=0A=
>         cpuset_wait_for_hotplug();=0A=
>=0A=
> -       read_lock(&tasklist_lock);=0A=
> +       rcu_read_lock();=0A=
>         for_each_process_thread(g, p) {=0A=
>                 /* No other threads should have PF_SUSPEND_TASK set */=0A=
>                 WARN_ON((p !=3D curr) && (p->flags & PF_SUSPEND_TASK));=
=0A=
>                 __thaw_task(p);=0A=
>         }=0A=
> -       read_unlock(&tasklist_lock);=0A=
> +       rcu_read_unlock();=0A=
>=0A=
>         WARN_ON(!(curr->flags & PF_SUSPEND_TASK));=0A=
>         curr->flags &=3D ~PF_SUSPEND_TASK;=0A=
> @@ -233,12 +233,12 @@ void thaw_kernel_threads(void)=0A=
>=0A=
>         thaw_workqueues();=0A=
>=0A=
> -       read_lock(&tasklist_lock);=0A=
> +       rcu_read_lock();=0A=
>         for_each_process_thread(g, p) {=0A=
>                 if (p->flags & PF_KTHREAD)=0A=
>                         __thaw_task(p);=0A=
>         }=0A=
> -       read_unlock(&tasklist_lock);=0A=
> +       rcu_read_unlock();=0A=
>=0A=
>         schedule();=0A=
>         pr_cont("done.\n");=0A=
> --=0A=
> 2.17.1=0A=
>=0A=