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 3607DC27C53 for ; Thu, 6 Jun 2024 01:57:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C2B996B00A5; Wed, 5 Jun 2024 21:57:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BB4156B00A6; Wed, 5 Jun 2024 21:57:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A2DBE6B00A7; Wed, 5 Jun 2024 21:57:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 8288E6B00A5 for ; Wed, 5 Jun 2024 21:57:57 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 119731A0226 for ; Thu, 6 Jun 2024 01:57:57 +0000 (UTC) X-FDA: 82198802994.16.AD88CA9 Received: from out30-133.freemail.mail.aliyun.com (out30-133.freemail.mail.aliyun.com [115.124.30.133]) by imf07.hostedemail.com (Postfix) with ESMTP id 455914000B for ; Thu, 6 Jun 2024 01:57:52 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=v3Xbs39i; spf=pass (imf07.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.133 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com; dmarc=pass (policy=none) header.from=linux.alibaba.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1717639074; a=rsa-sha256; cv=none; b=I7X+y+aYBuIlyJ7u1yE01IPuGAlW08mGNYMElvdV8+PGuYksDEmrTw+bUI8UpQSZARP5Je XbjzPS9eGYc/ikguyOmKKbeFEE7PS7oCJdY3v/3O7YuhPCSpMp0UCDSZ6ihMXrJpxHUIS1 JQucgt9RTlNCyMVPQ+tS/C35pjS6Jkg= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=linux.alibaba.com header.s=default header.b=v3Xbs39i; spf=pass (imf07.hostedemail.com: domain of baolin.wang@linux.alibaba.com designates 115.124.30.133 as permitted sender) smtp.mailfrom=baolin.wang@linux.alibaba.com; dmarc=pass (policy=none) header.from=linux.alibaba.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1717639074; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=S92oigTI4DbA9DJ4iXLbVTH+mvSy0U0OHozBRnHwsFY=; b=Gqi8iROgn3Px6Uq/g8P034JENAEVjmf49jR5TwqSIV7ETqkiXZhLz42ctIvxaY/Zw2yNRA p74dQ4RfkZ394RkrvUCSvVfiVvOojV2vOWTMjaYWfwh+itolrVv+qRzsUGFnUlvr2nlidO Xl+PGzmoN+jZDtv9I/TEoN7t9bx/rz8= DKIM-Signature:v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.alibaba.com; s=default; t=1717639070; h=Message-ID:Date:MIME-Version:Subject:To:From:Content-Type; bh=S92oigTI4DbA9DJ4iXLbVTH+mvSy0U0OHozBRnHwsFY=; b=v3Xbs39inoUgYMzzcv0KeVxAVNT3G0b5fGVDdQAvHLSWNB2s5VIxWiQGidSdTkys4rbsnV+npmyB7EnaOr0L5TIP/rnZJTTxO3VphQwnnqGowJ00ri58XXruCHFk1jFkVOMTaS7jsVy/BrccZCSM+g+XWUP4A05Eq86uK2YA2Is= X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R111e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=maildocker-contentspam033037067111;MF=baolin.wang@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0W7wW1k-_1717639067; Received: from 30.97.56.72(mailfrom:baolin.wang@linux.alibaba.com fp:SMTPD_---0W7wW1k-_1717639067) by smtp.aliyun-inc.com; Thu, 06 Jun 2024 09:57:48 +0800 Message-ID: Date: Thu, 6 Jun 2024 09:57:47 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] mm/gup: don't check page lru flag before draining it To: David Hildenbrand , yangge1116 , akpm@linux-foundation.org, Matthew Wilcox Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, liuzixing@hygon.cn References: <1717498121-20926-1-git-send-email-yangge1116@126.com> <0d7a4405-9a2e-4bd1-ba89-a31486155233@redhat.com> <776de760-e817-43b2-bd00-8ce96f4e37a8@redhat.com> <7063920f-963a-4b3e-a3f3-c5cc227bc877@redhat.com> <48150a28-ed48-49ff-9432-9cd30cda4da4@linux.alibaba.com> <11ef3deb-d1e3-46d5-97ed-9ba3c1fbbba9@redhat.com> <697a9bc2-a655-4035-aa5e-7d3acb23e79d@redhat.com> From: Baolin Wang In-Reply-To: <697a9bc2-a655-4035-aa5e-7d3acb23e79d@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 455914000B X-Stat-Signature: k4qiap831tdtg4uxu1nb7wy1s9ddws3h X-HE-Tag: 1717639072-116890 X-HE-Meta: U2FsdGVkX18vT4Olc9G/oem85HBFQT6Y/pSe3ZjxbE8JzxHTRxKziBJz2yGUr1fJTn2LyDuyyKUxV9oJL89Rt7/L4ThFcX38qF6BK8z1bi1O7pzR0A8JV47BMmZbsMW2kNbZScXrrFqhERdmdp0r6eEVxSgW/ZVWXg552PIXwCI+q+PEvqgGv8GJf4cvo4cnieoJr8/vZGuHq3LiSd0Vy02Bjc5xxUFDaqwv5TIBr1g+LttPAxo9izdTadC2yJVqGXHPNGlm7ZjXDPX1IPbrovr8SLHzHl20bxS2gILjxQGcHbbQ2MKBtpf2n4q6q5Eg9N8C8Te97eG/iW8+DZrVZseDmK7O4jQVWS2Y1CW+9G4L1IoqSwZxgPBieG3v0NTsAADsuBRc0Y3+/onIWfXdgXvRpH7cVVcN4glHoUANJXFxYvWYQfK5Bmri0HCq++PBXeFrkBg3iYLueaYptE7R42Y7vXtygwho4Xo/6cxBIVs82IrxtvYKtOw1elsbulZ8snyhpJSRpeQH04O76YMD5l5tdGvvKyD6bD28VAI0kOanXMMGOq97t2ANs+JeSFVYgOQE12zJg+OvR8nxDiwQIOjxjBG3kOPRARFQmL8w8YOnxYC12dIMWtVmM3ZpcVyfGa+ojUZKmCuT22NFyLXItTFdFBbYwatnrQvBmEThInOZRjjSIya1GhMdWMZyu9OEndQP5hieXVeP5U3sIKs8aq1K5fF+DX90BcKxDEoxSIl8gpcUS0kH497vf0r0EzU2kjdkgvXUqjyLAqeJwcwO7T8y+/JTURR/p6VNRJ20EvNhBsbk+wRVT0hMrMPz8s/7Whl+JiJkvntQj5l10H1pbjzWmdnhGkzroD0djtrEUJFHuQz2GgeUwONAlCzGl1skKCXUC5mqV6sAiCvmP5HmirP63b34tS7UO5RuQgeNG/sy5NuoOmlliFCJKhCbZkii10XuqwlaezscYrjvCZo iydOm4jG kFhlcREndPIC9S23SpHDTAX0CeU5bvkE6mTBiehX6UYZz6zHi1VGtC1990jDPUsYpsN/HqejLHme1I5Gtz/S4lT83OZHCFoWMXTZtzSVkt63yH/0QcjMhliNAJ/dsULpXTSaUvaZRfgoswdLVQjLEC1CV/+26PcaBSNCd61uWaiz/epiviwJIDq/1hDRDpCUnAtYoUIE6qieZW0VBGBGSzwNSVnu8GS4G7CAwp2KcZD2Ya5Riw8tEjUJaVNHJjJWQw3akaSdXTZodeeHa6vqx7piCq8VoE8Gk3YxBtDkkBlZ+v4b4ZQmUrSX8WxJ0UdQv6Pfrt/9NAmYYBSx4YPIZIKPrHnZnAKCvzKlX 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: List-Subscribe: List-Unsubscribe: On 2024/6/5 20:20, David Hildenbrand wrote: > On 05.06.24 13:41, David Hildenbrand wrote: >> On 05.06.24 13:37, Baolin Wang wrote: >>> >>> >>> On 2024/6/5 17:53, David Hildenbrand wrote: >>>> On 05.06.24 11:41, David Hildenbrand wrote: >>>>> On 05.06.24 03:18, yangge1116 wrote: >>>>>> >>>>>> >>>>>> 在 2024/6/4 下午9:47, David Hildenbrand 写道: >>>>>>> On 04.06.24 12:48, yangge1116@126.com wrote: >>>>>>>> From: yangge >>>>>>>> >>>>>>>> If a page is added in pagevec, its ref count increases one, remove >>>>>>>> the page from pagevec decreases one. Page migration requires the >>>>>>>> page is not referenced by others except page mapping. Before >>>>>>>> migrating a page, we should try to drain the page from pagevec in >>>>>>>> case the page is in it, however, folio_test_lru() is not sufficient >>>>>>>> to tell whether the page is in pagevec or not, if the page is in >>>>>>>> pagevec, the migration will fail. >>>>>>>> >>>>>>>> Remove the condition and drain lru once to ensure the page is not >>>>>>>> referenced by pagevec. >>>>>>> >>>>>>> What you are saying is that we might have a page on which >>>>>>> folio_test_lru() succeeds, that was added to one of the >>>>>>> cpu_fbatches, >>>>>>> correct? >>>>>> >>>>>> Yes >>>>>> >>>>>>> >>>>>>> Can you describe under which circumstances that happens? >>>>>>> >>>>>> >>>>>> If we call folio_activate() to move a page from inactive LRU list to >>>>>> active LRU list, the page is not only in LRU list, but also in one of >>>>>> the cpu_fbatches. >>>>>> >>>>>> void folio_activate(struct folio *folio) >>>>>> { >>>>>>          if (folio_test_lru(folio) && !folio_test_active(folio) && >>>>>>              !folio_test_unevictable(folio)) { >>>>>>              struct folio_batch *fbatch; >>>>>> >>>>>>              folio_get(folio); >>>>>>              //After this, folio is in LRU list, and its ref count >>>>>> have >>>>>> increased one. >>>>>> >>>>>>              local_lock(&cpu_fbatches.lock); >>>>>>              fbatch = this_cpu_ptr(&cpu_fbatches.activate); >>>>>>              folio_batch_add_and_move(fbatch, folio, >>>>>> folio_activate_fn); >>>>>>              local_unlock(&cpu_fbatches.lock); >>>>>>          } >>>>>> } >>>>> >>>>> Interesting, the !SMP variant does the folio_test_clear_lru(). >>>>> >>>>> It would be really helpful if we could reliably identify whether LRU >>>>> batching code has a raised reference on a folio. >>>>> >>>>> We have the same scenario in >>>>> * folio_deactivate() >>>>> * folio_mark_lazyfree() >>>>> >>>>> In folio_batch_move_lru() we do the folio_test_clear_lru(folio). >>>>> >>>>> No expert on that code, I'm wondering if we could move the >>>>> folio_test_clear_lru() out, such that we can more reliably identify >>>>> whether a folio is on the LRU batch or not. >>>> >>>> I'm sure there would be something extremely broken with the following >>>> (I don't know what I'm doing ;) ), but I wonder if there would be a way >>>> to make something like that work (and perform well enough?). >>>> >>>> diff --git a/mm/swap.c b/mm/swap.c >>>> index 67786cb771305..642e471c3ec5a 100644 >>>> --- a/mm/swap.c >>>> +++ b/mm/swap.c >>>> @@ -212,10 +212,6 @@ static void folio_batch_move_lru(struct >>>> folio_batch >>>> *fbatch, move_fn_t move_fn) >>>>            for (i = 0; i < folio_batch_count(fbatch); i++) { >>>>                    struct folio *folio = fbatch->folios[i]; >>>> >>>> -               /* block memcg migration while the folio moves between >>>> lru */ >>>> -               if (move_fn != lru_add_fn && >>>> !folio_test_clear_lru(folio)) >>>> -                       continue; >>>> - >>>>                    folio_lruvec_relock_irqsave(folio, &lruvec, &flags); >>>>                    move_fn(lruvec, folio); >>>> >>>> @@ -255,8 +251,9 @@ static void lru_move_tail_fn(struct lruvec *lruvec, >>>> struct folio *folio) >>>>      */ >>>>     void folio_rotate_reclaimable(struct folio *folio) >>>>     { >>>> -       if (!folio_test_locked(folio) && !folio_test_dirty(folio) && >>>> -           !folio_test_unevictable(folio) && folio_test_lru(folio)) { >>>> +       if (folio_test_lru(folio) && !folio_test_locked(folio) && >>>> +           !folio_test_dirty(folio) && >>>> !folio_test_unevictable(folio) && >>>> +           folio_test_clear_lru(folio)) { >>>>                    struct folio_batch *fbatch; >>>>                    unsigned long flags; >>>> >>>> @@ -354,7 +351,7 @@ static void folio_activate_drain(int cpu) >>>>     void folio_activate(struct folio *folio) >>>>     { >>>>            if (folio_test_lru(folio) && !folio_test_active(folio) && >>>> -           !folio_test_unevictable(folio)) { >>>> +           !folio_test_unevictable(folio) && >>>> folio_test_clear_lru(folio)) { >>> >>> IMO, this seems violate the semantics of the LRU flag, since it's clear >>> that this folio is still in the LRU list. >> >> Good point. >> >> But regarding "violation": we already do clear the flag temporarily in >> there, so it's rather that we make the visible time where it is cleared >> "longer". (yes, it can be much longer :) ) > > Some random thoughts about some folio_test_lru() users: > > mm/khugepaged.c: skips pages if !folio_test_lru(), but would fail skip > it either way if there is the unexpected reference from the LRU batch! > > mm/compaction.c: skips pages if !folio_test_lru(), but would fail skip > it either way if there is the unexpected reference from the LRU batch! > > mm/memory.c: would love to identify this case and to a lru_add_drain() > to free up that reference. > > mm/huge_memory.c: splitting with the additional reference will fail > already. Maybe we'd want to drain the LRU batch. Agree. > > mm/madvise.c: skips pages if !folio_test_lru(). I wonder what happens if > we have the same page twice in an LRU batch with different target goals ... IIUC, LRU batch can ignore this folio since it's LRU flag is cleared by folio_isolate_lru(), then will call folios_put() to frop the reference. > Some other users (there are not that many that don't use it for sanity > checks though) might likely be a bit different. mm/page_isolation.c: fail to set pageblock migratetype to isolate if !folio_test_lru(), then alloc_contig_range_noprof() can be failed. But the original code could set pageblock migratetype to isolate, then calling drain_all_pages() in alloc_contig_range_noprof() to drop reference of the LRU batch. mm/vmscan.c: will call lru_add_drain() before calling isolate_lru_folios(), so seems no impact. BTW, we also need to look at the usage of folio_isolate_lru(). It doesn’t seem to have major obstacles, but there are many details to analyze :)