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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D4A36C433EF for ; Tue, 31 May 2022 16:09:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 68C346B0080; Tue, 31 May 2022 12:09:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6106F6B0081; Tue, 31 May 2022 12:09:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4B3006B0082; Tue, 31 May 2022 12:09:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 39C996B0080 for ; Tue, 31 May 2022 12:09:55 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id EEA972142E for ; Tue, 31 May 2022 16:09:54 +0000 (UTC) X-FDA: 79526524308.01.52486F1 Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) by imf06.hostedemail.com (Postfix) with ESMTP id 77E9518004D for ; Tue, 31 May 2022 16:09:50 +0000 (UTC) Received: from in01.mta.xmission.com ([166.70.13.51]:46482) by out03.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1nw4RG-001g9g-Vu; Tue, 31 May 2022 10:09:47 -0600 Received: from ip68-227-174-4.om.om.cox.net ([68.227.174.4]:40316 helo=email.froward.int.ebiederm.org.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1nw4RF-007jjf-IH; Tue, 31 May 2022 10:09:46 -0600 From: "Eric W. Biederman" To: Miaohe Lin Cc: Ying Huang , , , , , , , , , , , , References: <20220530113016.16663-1-linmiaohe@huawei.com> <20220530113016.16663-2-linmiaohe@huawei.com> Date: Tue, 31 May 2022 11:09:01 -0500 In-Reply-To: (Miaohe Lin's message of "Tue, 31 May 2022 17:01:09 +0800") Message-ID: <87bkvdfzvm.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1nw4RF-007jjf-IH;;;mid=<87bkvdfzvm.fsf@email.froward.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.174.4;;;frm=ebiederm@xmission.com;;;spf=softfail X-XM-AID: U2FsdGVkX1/wo32upDdcYiQB3D7INFrZxxd/vhQS/wg= X-SA-Exim-Connect-IP: 68.227.174.4 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v4 1/4] mm: reduce the rcu lock duration X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 77E9518004D Authentication-Results: imf06.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=xmission.com; spf=pass (imf06.hostedemail.com: domain of ebiederm@xmission.com designates 166.70.13.233 as permitted sender) smtp.mailfrom=ebiederm@xmission.com X-Stat-Signature: 3naw95nq4tiqd55mwkuk8gdyr779x7ra X-Rspam-User: X-HE-Tag: 1654013390-255288 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Miaohe Lin writes: > On 2022/5/31 14:06, Ying Huang wrote: >> On Mon, 2022-05-30 at 19:30 +0800, Miaohe Lin wrote: >>> Commit 3268c63eded4 ("mm: fix move/migrate_pages() race on task struct") >>> extends the period of the rcu_read_lock until after the permissions checks >>> are done to prevent the task pointed to from changing from under us. But >>> the task_struct refcount is also taken at that time, the reference to task >>> is guaranteed to be stable. So it's unnecessary to extend the period of >>> the rcu_read_lock. Release the rcu lock after task refcount is successfully >>> grabbed to reduce the rcu holding time. >> >> Sorry for late reply, I am busy on something else recently. > > That's all right. Many thanks for your hard work. :) > >> >> I have just read the whole thread of the original patch discussion. >> During discussion, in >> >> https://lore.kernel.org/lkml/alpine.DEB.2.00.1202241131400.3726@router.home/ >> >> a patch that is same as your one is proposed. Then in the following >> message, Eric think that the rcu read lock should be released until >> permission is checked, >> >> https://lore.kernel.org/lkml/87sjhzun47.fsf@xmission.com/ >> >> " >> At the moment I suspect the permissions checks are not safe unless >> performed under both rcu_read_lock and task_lock to ensure that >> the task<->mm association does not change on us while we are >> working. Even with that the cred can change under us but at least >> we know the cred will be valid until rcu_read_unlock happens. >> " >> >> So the rcu lock duration is enlarged in the following message. >> >> https://lore.kernel.org/lkml/alpine.DEB.2.00.1202271238450.32410@router.home/ >> >> But, after some thought, I don't think extended rcu read lock adds much >> value. Because after permission checking the permission may still be >> changed. There's no much difference. >> >> So, I have no objection to the patch itself. But you should add more >> information in patch description about why the RCU proected region is >> extended and why we can reduce it. > > Does below patch description makes sense for you? > > " > Commit 3268c63eded4 ("mm: fix move/migrate_pages() race on task struct") > extends the period of the rcu_read_lock until after the permissions checks > are done because it suspects the permissions checks are not safe unless > performed under both rcu_read_lock and task_lock to ensure the task<->mm > association does not change on us while we are working [1]. But extended > rcu read lock does not add much value. Because after permission checking > the permission may still be changed. There's no much difference. So it's > unnecessary to extend the period of the rcu_read_lock. Release the rcu > lock after task refcount is successfully grabbed to reduce the rcu holding > time. > > [1] https://lore.kernel.org/lkml/87sjhzun47.fsf@xmission.com/ > " It doesn't make sense to me. I don't see any sleeping functions called from find_mm_struct or kernel_migrate_pages in the area kernel_migrate_pages in the area of the code protected by get_task_struct. So at a very basic level I see a justification for dirtying a cache line twice with get_task_struct and put_task_struct to reduce rcu_read_lock hold times. I would contend that a reasonable cleanup based up on the current state of the code would be to extend the rcu_read_lock over get_task_mm so that a reference to task_struct does not need to be taken. That has the potential to reduce contention and reduce lock hold times. The code is missing a big fat comment with the assertion that it is ok if the permission checks are racy because the race is small, and the worst case thing that happens is the page is migrated to another numa node. Given that the get_mm_task takes task_lock the cost of dirtying the cache line is already being paid. Perhaps not extending task_lock hold times a little bit is justified, but I haven't seen that case made. This seems like code that is called little enough it would be better for it to be correct, and not need big fat comments explaining why it doesn't matter that they code is deliberately buggy. In short it does not make sense to me to justify a patch for performance reasons when it appears that extending the rcu_read_lock hold time and not touch the task reference count would stop dirtying a cache line and likely have more impact. Eric