Re: + mm-constify-highmem-related-functions-for-improved-const-correctness-fix.patch added to mm-unstable branch

[David Hildenbrand <david@redhat.com>]

On 04.09.25 11:48, Vlastimil Babka wrote:
> On 9/4/25 11:40, David Hildenbrand wrote:
>> On 04.09.25 11:28, Vlastimil Babka wrote:
>>> On 9/4/25 11:16, David Hildenbrand wrote:
>>>> On 04.09.25 09:57, Vlastimil Babka wrote:
>>>>> On 9/4/25 08:37, David Hildenbrand wrote:
>>>>>> On 04.09.25 04:59, Andrew Morton wrote:
>>> I thought the benefit of the generic is if you pass const folio you get
>>> const page, and if you pass non-const you get non-stop, and it doesn't
>>> involve any cast of something already assumed const to non-const which I
>>> assume is bad (TM). But I don't know how much bad in practice (can it lead
>>> to optimizations screwing with your code?).
>>>
>>> With the inline function in place there's such a cast const -> non-const on
>>> the page pointer it returns, but maybe it's fine because it only happens
>>> when the original folio was non-const? I don't know. It just doesn't seem to
>>> add benefits when only used via the generic?
>>
>> Well, take a look at page_folio(), I really just mimic what they do.
> 
> Yeah, but that one is different due to _compound_head() returning unsigned long.
> 
>> I think the point is that with generics you can control that where you
>> cast the const of is sane.
>>
>> In Andrew's patch you just cast the const away unconditionally, which is
>> rather nasty.
> 
> Yup I'm in favor of your approach.
> 
>> With generic you could write two functions _folio_page_non_const() +
>> _folio_page_const(), but that's just stupid.
>>
>> A plain
>>
>> 	#define folio_page(f, n) (&(f)->page + (n))	
>>
>> Would also make the compiler respect the const'ness of f, because
>> "->page" is not a pointer.
> 
> Hm that's true. The generic then only adds some type restrictions.
> 
>> Now, of course that would change once ->page becomes a pointer (memdescs
>> ...)
> 
> Right. I'm not opposed to your patch, just thought it could be smaller, but
> not insisting. Do you want anyone to use _folio_page() directly?

I think just like _compound_head it's rather implied that this is only 
for people that know what they are doing, but sure we could add a comment.

> If not
> maybe add a comment that it's internal and only folio_page() should be used,
> to help avoid accidental misuse?

Just to clarify: I'm not married to any of that, but I enjoyed having an 
inline function. So I'm mostly wondering "how could we do better than 
just going back to a plain macro".

I'll note that _compound_head() is rather special as in it essentially 
consumes a "const pointer" but then just looks up another pointer where 
we lose the const-ness.

-- 
Cheers

David / dhildenb