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 0FADFECAAA3 for ; Fri, 26 Aug 2022 01:14:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9474E940007; Thu, 25 Aug 2022 21:14:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8F68D6B0075; Thu, 25 Aug 2022 21:14:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7BE9B940007; Thu, 25 Aug 2022 21:14:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 6A04F6B0074 for ; Thu, 25 Aug 2022 21:14:42 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 358BB14042A for ; Fri, 26 Aug 2022 01:14:42 +0000 (UTC) X-FDA: 79839974004.28.B4FCBE5 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by imf26.hostedemail.com (Postfix) with ESMTP id 803CE14000C for ; Fri, 26 Aug 2022 01:14:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1661476480; x=1693012480; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=5ROJ2MWnCYa+EFmKpijhLOvfObO3r2DirT5yuRlt2dk=; b=OJrVT4w7k/2UagQVsa3muLWN3jXxnSV3CAyT1UKm9SEfEKTZvwBV5J/+ 8i/D4bBe/NlVkeNpqcIHf1HjN38eMfSv0VMwzBPfb9+qtsvoB7rSqJiA5 JH42skp+vLAv6/nUMkziISCk8lpu4/1075m+t/3ewBXPcQe1KBeU9z91V yIGnyzCYyBC4ZuVy+W5RJBjgSSl0jiOQi6EPN5XrtH4km1WEgniDkuLxY kWGKaPSZmqct4kGEAFlKp7xg+wtPQ6YP/BylzWETqr5b17b/86A6SbDea Mjttg/Lm7ivE9TpzcLTMKE6d/3Ew4q2qKgybx7XT/YkG9/QLH3A9331Lj Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10450"; a="380686476" X-IronPort-AV: E=Sophos;i="5.93,264,1654585200"; d="scan'208";a="380686476" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Aug 2022 18:14:38 -0700 X-IronPort-AV: E=Sophos;i="5.93,264,1654585200"; d="scan'208";a="587113278" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Aug 2022 18:14:33 -0700 From: "Huang, Ying" To: Alistair Popple Cc: Peter Xu , , , Nadav Amit , huang ying , LKML , "Sierra Guiza, Alejandro (Alex)" , Felix Kuehling , "Jason Gunthorpe" , John Hubbard , "David Hildenbrand" , Ralph Campbell , Matthew Wilcox , Karol Herbst , Lyude Paul , Ben Skeggs , "Logan Gunthorpe" , , , Subject: Re: [PATCH v3 2/3] mm/migrate_device.c: Copy pte dirty bit to page References: <3b01af093515ce2960ac39bb16ff77473150d179.1661309831.git-series.apopple@nvidia.com> <8735dkeyyg.fsf@nvdebian.thelocal> <8735dj7qwb.fsf@nvdebian.thelocal> Date: Fri, 26 Aug 2022 09:14:31 +0800 In-Reply-To: <8735dj7qwb.fsf@nvdebian.thelocal> (Alistair Popple's message of "Fri, 26 Aug 2022 11:02:58 +1000") Message-ID: <87bks73ix4.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1661476481; a=rsa-sha256; cv=none; b=1iM5lXOmQQy8QYXww3upVgTsg7XtTEff0Ca0itSYWOl4G41nQdXaRWkqm57CifhMaG0xe4 cNf/86kWJKvsNuaS+8rKSn592KUCyi0LKSOcJZhePGTt349lxV11obKnSub8ze8Kt0tLpk jctwIYv65xO0R/WIX8d4olvCAQTHgjM= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=OJrVT4w7; spf=pass (imf26.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.43 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1661476481; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KwpJdJfkcuVvOa/Nq+I2OZXULRzpma7MgvZb2UqHpXE=; b=w5UQv5t9BFNhEGJPrNkwxhnuAb74DM95weZ2g6x5wX0JkqEnEZYlMHG13UgXBquKLuA/0/ v8Ovq8B4vTw+ObpcXiFUKtQBteRKVlpW5KbFnmQpiorqJ2E7oE2XrsKpCI8K1m6RUcXYxt ewLYVnpaIXCFL/feL81fsLPtqp+HITQ= X-Stat-Signature: gchwyk896t3d69yhak8ekumkmecsbzks X-Rspamd-Queue-Id: 803CE14000C X-Rspam-User: X-Rspamd-Server: rspam06 Authentication-Results: imf26.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=OJrVT4w7; spf=pass (imf26.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.43 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com X-HE-Tag: 1661476480-355994 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: Alistair Popple writes: > Peter Xu writes: > >> On Fri, Aug 26, 2022 at 08:21:44AM +1000, Alistair Popple wrote: >>> >>> Peter Xu writes: >>> >>> > On Wed, Aug 24, 2022 at 01:03:38PM +1000, Alistair Popple wrote: >>> >> migrate_vma_setup() has a fast path in migrate_vma_collect_pmd() that >>> >> installs migration entries directly if it can lock the migrating page. >>> >> When removing a dirty pte the dirty bit is supposed to be carried over >>> >> to the underlying page to prevent it being lost. >>> >> >>> >> Currently migrate_vma_*() can only be used for private anonymous >>> >> mappings. That means loss of the dirty bit usually doesn't result in >>> >> data loss because these pages are typically not file-backed. However >>> >> pages may be backed by swap storage which can result in data loss if an >>> >> attempt is made to migrate a dirty page that doesn't yet have the >>> >> PageDirty flag set. >>> >> >>> >> In this case migration will fail due to unexpected references but the >>> >> dirty pte bit will be lost. If the page is subsequently reclaimed data >>> >> won't be written back to swap storage as it is considered uptodate, >>> >> resulting in data loss if the page is subsequently accessed. >>> >> >>> >> Prevent this by copying the dirty bit to the page when removing the pte >>> >> to match what try_to_migrate_one() does. >>> >> >>> >> Signed-off-by: Alistair Popple >>> >> Acked-by: Peter Xu >>> >> Reported-by: Huang Ying >>> >> Fixes: 8c3328f1f36a ("mm/migrate: migrate_vma() unmap page from vma while collecting pages") >>> >> Cc: stable@vger.kernel.org >>> >> >>> >> --- >>> >> >>> >> Changes for v3: >>> >> >>> >> - Defer TLB flushing >>> >> - Split a TLB flushing fix into a separate change. >>> >> >>> >> Changes for v2: >>> >> >>> >> - Fixed up Reported-by tag. >>> >> - Added Peter's Acked-by. >>> >> - Atomically read and clear the pte to prevent the dirty bit getting >>> >> set after reading it. >>> >> - Added fixes tag >>> >> --- >>> >> mm/migrate_device.c | 9 +++++++-- >>> >> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >> >>> >> diff --git a/mm/migrate_device.c b/mm/migrate_device.c >>> >> index 6a5ef9f..51d9afa 100644 >>> >> --- a/mm/migrate_device.c >>> >> +++ b/mm/migrate_device.c >>> >> @@ -7,6 +7,7 @@ >>> >> #include >>> >> #include >>> >> #include >>> >> +#include >>> >> #include >>> >> #include >>> >> #include >>> >> @@ -196,7 +197,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, >>> >> anon_exclusive = PageAnon(page) && PageAnonExclusive(page); >>> >> if (anon_exclusive) { >>> >> flush_cache_page(vma, addr, pte_pfn(*ptep)); >>> >> - ptep_clear_flush(vma, addr, ptep); >>> >> + pte = ptep_clear_flush(vma, addr, ptep); >>> >> >>> >> if (page_try_share_anon_rmap(page)) { >>> >> set_pte_at(mm, addr, ptep, pte); >>> >> @@ -206,11 +207,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp, >>> >> goto next; >>> >> } >>> >> } else { >>> >> - ptep_get_and_clear(mm, addr, ptep); >>> >> + pte = ptep_get_and_clear(mm, addr, ptep); >>> >> } >>> > >>> > I remember that in v2 both flush_cache_page() and ptep_get_and_clear() are >>> > moved above the condition check so they're called unconditionally. Could >>> > you explain the rational on why it's changed back (since I think v2 was the >>> > correct approach)? >>> >>> Mainly because I agree with your original comments, that it would be >>> better to keep the batching of TLB flushing if possible. After the >>> discussion I don't think there is any issues with HW pte dirty bits >>> here. There are already other cases where HW needs to get that right >>> anyway (eg. zap_pte_range). >> >> Yes tlb batching was kept, thanks for doing that way. Though if only apply >> patch 1 we'll have both ptep_clear_flush() and batched flush which seems to >> be redundant. >> >>> >>> > The other question is if we want to split the patch, would it be better to >>> > move the tlb changes to patch 1, and leave the dirty bit fix in patch 2? >>> >>> Isn't that already the case? Patch 1 moves the TLB flush before the PTL >>> as suggested, patch 2 atomically copies the dirty bit without changing >>> any TLB flushing. >> >> IMHO it's cleaner to have patch 1 fix batch flush, replace >> ptep_clear_flush() with ptep_get_and_clear() and update pte properly. > > Which ptep_clear_flush() are you referring to? This one? > > if (anon_exclusive) { > flush_cache_page(vma, addr, pte_pfn(*ptep)); > ptep_clear_flush(vma, addr, ptep); > > My understanding is that we need to do a flush for anon_exclusive. > >> No strong opinions on the layout, but I still think we should drop the >> redundant ptep_clear_flush() above, meanwhile add the flush_cache_page() >> properly for !exclusive case too. > > Good point, we need flush_cache_page() for !exclusive. Will add. That can be in another patch. For the patch itself, it looks good to me. Feel free to add, Reviewed-by: "Huang, Ying" Best Regards, Huang, Ying