Message-ID: <2ca5cbb3-9795-49ef-ae53-11c3143edee1@arm.com>
Date: Mon, 30 Jun 2025 11:57:11 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: David Hildenbrand <david@...hat.com>, Dev Jain <dev.jain@....com>,
 linux-kernel@...r.kernel.org
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
 Mike Rapoport <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>,
 Michal Hocko <mhocko@...e.com>, Zi Yan <ziy@...dia.com>,
 Matthew Brost <matthew.brost@...el.com>,
 Joshua Hahn <joshua.hahnjy@...il.com>, Rakie Kim <rakie.kim@...com>,
 Byungchul Park <byungchul@...com>, Gregory Price <gourry@...rry.net>,
 Ying Huang <ying.huang@...ux.alibaba.com>,
 Alistair Popple <apopple@...dia.com>, Pedro Falcato <pfalcato@...e.de>,
 Rik van Riel <riel@...riel.com>, Harry Yoo <harry.yoo@...cle.com>
Subject: Re: [PATCH v1 1/4] mm: convert FPB_IGNORE_* into FPB_HONOR_*

On 30/06/2025 10:24, David Hildenbrand wrote:
> On 30.06.25 11:18, Ryan Roberts wrote:
>> On 30/06/2025 10:08, David Hildenbrand wrote:
>>> On 30.06.25 11:04, Ryan Roberts wrote:
>>>> On 30/06/2025 04:34, Dev Jain wrote:
>>>>>
>>>>> On 29/06/25 2:30 am, David Hildenbrand wrote:
>>>>>> On 28.06.25 05:37, Dev Jain wrote:
>>>>>>>
>>>>>>> On 27/06/25 5:25 pm, David Hildenbrand wrote:
>>>>>>>> Honoring these PTE bits is the exception, so let's invert the meaning.
>>>>>>>>
>>>>>>>> With this change, most callers don't have to pass any flags.
>>>>>>>>
>>>>>>>> No functional change intended.
>>>>>>>
>>>>>>> FWIW I had proposed this kind of change earlier to Ryan (CCed) and
>>>>>>> he pointed out: "Doesn't that argument apply in reverse if you want
>>>>>>> to ignore something new in future?
>>>>>>>
>>>>>>> By default we are comparing all the bits in the pte when determining the
>>>>>>> batch.
>>>>>>> The flags request to ignore certain bits.
>>>>>>
>>>>>> That statement is not true: as default we ignore the write and young bit. And
>>>>>> we don't have flags for that ;)
>>>>>>
>>>>>> Now we also ignore the dirty and soft-dity bit as default, unless told not to
>>>>>> do that by one very specific caller.
>>>>>>
>>>>>>> If we want to ignore extra bits in
>>>>>>> future, we add new flags and the existing callers don't need to be updated.
>>>>>>
>>>>>> What stops you from using FPB_IGNORE_* for something else in the future?
>>>>>>
>>>>>> As a side note, there are not that many relevant PTE bits to worry about in
>>>>>> the near future ;)
>>>>>>
>>>>>> I mean, uffd-wp, sure, ... and before we add a FPB_HONOR_UFFD_WP to all users
>>>>>> to be safe (and changing the default to ignore), you could add a
>>>>>> FPB_IGNORE_UFFD_WP first, to then check who really can tolerate just ignoring
>>>>>> it (most of them, I assume).
>>>>> I agree.
>>>>
>>>> Meh. Personally I think if you start mixing HONOR and IGNORE flags, it becomes
>>>> very confusing to work out what is being checked for and what is not. I
>>>> stand by
>>>> my original view. But yeah, writable and young confuse it a bit... How about
>>>> generalizing by explicitly requiring IGNORE flags for write and young, then
>>>> also
>>>> create a flags macro for the common case?
>>>>
>>>> #define FPB_IGNORE_COMMON (FPB_IGNORE_WRITE | FPB_IGNORE_YOUNG |    \
>>>>                 FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY)
>>>>
>>>> It's not a hill I'm going to die on though...
>>>
>>> How about we make this function simpler, not more complicated? ;)
>>
>> I think here we both have different views of what is simpler... You are trying
>> to optimize for the callers writing less code. I'm trying to optimize for the
>> reader to be able to easily determine what the function will do for a given
>> caller.
> 
> See patch number #3: I want the default function -- folio_pte_batch() -- to not
> have any flags at all.
> 
> And I don't want to achieve that by internally using flags when calling
> folio_pte_batch_ext().
> 
> If you don't specify flags (folio_pte_batch()), behave just as if calling
> folio_pte_batch_ext() without flags. Anything else would be more confusing IMHO.

OK, sorry I don't have the context of your actual series... I was just trying to
defend what my quote that Dev sent. I'll go take a look at your series.

> 
> I agree that mixing HONOR and IGNORE is not a good idea. But then, it's really
> only uffd-wp that still could be batched, and likely we want it to be the
> default, and respect/honor/whatever instead in the cases where we really have to.
> 
> (If we really want to go down that path and batch it :) )

FWIW, I think we will need to honor the write bit for Dev's mprotect series (I
just sent review comments against his v4). So if you want most callers to just
call folio_pte_batch() and that will ignore the write bit, then I guess
folio_pte_batch_ext() will have to accept a FPB_HONOR_WRITE bit? Which I guess
aligns with what you are doing here to make all the flags HONOR.

Powered by blists - more mailing lists