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 874D3C4332F for ; Thu, 24 Nov 2022 00:15:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CD8C86B0071; Wed, 23 Nov 2022 19:15:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C88B26B0072; Wed, 23 Nov 2022 19:15:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B50B96B0074; Wed, 23 Nov 2022 19:15:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id A0D106B0071 for ; Wed, 23 Nov 2022 19:15:02 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 5DDD040B16 for ; Thu, 24 Nov 2022 00:15:02 +0000 (UTC) X-FDA: 80166415644.14.6737C00 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf16.hostedemail.com (Postfix) with ESMTP id 9BCC518000A for ; Thu, 24 Nov 2022 00:15:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669248901; h=from:from:reply-to: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RGBaalQAMOMB18Hyg4LVBpssuniV3B6MVURv6Y4rVAc=; b=O2zi01oilkCxc2ardRrgDG8QLAzlRyZKHxNr5aBSxD+XZtWeetxKWWoLEjH1R2FVgEGAW9 4PpGDOhiqs+Ks+9x1MrXW9W14g6LrRXnKGYvF2yjxnvYZ2gSVbb2m7bkoj9+oJcF2vBxXr vCDlTstR9L7z1C3ADvbM6S5PDKORCNo= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-629-0zcAGajzMSGKrYb48y_aoQ-1; Wed, 23 Nov 2022 19:14:56 -0500 X-MC-Unique: 0zcAGajzMSGKrYb48y_aoQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 972B585A588; Thu, 24 Nov 2022 00:14:55 +0000 (UTC) Received: from [10.64.54.95] (vpn2-54-95.bne.redhat.com [10.64.54.95]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 311402166B26; Thu, 24 Nov 2022 00:14:50 +0000 (UTC) Reply-To: Gavin Shan Subject: Re: [PATCH] mm: migrate: Fix THP's mapcount on isolation To: David Hildenbrand , Hugh Dickins Cc: 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, apopple@nvidia.com References: <20221123005752.161003-1-gshan@redhat.com> From: Gavin Shan Message-ID: <9d2ce3f4-9f5d-ff09-c28f-fc28ee6f2817@redhat.com> Date: Thu, 24 Nov 2022 08:14:48 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=O2zi01oi; spf=pass (imf16.hostedemail.com: domain of gshan@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=gshan@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669248902; a=rsa-sha256; cv=none; b=brSAbdXVXTBFqmvDLBp2SSXTbIPsHxbQ3ovlJBfEF3Bi7FvC/55mpPsQdhTAPg4VSZZCfa 2Bvv1HWKuaVJkCA77xPKYf2MzAtSA29J7vpgrm0fQ8XlRhR+b3eSiKhd4LZ4KY+EhToFE3 IF9XrfUOqZODYCXSHqxKG+dPAJTmEy0= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669248902; h=from:from:sender:reply-to: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=RGBaalQAMOMB18Hyg4LVBpssuniV3B6MVURv6Y4rVAc=; b=PdDDRQX11ggD6V/g5ejGQ0y95kZhc6yfrgtoQbgkrCL8Zeq7bVXEeCLqGSepY8aACLgKDG b3fHtk2/p0wKWACf7yUz7tsc+Ws0S0Xo0eTfe74bildtIMAl7twGUPYJNDsnaO2dBqptJ0 4VbUcsT3wIHZWLoAfRWWEejs9Nv65IE= X-Stat-Signature: y6sgx6ay4nbnzz61kzs7cw69rtd19jmo X-Rspamd-Server: rspam08 X-Rspam-User: Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=O2zi01oi; spf=pass (imf16.hostedemail.com: domain of gshan@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=gshan@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspamd-Queue-Id: 9BCC518000A X-HE-Tag: 1669248901-699185 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: On 11/23/22 4:56 PM, David Hildenbrand wrote: > 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 >>> Suggested-by: David Hildenbrand >>> Signed-off-by: Gavin Shan >> >> 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+ >> Right, commit 1da2f328fa64 looks more accurate in this particular case, I will fix it up in next revision. >> 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. > Yeah, it sounds reasonable to me to grab a page->__refcount in the first place. Looking at isolate_migratepages_block(), the page's refcount is increased by get_page_unless_zero(), but it's too late. To increase the page's refcount at the first place in the function will be conflicting with hugetlb page and non-LRU page. I mean there will be a series to refactor the code so that the page's refcount can be grabbed in the first place. So I plan to post a followup series to refactor the code and grab the page's refcount in the first place. In this way, the fix can be merged as soon as possible. David and Hugh, please let me know if it's reasonable plan? :) static int isolate_migratepages_block() { for (; low_pfn < end_pfn; low_pfn++) { : page = pfn_to_page(low_pfn); if (unlikely(!get_page_unless_zero(page))) // grab page's refcount in the first place goto isolate_fail_put; : if (PageHuge(page) && cc->alloc_contig) { ret = isolate_or_dissolve_huge_page(page, &cc->migratepages); // refcount is increased by this function : } : if (!PageLRU(page)) { if (unlikely(__PageMovable(page)) && !PageIsolated(page)) { if (!isolate_movable_page(page, mode))) // refcunt is increased here too goto isolate_success; } } : mapping = page_mapping(page); if (!mapping && page_count(page) > total_mapcount(page)) goto isolate_fail; : if (unlikely(!get_page_unless_zero(page))) // too late to grab the refcount? goto isolate_fail; : } } [...] Thanks, Gavin