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;
Message-ID: <78ae828c-bdc3-7318-5a3e-fe95ad7ef5cf@intel.com>
Date:   Thu, 13 Jul 2023 09:55:00 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.0
Subject: Re: [RFC PATCH v2 2/3] mm: handle large folio when large folio in
 VM_LOCKED VMA range
Content-Language: en-US
To:     Yu Zhao <yuzhao@google.com>
CC:     <hughd@google.com>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>,
        <willy@infradead.org>, <david@redhat.com>, <ryan.roberts@arm.com>,
        <shy828301@gmail.com>
References: <20230712060144.3006358-1-fengwei.yin@intel.com>
 <20230712060144.3006358-3-fengwei.yin@intel.com>
 <CAOUHufZQ0EpyRZ_jAMxs8uNSQOz6uAkzVjvvgD+4wBvmgHJoCA@mail.gmail.com>
 <6cc5a915-a28c-983f-9b32-6040f033970b@intel.com>
 <CAOUHufbK87NBia8-3OnKwma3JVxe5mJ593yCy_a3HWC1tYH_EA@mail.gmail.com>
From:   Yin Fengwei <fengwei.yin@intel.com>
In-Reply-To: <CAOUHufbK87NBia8-3OnKwma3JVxe5mJ593yCy_a3HWC1tYH_EA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?b2V5VXZFRjEweHhJc3V3bkZEK2RmMFFGRE9CS2FGWTIzN1g3OGNPZ0tmSzVx?=
 =?utf-8?B?Q1VjZDdIdG1IZ2dXOU5QV3gyNmY0QTZBb1dHOU93OUhGSFMvbGdFNklQQzdX?=
 =?utf-8?B?a3JHSEozWDlEY1J6VTI1b0JlZ0owRi9xVjNWOGFGYjUwemFUejNudUhKVnhF?=
 =?utf-8?B?Ykd3NkhTd3B1NmViUCtocnNjVXRpMVF1Q0hkMlcvbGFoaEpQeXN6YVAySXdl?=
 =?utf-8?B?cFlqSGFpdGJlNElzRUdQSGUxRHhtYXFsdjIrM24va2p2N0s5dGI3cDRHODA0?=
 =?utf-8?B?bmVMdzdxVVVZWkhSTFdIclhzZXFObDAxK1RFUElsSnc1N0ZtNzIvcVRzWktr?=
 =?utf-8?B?K2pTL1hGUDdDbjVkZmVzYWFWYUFqUFM3U2VXbXVMZzM1bkxreDRWejhJVzZl?=
 =?utf-8?B?bENUN2oyS2REQVl2VVljNFFBQUI1bUpXeVR0M285dzFLT2lmN0s2NlRiQVhI?=
 =?utf-8?B?d3g4eVAwUUxDUkVNVEVPb3Y3K3k2K3podUk3bXZyamU5QlVmdy85amVFMTFj?=
 =?utf-8?B?aU1uNjhuc2JjaHpIMVhwdFdBN09vVStvYmdvWU1EWHZxeUtLam51eDdVWGV5?=
 =?utf-8?B?eUgrbklseWRodnF6UkViUzhGeDhRN2JaM0lJSG9ZcG9Pa0NMTGZwQlh4ckdS?=
 =?utf-8?B?b3FVbWZXQ1ZxNE9WTWJzTDVrY2tSWCs4TzdvMzNwTm9CYlZIVFB4dWU1SHNN?=
 =?utf-8?B?M01ZZWxrdmFyUVpzZUNwSWZxVjFXM3JqWnFLVVdjcmoxenBQRFJaTG1HSHRa?=
 =?utf-8?B?T0hTVU1DNVI1MS9QMXVETEV4Rk8vdWp0QlJSVTB2OEtvK2xQbjVhbjVCVDJx?=
 =?utf-8?B?MlRBY2RXVnh2YzF4eUdPWnR6RkREaG1HY1NZbVF4bjMyZVVwMzJ1NGZOM1dy?=
 =?utf-8?B?T3dYR0RRT04yd2ZwRy9NSjJZRURhUDRKVHA3TWZaYlFHOHJyOWIvaFYxa0RH?=
 =?utf-8?B?YzdoYWE1d0dFdFFES0JheDFublptcWZpeWQ0MW1wMkhKWG9JUXZ2VzFNMWF4?=
 =?utf-8?B?SytuQzlZRm9uTFlFQTdnSlE1TzNiSW1pQVJ3QmFMQkg4UVhnbDNydzIraWxj?=
 =?utf-8?B?NlplRUMwN2lOeUNYMXpqMzNDcWZTUXFNY2g0aDVnSXByaDdHT204Qnp0eExj?=
 =?utf-8?B?TTNrVWtGTlMvS3F5NVdTbUd4ZnZuTy8zelRHa1h0Wnl0bUFENHptOENaeXdv?=
 =?utf-8?B?VVFVTXB5K3FGZnJxQlVWclFxYUFtblJwNUpHdnlqWFdFY1kxclJVeHBMM0I5?=
 =?utf-8?B?bGttZThNVnZSOTFFM05iOHFMNlBpOWt3Q05tTzBQREk3ajBoMUU1dTRKWEk2?=
 =?utf-8?B?b2ZCR2R6cG00eUFoSUIrelJvZytNUGY5bU1sUnZVU2JzL3JwdnBHR0l6ZmVu?=
 =?utf-8?B?bVhQbnBuUFliWENWQlhzbWZNc1hUNkFCakdsWm1wZTYvemQyaWJGTnpWRCtE?=
 =?utf-8?B?NjFSNlJMYzU5eUxNaE5YQzh1dm1WelBpLzY0anNMRlp0UENpZFMwYk5BNjVZ?=
 =?utf-8?B?clVyb3U4cldRR21kS09GS3ZkTWNzcWM5QTIzR1hOSDIyTm1IUjZJQzc0bHk5?=
 =?utf-8?B?MTNrUys3ZDFPSjhCS0gwM0VXaHRBNStEQ0V5WEhwdkt5bU5GYmRBbjd4YXhB?=
 =?utf-8?B?d051N01PYnhKUWFBajY2VkFZMjNzSVFOVnpaeTdhTVRTdWVQQzB6emlTQ2JR?=
 =?utf-8?B?SGtXc2U0dzZBMUtwOXp2S0IxelpTU21kWTJnWnVkNllKcjl1d3c5bEVSZDhU?=
 =?utf-8?B?bS81dDlpUmJXK1lJZG92OUZIRWhob0xFaktxUTNJY3Jac0RxY1lSQnRPUVgz?=
 =?utf-8?B?NTcvODgwZlVTRnM3c0VBb09LalNaNnREZldoR29HZ25uWGpEeURveUk3MXl0?=
 =?utf-8?B?TXZ5dElhLy9SNTJ4Z1BiYys4eG4wdU02WWR3M3h1cUplSnNvWFdBQWZ4alpF?=
 =?utf-8?B?Vjhmc0hGUmxHcWxVVGdCYlJnVGJpU1hJVnNNMlUyWENHQ3BiYmpyazZlYmx4?=
 =?utf-8?B?dXFBcjdvcE0rU1lLNG8zdW1PMk1vZll0NFV0TFFiZVpaa1NiN0hJVEJkMlBQ?=
 =?utf-8?B?a3lPNGtLN0dLWndrWXh5RW1LcHdML3ltcU4rMHZVRjBzQytieGFSSmFzOThG?=
 =?utf-8?Q?uWqz73/J0MyPR4SydvflLcWNZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 579bf9e5-81ee-411b-8be4-08db83442bbd
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jul 2023 01:55:01.9448
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Gz9IexaA5L16mPkvSxZ6IBK2+uiD5TKbhs9BVfgRmjICdjL65Sn2SrLA3BAozcEH62dqJKGxve5/hYZk5Y9vAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6357
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham 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


