lkml.org 
[lkml]   [2015]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 16/34] x86, mm: simplify get_user_pages() PTE bit handling
    On Thu, 3 Dec 2015, Dave Hansen wrote:
    >
    > From: Dave Hansen <dave.hansen@linux.intel.com>
    >
    > The current get_user_pages() code is a wee bit more complicated
    > than it needs to be for pte bit checking. Currently, it establishes
    > a mask of required pte _PAGE_* bits and ensures that the pte it
    > goes after has all those bits.
    >
    > This consolidates the three identical copies of this code.
    >
    > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
    > ---
    >
    > b/arch/x86/mm/gup.c | 45 ++++++++++++++++++++++++++++-----------------
    > 1 file changed, 28 insertions(+), 17 deletions(-)
    >
    > diff -puN arch/x86/mm/gup.c~pkeys-16-gup-swizzle arch/x86/mm/gup.c
    > --- a/arch/x86/mm/gup.c~pkeys-16-gup-swizzle 2015-12-03 16:21:25.148649631 -0800
    > +++ b/arch/x86/mm/gup.c 2015-12-03 16:21:25.151649767 -0800
    > @@ -63,6 +63,30 @@ retry:
    > #endif
    > }
    >
    > +static inline int pte_allows_gup(pte_t pte, int write)
    > +{
    > + /*
    > + * 'pte' can reall be a pte, pmd or pud. We only check
    > + * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
    > + * are the same value on all 3 types.
    > + */
    > + if (!(pte_flags(pte) & (_PAGE_PRESENT|_PAGE_USER)))
    > + return 0;
    > + if (write && !(pte_write(pte)))
    > + return 0;
    > + return 1;
    > +}
    > +
    > +static inline int pmd_allows_gup(pmd_t pmd, int write)
    > +{
    > + return pte_allows_gup(*(pte_t *)&pmd, write);
    > +}
    > +
    > +static inline int pud_allows_gup(pud_t pud, int write)
    > +{
    > + return pte_allows_gup(*(pte_t *)&pud, write);
    > +}

    This still puzzles me. And the only reason it compiles is because we
    have -fno-strict-aliasing set ...

    All this operates on the pteval or even just on the pte_flags(). Even
    the new arch_pte_access_permitted() thingy which you add later is only
    interrested in pte_flags() and its just a wrapper around
    __pkru_allows_pkey().

    So for readability and simplicity sake, can we please do something
    like this (pkey check already added):

    /*
    * 'pteval' can reall be a pte, pmd or pud. We only check
    * _PAGE_PRESENT, _PAGE_USER, and _PAGE_RW in here which
    * are the same value on all 3 types.
    */
    static inline int pte_allows_gup(unsigned long pteval, int write)
    {
    unsigned long mask = _PAGE_PRESENT|_PAGE_USER;

    if (write)
    mask |= _PAGE_RW;

    if ((pteval & mask) != mask)
    return 0;

    if (!__pkru_allows_pkey(pte_flags_pkey(pteval), write))
    return 0;
    return 1;
    }

    and at the callsites do:

    if (pte_allows_gup(pte_val(pte, write))

    if (pte_allows_gup(pmd_val(pmd, write))

    if (pte_allows_gup(pud_val(pud, write))

    Hmm?


    tglx



    \
     
     \ /
      Last update: 2015-12-08 19:21    [W:4.145 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site