Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20221123005752.161003-1-gshan@redhat.com>
 <e8a64b11-98f9-b571-dce9-a60df98e3e5@google.com>
 <c61612f7-b861-39cf-3e73-dbe4d134eec0@redhat.com>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Alistair Popple <apopple@nvidia.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     Hugh Dickins <hughd@google.com>, Gavin Shan <gshan@redhat.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, william.kucharski@oracle.com,
        ziy@nvidia.com, kirill.shutemov@linux.intel.com,
        zhenyzha@redhat.com, shan.gavin@gmail.com, riel@surriel.com,
        willy@infradead.org
Subject: Re: [PATCH] mm: migrate: Fix THP's mapcount on isolation
Date:   Thu, 24 Nov 2022 12:06:56 +1100
In-reply-to: <c61612f7-b861-39cf-3e73-dbe4d134eec0@redhat.com>
Message-ID: <871qptrvsw.fsf@nvidia.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?jV2dL3DFoXVYeSdgYJB16VpkWUXOkTBTiXKKnpsScomE9Yc76UH3/e7j8k8g?=
 =?us-ascii?Q?iAFgiMEC05+WDLF1iw3hF+MHsoG2jD7pD66A0pFbIKFoxHTWqm14mZ7NBSWX?=
 =?us-ascii?Q?7EyL96EWECcrVf0qbFUIgMxBTMRiz6EZsQTn5lF8+/CcZbtPibWuGPPGK6fd?=
 =?us-ascii?Q?huySxdY7pIcXyzdabdmm7HWOfk+yGvbkLnD6nyKp8iIPTbsxpbWR5khou5Ry?=
 =?us-ascii?Q?2sDWN0eyd3Rkr2Kgqu5Ktf25IHJdstLT50kznPhNb3hT3vdfqQ8PMOApCb2F?=
 =?us-ascii?Q?eTp9wILJtIAJHp6ST3WNVNAh+B58tKaMEsN48sTJ/Zmz5MgXLzOdckF6RTQ3?=
 =?us-ascii?Q?k2KAqjcu3rTWdbgWFVqrTcm3JBoQ93moe/nX1caANo+o1xs9ZhZUFlMJMMHN?=
 =?us-ascii?Q?ISoe0zjbEVUVex3cANhrCr5LH03jbGDxz1F1W64UdbjS3uZ1CUDLQz6eDBQI?=
 =?us-ascii?Q?VGXN/4t9ZQ0msPHOnQAxnsaEiZmVh6OJ8kkPxJ3yo4lU3PuP4c9Shovyb/c+?=
 =?us-ascii?Q?kM9VBvdagMnULD6LNxIGX/AVsll74vjEO47bSVBjZJXvWuJxE+I1FUzwhs2M?=
 =?us-ascii?Q?+97I7MvDAW9UsCsLMz5ktxsUeBlHSjmwh01NbHRLtbj//mD0t5JJoYkcDn58?=
 =?us-ascii?Q?lDLwgYg6h1sYTHfCt6YVw7kJLErf10ASUqD9bqDmqhm2I2xkRCt3iTRWhv7C?=
 =?us-ascii?Q?QAF8dUkAh0XN52tL68HQLEkX0zc51U7brnhTIUtnbBGVM4+rd+p1+PG/eLk5?=
 =?us-ascii?Q?i4iM3trG9UOEu0s5awWXCViDhAwTML6NFRE27tgsqYOnB8g1iKAP/xt3OA3z?=
 =?us-ascii?Q?/msl3vGFxfgty71iOdEhfXtmrPxQHwiSezb7cH8muu96fBXPiOx0Ut/KyyMv?=
 =?us-ascii?Q?Rp9TNYMGQwWbv6F+fq4RPwN3nNMDbQyT30LWZvjMDs9TfXAR/ERZJMhxGHk4?=
 =?us-ascii?Q?oFry2mYtDxe+pnTkYzkv8XGsBhzWjOErcate+JC+ZBdIV7uRC+N5VLDueA78?=
 =?us-ascii?Q?dHOsZXKUU9j5++lj+mR5Meso9kkpsyBWJvoQTu8r6U07fmn045u7+T9kgIgP?=
 =?us-ascii?Q?0lg2XvQtpSsjaG/DAWxPnL5wO64b/qxyOsqG4IwYjK1FwRK0hzzMWuPwQ1g/?=
 =?us-ascii?Q?YlWGYiPjz/xMjBC74HjI3H7NPLOJRBMa39eaA00+RLxv50H6K/tC5TQnN4fM?=
 =?us-ascii?Q?UAm9iM81UUrt/hJTI7VGAlsVVvbFnfmnZ/lyXdxj9vDpR+mK5XHBc9MKlrdU?=
 =?us-ascii?Q?LI8NMogGLUE1Cj0C856hHReXrw2nGOL5H24MKbgv0DJnNhainxTILPieLvux?=
 =?us-ascii?Q?j4wKwiPZPwhCDNlAqtFnhoUTSKiHz8ezvNnAGYghz1P0HG31MKoBslnZbwBM?=
 =?us-ascii?Q?nsdpMlQHzZ59OKQU86umEOzbG0IzvAUPnXUNs8cTvSAsVJVKRroMoi3wR5Zz?=
 =?us-ascii?Q?dFmbE3FMCSh6RYOqI/4YFYzwxPG3+QYQIHaqvbDY24JmjifDqy0Zw5aH0eLa?=
 =?us-ascii?Q?4kFHOTVoY0wuhehsCqhA9dE01rcILgHe8ajVU9OWJv0Ty+9MZs9BaaMfQW9a?=
 =?us-ascii?Q?EtsinJL92qPVbI5SqUIJJSAwblrp7ZY9C68zYx0t?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f7400370-1065-4c1b-76c3-08dacdba4f5d
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2022 01:22:10.6576
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mZV/B5m9ghsvH5mn7HSrvyeOZK3dY0SiIe2xFGQFVNQc2o/8v4KzGfk94jTay0+a0/tmbfhHmuwtEuZT4OrsEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR12MB6285
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