On 7/13/23 01:03, Yu Zhao wrote:
> On Wed, Jul 12, 2023 at 12:44 AM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>
>>
>> On 7/12/23 14:23, Yu Zhao wrote:
>>> On Wed, Jul 12, 2023 at 12:02 AM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>
>>>> If large folio is in the range of VM_LOCKED VMA, it should be
>>>> mlocked to avoid being picked by page reclaim. Which may split
>>>> the large folio and then mlock each pages again.
>>>>
>>>> Mlock this kind of large folio to prevent them being picked by
>>>> page reclaim.
>>>>
>>>> For the large folio which cross the boundary of VM_LOCKED VMA,
>>>> we'd better not to mlock it. So if the system is under memory
>>>> pressure, this kind of large folio will be split and the pages
>>>> ouf of VM_LOCKED VMA can be reclaimed.
>>>>
>>>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>>>> ---
>>>>  mm/internal.h | 11 ++++++++---
>>>>  mm/rmap.c     | 34 +++++++++++++++++++++++++++-------
>>>>  2 files changed, 35 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/mm/internal.h b/mm/internal.h
>>>> index c7dd15d8de3ef..776141de2797a 100644
>>>> --- a/mm/internal.h
>>>> +++ b/mm/internal.h
>>>> @@ -643,7 +643,8 @@ static inline void mlock_vma_folio(struct folio *folio,
>>>>          *    still be set while VM_SPECIAL bits are added: so ignore it then.
>>>>          */
>>>>         if (unlikely((vma->vm_flags & (VM_LOCKED|VM_SPECIAL)) == VM_LOCKED) &&
>>>> -           (compound || !folio_test_large(folio)))
>>>> +           (compound || !folio_test_large(folio) ||
>>>> +           folio_in_range(folio, vma, vma->vm_start, vma->vm_end)))
>>>>                 mlock_folio(folio);
>>>>  }
>>>
>>> This can be simplified:
>>> 1. remove the compound parameter
>> Yes. There is not difference here for pmd mapping of THPs and pte mappings of THPs
>> if the only condition need check is whether the folio is within VMA range or not.
>>
>> But let me add Huge for confirmation.
>>
>>
>>> 2. make the if
>>>         if (unlikely((vma->vm_flags & (VM_LOCKED|VM_SPECIAL)) == VM_LOCKED) &&
>>>             folio_within_vma())
>>>                 mlock_folio(folio);
>> !folio_test_large(folio) was kept here by purpose. For normal 4K page, don't need
>> to call folio_within_vma() which is heavy for normal 4K page.
> 
> I suspected you would think so -- I don't think it would make any
> measurable (for systems with mostly large folios, it would actually be
> an extra work). Since we have many places like this once, probably we
> could wrap folio_test_large() into folio_within_vma() and call it
> large_folio_within_vma(), if you feel it's necessary.
I thought about moving folio_test_large() to folio_in_range(). But gave
it up because of checking folio addr in vma range.

