Re: [PATCH v9 2/7] mm: introduce clear_pages() and clear_user_pages()

[David Hildenbrand (Red Hat)]

Replying here while I am already at it.

> > +#ifndef clear_pages
> > +/**
> > + * clear_pages() - clear a page range for kernel-internal use.
> > + * @addr: start address
> > + * @npages: number of pages
> > + *
> > + * Use clear_user_pages() instead when clearing a page range to be
> > + * mapped to user space.
> > + *
> > + * Does absolutely no exception handling.
> > + */
> > +static inline void clear_pages(void *addr, unsigned int npages)
> > +{
> > +	do {
> > +		clear_page(addr);
> > +		addr += PAGE_SIZE;
> > +	} while (--npages);
>
> Why a 'do while' instead of a 'while' ?

More efficient when we know that npages > 0.

> Are you certain that this function will never ever be called with a nul
> npages ?

That is the expectation here, yes. We should probably document that 
expectation.

> > +}
> > +#endif
> > +
> >    #ifndef clear_user_page
> >    /**
> >     * clear_user_page() - clear a page to be mapped to user space
> > @@ -3901,6 +3921,27 @@ static inline void clear_user_page(void *addr, unsigned long vaddr, struct page
> >    }
> >    #endif
> >    
> > +/**
> > + * clear_user_pages() - clear a page range to be mapped to user space
> > + * @addr: start address
> > + * @vaddr: start address of the user mapping
> > + * @page: start page
> > + * @npages: number of pages
> > + *
> > + * Assumes that the region (@addr, +@npages) has been validated
> > + * already so this does no exception handling.
> > + */
> > +#ifdef clear_user_pages
> > +void clear_user_pages(void *addr, unsigned long vaddr,
> > +		struct page *page, unsigned int npages);
>
> By doing this you forbid architectures to define it as a static inline,
> is that wanted ?

Note that this is not the intention. The intention is to either use a 
direct mapping to clear_pages(), or fallback to the variant in mm/util.c.

The architecture is currently never expected to provide clear_user_pages().

Wondering if we can make that cleaner.

I'm wondering if the dependency on highmem.h here in mm.h is rather the 
problem.

How I hate this macro crap with arch overrides.

> > +#else
> > +static inline void clear_user_pages(void *addr, unsigned long vaddr,
> > +		struct page *page, unsigned int npages)
> > +{
> > +	clear_pages(addr, npages);
> > +}
> > +#endif
> > +
> >    #ifdef __HAVE_ARCH_GATE_AREA
> >    extern struct vm_area_struct *get_gate_vma(struct mm_struct *mm);
> >    extern int in_gate_area_no_mm(unsigned long addr);
> > diff --git a/mm/util.c b/mm/util.c
> > index 8989d5767528..3c6cd44db1bd 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -1344,3 +1344,16 @@ bool page_range_contiguous(const struct page *page, unsigned long nr_pages)
> >    }
> >    EXPORT_SYMBOL(page_range_contiguous);
> >    #endif
> > +
> > +#ifdef clear_user_page
> > +void clear_user_pages(void *addr,
>
> What happens if clear_user_page is defined but not clear_user_pages ? In
> that case it seems like the definition in linux/mm.h will conflict.

The generic mm.h variant will not set clear_user_page() and consequently 
we map directly to clear_pages().

--
Cheers

David