David Hildenbrand <david@redhat.com> writes:

> On 23.11.22 06:14, Hugh Dickins wrote:
>> On Wed, 23 Nov 2022, Gavin Shan wrote:
>> 
>>> The issue is reported when removing memory through virtio_mem device.
>>> The transparent huge page, experienced copy-on-write fault, is wrongly
>>> regarded as pinned. The transparent huge page is escaped from being
>>> isolated in isolate_migratepages_block(). The transparent huge page
>>> can't be migrated and the corresponding memory block can't be put
>>> into offline state.
>>>
>>> Fix it by replacing page_mapcount() with total_mapcount(). With this,
>>> the transparent huge page can be isolated and migrated, and the memory
>>> block can be put into offline state.
>>>
>>> Fixes: 3917c80280c9 ("thp: change CoW semantics for anon-THP")
>>> Cc: stable@vger.kernel.org   # v5.8+
>>> Reported-by: Zhenyu Zhang <zhenyzha@redhat.com>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> Interesting, good catch, looked right to me: except for the Fixes
>> line
>> and mention of v5.8.  That CoW change may have added a case which easily
>> demonstrates the problem, but it would have been the wrong test on a THP
>> for long before then - but only in v5.7 were compound pages allowed
>> through at all to reach that test, so I think it should be
>> Fixes: 1da2f328fa64 ("mm,thp,compaction,cma: allow THP migration for
>> CMA allocations")
>> Cc: stable@vger.kernel.org   # v5.7+
>> Oh, no, stop: this is not so easy, even in the latest tree.
>> Because at the time of that "admittedly racy check", we have no hold
>> at all on the page in question: and if it's PageLRU or PageCompound
>> at one instant, it may be different the next instant.  Which leaves it
>> vulnerable to whatever BUG_ON()s there may be in the total_mapcount()
>> path - needs research.  *Perhaps* there are no more BUG_ON()s in the
>> total_mapcount() path than in the existing page_mapcount() path.
>> I suspect that for this to be safe (before your patch and more so
>> after),
>> it will be necessary to shift the "admittedly racy check" down after the
>> get_page_unless_zero() (and check the sequence of operations when a
>> compound page is initialized).
>
> Grabbing a reference first sounds like the right approach to me.

I think you're right. Without a page reference I don't think it is even
safe to look at struct page, at least not without synchronisation
against memory hot unplug which could remove the struct page. From a
quick glance I didn't see anything here that obviously did that though.

>> The races I'm talking about are much much rarer than the condition
>> you
>> are trying to avoid, so it's frustrating; but such races are real,
>> and increasing stable's exposure to them is not so good.
>
> Such checks are always racy and the code has to be able to deal with
> false negatives/postives (we're not even holding the page lock); as
> you state, we just don't want to trigger undefined behavior/BUG.
>
>
> I'm also curious how that migration code handles a THP that's in the
> swapcache. It better should handle such pages correctly, for example,
> by removing them from the swapcache first, otherwise that could block
> migration.
>
>
> For example, in mm/ksm.c:write_protect_page() we have
>
> "page_mapcount(page) + 1 + swapped != page_count(page)"
>
> page_mapcount() and "swapped==0/1" makes sense to me, because KSM only
> cares about order-0 pages, so no need for THP games.
>
>
> But we do have an even better helper in place already:
> mm/huge_memory.c:can_split_folio()
>
> Which cares about
>
> a) Swapcache for THP: each subpage could be in the swapcache
> b) Requires the caller to hold one reference to be safe
>
> But I am a bit confused about the "extra_pins" for !anon. Where do the
> folio_nr_pages() references come from?
>
> So *maybe* it makes sense to factor out can_split_folio() and call it
> something like: "folio_maybe_additionally_referenced"  [to clearly
> distinguish it from "folio_maybe_dma_pinned" that cares about actual
> page pinning (read/write page content)].
>
> Such a function could return false positives/negatives due to races
> and the caller would have to hold one reference and be able to deal
> with the semantics.