But with new folio_within_vma(), we could do that. Will move folio_test_large()
to folio_within_vma() (I will keep current naming) and make it like:

     return !folio_test_large() || folio_in_range();

> 
>>>> @@ -651,8 +652,12 @@ void munlock_folio(struct folio *folio);
>>>>  static inline void munlock_vma_folio(struct folio *folio,
>>>>                         struct vm_area_struct *vma, bool compound)
>>>
>>> Remove the compound parameter here too.
>>>
>>>>  {
>>>> -       if (unlikely(vma->vm_flags & VM_LOCKED) &&
>>>> -           (compound || !folio_test_large(folio)))
>>>> +       /*
>>>> +        * To handle the case that a mlocked large folio is unmapped from VMA
>>>> +        * piece by piece, allow munlock the large folio which is partially
>>>> +        * mapped to VMA.
>>>> +        */
>>>> +       if (unlikely(vma->vm_flags & VM_LOCKED))
>>>>                 munlock_folio(folio);
>>>>  }
>>>>
>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>> index 2668f5ea35342..455f415d8d9ca 100644
>>>> --- a/mm/rmap.c
>>>> +++ b/mm/rmap.c
>>>> @@ -803,6 +803,14 @@ struct folio_referenced_arg {
>>>>         unsigned long vm_flags;
>>>>         struct mem_cgroup *memcg;
>>>>  };
>>>> +
>>>> +static inline bool should_restore_mlock(struct folio *folio,
>>>> +               struct vm_area_struct *vma, bool pmd_mapped)
>>>> +{
>>>> +       return !folio_test_large(folio) ||
>>>> +                       pmd_mapped || folio_within_vma(folio, vma);
>>>> +}
>>>
>>> This is just folio_within_vma() :)
>>>
>>>>  /*
>>>>   * arg: folio_referenced_arg will be passed
>>>>   */
>>>> @@ -816,13 +824,25 @@ static bool folio_referenced_one(struct folio *folio,
>>>>         while (page_vma_mapped_walk(&pvmw)) {
>>>>                 address = pvmw.address;
>>>>
>>>> -               if ((vma->vm_flags & VM_LOCKED) &&
>>>> -                   (!folio_test_large(folio) || !pvmw.pte)) {
>>>> -                       /* Restore the mlock which got missed */
>>>> -                       mlock_vma_folio(folio, vma, !pvmw.pte);
>>>> -                       page_vma_mapped_walk_done(&pvmw);
>>>> -                       pra->vm_flags |= VM_LOCKED;
>>>> -                       return false; /* To break the loop */
>>>> +               if (vma->vm_flags & VM_LOCKED) {
>>>> +                       if (should_restore_mlock(folio, vma, !pvmw.pte)) {
>>>> +                               /* Restore the mlock which got missed */
>>>> +                               mlock_vma_folio(folio, vma, !pvmw.pte);
>>>> +                               page_vma_mapped_walk_done(&pvmw);
>>>> +                               pra->vm_flags |= VM_LOCKED;
>>>> +                               return false; /* To break the loop */
>>>> +                       } else {
>>>
>>> There is no need for "else", or just
>>>
>>>   if (!folio_within_vma())
>>>     goto dec_pra_mapcount;
>> I tried not to use goto as much as possible. I suppose you mean:
>>
>>     if (!should_restore_lock())
>>         goto dec_pra_mapcount; (I may use continue here. :)).
> 
> should_restore_lock() is just folio_within_vma() -- see the comment
> above. "continue" looks good to me too (prefer not to add more indents
> to the functions below).
Yes.

> 
>>     mlock_vma_folio();
>>     page_vma_mapped_walk_done()
>>    ...
>>
>> Right?
> 
> Right.
This is very good suggestion. Will update v3 accordingly after wait
for a while in case other comments. Thanks.


Regards
Yin, Fengwei