Re: [PATCH v2 03/13] mm: Add generic p?d_large() macros

[Mike Rapoport]

On Mon, Mar 04, 2019 at 02:35:56PM +0000, Steven Price wrote:
> On 03/03/2019 07:12, Mike Rapoport wrote:
> > On Fri, Mar 01, 2019 at 01:39:30PM +0000, Steven Price wrote:
> >> On 01/03/2019 12:30, Kirill A. Shutemov wrote:
> >>> On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote:
> >>>> Him Kirill,
> >>>>
> >>>> On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote:
> >>>>> On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote:
> >>>>>>>> Note that in terms of the new page walking code, these new defines are
> >>>>>>>> only used when walking a page table without a VMA (which isn't currently
> >>>>>>>> done), so architectures which don't use p?d_large currently will work
> >>>>>>>> fine with the generic versions. They only need to provide meaningful
> >>>>>>>> definitions when switching to use the walk-without-a-VMA functionality.
> >>>>>>>
> >>>>>>> How other architectures would know that they need to provide the helpers
> >>>>>>> to get walk-without-a-VMA functionality? This looks very fragile to me.
> >>>>>>
> >>>>>> Yes, you've got a good point there. This would apply to the p?d_large
> >>>>>> macros as well - any arch which (inadvertently) uses the generic version
> >>>>>> is likely to be fragile/broken.
> >>>>>>
> >>>>>> I think probably the best option here is to scrap the generic versions
> >>>>>> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which
> >>>>>> would enable the new functionality to those arches that opt-in. Do you
> >>>>>> think this would be less fragile?
> >>>>>
> >>>>> These helpers are useful beyond pagewalker.
> >>>>>
> >>>>> Can we actually do some grinding and make *all* archs to provide correct
> >>>>> helpers? Yes, it's tedious, but not that bad.
> >>>>
> >>>> Many architectures simply cannot support non-leaf entries at the higher
> >>>> levels. I think letting the use a generic helper actually does make sense.
> >>>
> >>> I disagree.
> >>>
> >>> It's makes sense if the level doesn't exists on the arch.
> >>
> >> This is what patch 24 [1] of the series does - if the level doesn't
> >> exist then appropriate stubs are provided.
> >>
> >>> But if the level exists, it will be less frugile to ask the arch to
> >>> provide the helper. Even if it is dummy always-false.
> >>
> >> The problem (as I see it), is we need a reliable set of p?d_large()
> >> implementations to be able to walk arbitrary page tables. Either the
> >> entire functionality of walking page tables without a VMA has to be an
> >> opt-in per architecture, or we need to mandate that every architecture
> >> provide these implementations.
> > 
> > I agree that we need a reliable set of p?d_large(), but I'm still not
> > convinced that every architecture should provide these.
> > 
> > Why having generic versions if p?d_large() is more fragile, than e.g.
> > p??__access_permitted() or atomic ops?
> 
> Personally I feel having p?d_large implemented for each arch has the
> following benefits:
> 
>  * Matches p?d_present/p?d_none/p?d_bad which all similarly have to be
> implemented for all arches except for folded levels (when folded using
> the generic code).
> 
>  * Gives the architecture maintainers a heads-up and an opportunity to
> ensure that the implementations I've written are correct rather than
> silently picking up the generic version.
> 
>  * When adding a new architecture it will be obvious that p?d_large
> implementations are needed.
> 
> The benefits of having a generic version seem to be:
> 
>  * No boiler plate for the architectures that don't support large pages
> (saves a handful of lines).
> 
>  * Easier to merge (fewer patches).
> 
> While the last one is certainly appealing (to me at least), I'm not
> convinced the benefits of the generic version outweigh those of
> providing implementations per-arch.
> 
> Am I missing something?
> 
> > IMHO, adding those functions/macros for architectures that support large
> > pages and providing defines to avoid override of 'static inline' implementations
> > would be robust enough and will avoid unnecessary stubs in architectures
> > that don't have large pages.
> 
> Clearly at run time there's no difference in the "robustness" - the code
> generation should be the same. So it's purely down to development processes.
> 
> However, if you prefer I can resurrect the generic versions and drop the
> patches that simply add dummy implementations.

My concern was the code duplication, which didn't seem necessary. It's not
only about saving a handful of lines, but rather having as many of the code
shared by different architectures actually shared and not copied.

I'd really appreciate having the dummy versions in include/asm-generic
rather than all over arch/*/include/asm.
 
> Steve
> 

-- 
Sincerely yours,
Mike.