[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAGsJ_4y5QB7UTD3Mvwqib-c6DYkKCP_9V1s9eVoXLZGx+A5ObA@mail.gmail.com>
Date: Thu, 1 May 2025 11:23:27 +1200
From: Barry Song <21cnbao@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: Feng Lee <379943137@...com>, akpm@...ux-foundation.org, ryan.roberts@....com,
libang.li@...group.com, peterx@...hat.com, maobibo@...ngson.cn,
lance.yang@...ux.dev, anshuman.khandual@....com, trivial@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: remove useless code
On Tue, Apr 29, 2025 at 1:59 AM David Hildenbrand <david@...hat.com> wrote:
>
> On 28.04.25 13:03, Barry Song wrote:
> > On Mon, Apr 28, 2025 at 7:17 PM David Hildenbrand <david@...hat.com> wrote:
> >>
> >> On 27.04.25 10:22, Barry Song wrote:
> >>> On Sun, Apr 27, 2025 at 2:16 PM Feng Lee <379943137@...com> wrote:
> >>>>
> >>>> Remove unused conditional macros.
> >>>>
> >>>> Signed-off-by: Feng Lee <379943137@...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.
I assume that reside > TASK_SIZE can only be true on 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)
Since IA64 is dead, is the code also dead? It seems we can safely move
forward with the approach you're proposing.
>
> 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
>
Thanks
Barry
Powered by blists - more mailing lists