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.
