Re: [PATCH] mm: remove useless code

[David Hildenbrand <david@redhat.com>]

On 28.04.25 13:03, Barry Song wrote:
> On Mon, Apr 28, 2025 at 7:17 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 27.04.25 10:22, Barry Song wrote:
>>> On Sun, Apr 27, 2025 at 2:16 PM Feng Lee <379943137@qq.com> wrote:
>>>>
>>>> Remove unused conditional macros.
>>>>
>>>> Signed-off-by: Feng Lee <379943137@qq.com>
>>>> ---
>>>>    include/linux/pgtable.h | 2 --
>>>>    1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
>>>> index b50447ef1c92..47c5a54b7551 100644
>>>> --- a/include/linux/pgtable.h
>>>> +++ b/include/linux/pgtable.h
>>>> @@ -1164,9 +1164,7 @@ static inline void arch_swap_restore(swp_entry_t entry, struct folio *folio)
>>>>    }
>>>>    #endif
>>>>
>>>> -#ifndef __HAVE_ARCH_PGD_OFFSET_GATE
>>>>    #define pgd_offset_gate(mm, addr)      pgd_offset(mm, addr)
>>>> -#endif
>>>
>>> Do you know who else had pgd_offset_gate() before except ia64?
>>>
>>> /* Look up a pgd entry in the gate area.  On IA-64, the gate-area
>>>      resides in the kernel-mapped segment, hence we use pgd_offset_k()
>>>      here.  */
>>> #define pgd_offset_gate(mm, addr) pgd_offset_k(addr)
>>>
>>> btw, do we still
>>> need pgd_offset_gate() given that nobody needs it now?
>>>
>>>      1   1168  include/linux/pgtable.h <<GLOBAL>>
>>>                #define pgd_offset_gate(mm, addr) pgd_offset(mm, addr)
>>>
>>>      2   1112  mm/gup.c <<get_gate_page>>
>>>                pgd = pgd_offset_gate(mm, address);
>>>
>>
>> Right, we should just remove pgd_offset_gate() completely in this patch
>> and simply make the single caller use pgd_offset().
> 
> Yes, exactly. The original patch doesn’t seem to be appropriate.
> 
>>
>> I think we can even do:
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 84461d384ae2b..05dd87ccce155 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1106,10 +1106,7 @@ static int get_gate_page(struct mm_struct *mm,
>> unsigned long address,
>>           /* user gate pages are read-only */
>>           if (gup_flags & FOLL_WRITE)
>>                   return -EFAULT;
>> -       if (address > TASK_SIZE)
>> -               pgd = pgd_offset_k(address);
>> -       else
>> -               pgd = pgd_offset_gate(mm, address);
>> +       pgd = pgd_offset(address);
>>           if (pgd_none(*pgd))
>>                   return -EFAULT;
>>           p4d = p4d_offset(pgd, address);
>>
>> Unless I am missing something important :)
> 
> Technically, it appears to be correct. However, it seems that
> pgd_offset_k is primarily used to improve readability by
> distinguishing between kernel space and user space?

Yeah, but this is GUP ... ("user") ... looks like that check/handling 
was in there ever since git happened.

get_gate_vma() only exists on x86-64 and uml.

I wonder if that could ever actually reside > TASK_SIZE such that we 
would even need that.

But this whole gate stuff is confusing ... IIRC it's a single VMA shared 
by all processes, and not actually linked in the maple tree etc.

-- 
Cheers,

David / dhildenb