On Wed, 26 Nov 2008, Russell King wrote:
> I've not had any response to this, so in liu of any response by this
> coming weekend, I'm going to assume that everyone's happy with this
> change (at which point it's going to become buried under a lot of
> merges with other trees.)
>
> ----- Forwarded message from Russell King <rmk+lkml@xxxxxxxxxxxxxxxx> -----
>
> Date: Thu, 20 Nov 2008 17:50:17 +0000
> From: Russell King <rmk+lkml@xxxxxxxxxxxxxxxx>
> To: linux-arch@xxxxxxxxxxxxxxx,
> Linux Kernel List <linux-kernel@xxxxxxxxxxxxxxx>,
> linux-mm@xxxxxxxxx
> Subject: [PATCH] [ARM] clearpage: provide our own clear_user_highpage()
>
> This patch is part of a larger ARM specific patch set cleaning up
> aliasing VIPT cache support.
>
> With aliasing VIPT cache support, our implementation of clear_user_page()
> and copy_user_page() sets up a temporary kernel space mapping such that
> we have the same cache colour as the userspace page. This avoids having
> to consider any userspace aliases from this operation.
>
> However, when highmem is enabled, kmap_atomic() have to setup mappings.
> The copy_user_highpage() and clear_user_highpage() call these functions
> before delegating the copies to copy_user_page() and clear_user_page().
>
> The effect of this is that each of the *_user_highpage() functions setup
> their own kmap mapping, followed by the *_user_page() functions setting
> up another mapping. This is rather wasteful.
>
> Thankfully, copy_user_highpage() can be overriden by architectures by
> defining __HAVE_ARCH_COPY_USER_HIGHPAGE. However, replacement of
> clear_user_highpage() is more difficult because its inline definition
> is not conditional. It seems that you're expected to define
> __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE and provide a replacement
> __alloc_zeroed_user_highpage() implementation instead.
>
> The allocation itself is fine, so we don't want to override that. What
> we really want to do is to override clear_user_highpage() with our own
> version which doesn't kmap_atomic() unnecessarily.
>
> However, there are two drivers (drivers/media/video/videobuf-dma-sg.c
> and drivers/staging/go7007/go7007-v4l2.c) which want to provide non-
> highmem clear_user_page()'d pages to userspace.
>
> Requiring an architecture to provide __alloc_zeroed_user_highpage(),
> a sub-optimal clear_user_page(), and keep the sub-optimal
> clear_user_highpage() around seems rather silly and potentially
> error prone.
>
> So, what this patch below does is allow clear_user_highpage() itself
> to be overriden by architectures, so that they can provide just one
> implementation.
>
> What needs to follow on from this is converting those two drivers to
> use clear_user_highpage() instead of clear_user_page() - that should
> be a trivial patch.
>
> Are there any objections to this approach? Can I get any acked-by's
> from any MM folk for the include/linux/highmem.h change?
Looks eminently sensible to me, and the #define clear_user_highpage
style you've chosen is indeed our favoured technique these days.
I expect we might want to get away from that mixture of __HAVE_ARCH..
and #define actual_function_name all in the same highmem.h, but
no reason why you should have to be the one to do such cleanup.
> From: Russell King <rmk+lkml@xxxxxxxxxxxxxxxx>
> Date: Mon, 17 Nov 2008 14:08:49 +0000
> Subject: Re: [PATCH] [ARM] clearpage: provide our own clear_user_highpage()
>
> From: Russell King <rmk@xxxxxxxxxxxxxxxxxxxxxxx>
>
> For similar reasons as copy_user_page(), we want to avoid the
> additional kmap_atomic if it's unnecessary.
>
> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
Acked-by: Hugh Dickins <hugh@xxxxxxxxxxx>
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -63,12 +63,14 @@ static inline void *kmap_atomic(struct page *page, enum km_type idx)
> #endif /* CONFIG_HIGHMEM */
>
> /* when CONFIG_HIGHMEM is not set these will be plain clear/copy_page */
> +#ifndef clear_user_highpage
> static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
> {
> void *addr = kmap_atomic(page, KM_USER0);
> clear_user_page(addr, vaddr, page);
> kunmap_atomic(addr, KM_USER0);
> }
> +#endif
>
> #ifndef __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE
> /**
>
> ----- End forwarded message -----
>
> ----- End forwarded message -----
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
