Re: add page argument to copy/clear_user_page
Linus Torvalds writes: > > As for the `to' argument, yes it is redundant since it is just kmap(page). > > And why not let "clear_page()" just do that itself? OK, here's a patch that does that. > The thing is, copy/clear_page shouldn't exist at all (or rather, the > "highpage" versions should be renamed to the non-highpage names, because > the non-highmem case simply isn't interesting any more). Each architecture already had a clear_page that was functionally equivalent to memset(p, 0, PAGE_SIZE), but often in assembler, and likewise a copy_page that was equivalent to memcpy(d, s, PAGE_SIZE). So I renamed all the existing clear_page's and copy_page's to __clear_page and __copy_page (since they are "lower-level" or "raw" clear/copy page routines). In highmem.h I have renamed copy_highpage to copy_page and clear_highpage to clear_page. I also have default versions of copy_user_page and clear_user_page which just do copy_page/clear_page for those architectures that don't have any cache issues to deal with. Architectures can define __HAVE_ARCH_USER_PAGE in asm/page.h and then define their own copy/clear_user_page routines if they want to. I have fixed up all the architectures except sparc64. There the copy/clear_user_page routines are in assembler and my sparc assembler is pretty rusty these days (particularly when DaveM goes doing hairy things with the %g registers :). I'll let Dave fix that one up; the change is that copy/clear_user_page take page * arguments instead of void * arguments. This patch is a fair bit bigger than the last one, but most of the bulk is just the renaming of clear_page to __clear_page and copy_page to __copy_page. I also renamed memclear_highpage to memclear_page (which isn't actually used anywhere) and memclear_highpage_flush to memclear_page_flush. Let me know what you think of this; if it's OK, could you apply it to your tree? Thanks, Paul. diff -urN linux/Documentation/cachetlb.txt linux.new/Documentation/cachetlb.txt --- linux/Documentation/cachetlb.txtSat Mar 31 03:05:54 2001 +++ linux.new/Documentation/cachetlb.txtWed May 23 20:48:38 2001 @@ -260,8 +260,9 @@ Here is the new interface: - void copy_user_page(void *to, void *from, unsigned long address) - void clear_user_page(void *to, unsigned long address) + void copy_user_page(struct page *to, struct page *from, + unsigned long address) + void clear_user_page(struct page *to, unsigned long address) These two routines store data in user anonymous or COW pages. It allows a port to efficiently avoid D-cache alias @@ -279,6 +280,11 @@ If D-cache aliasing is not an issue, these two routines may simply call memcpy/memset directly and do nothing more. + + There are default versions of these procedures supplied in + include/linux/highmem.h. If a port does not want to use the + default versions it should declare them and define the symbol + __HAVE_ARCH_USER_PAGE in include/asm/page.h. void flush_dcache_page(struct page *page) diff -urN linux/arch/alpha/kernel/alpha_ksyms.c linux.new/arch/alpha/kernel/alpha_ksyms.c --- linux/arch/alpha/kernel/alpha_ksyms.c Sat Apr 28 23:02:30 2001 +++ linux.new/arch/alpha/kernel/alpha_ksyms.c Wed May 23 20:39:23 2001 @@ -98,8 +98,8 @@ EXPORT_SYMBOL(__memset); EXPORT_SYMBOL(__memsetw); EXPORT_SYMBOL(__constant_c_memset); -EXPORT_SYMBOL(copy_page); -EXPORT_SYMBOL(clear_page); +EXPORT_SYMBOL(__copy_page); +EXPORT_SYMBOL(__clear_page); EXPORT_SYMBOL(__direct_map_base); EXPORT_SYMBOL(__direct_map_size); diff -urN linux/arch/alpha/lib/clear_page.S linux.new/arch/alpha/lib/clear_page.S --- linux/arch/alpha/lib/clear_page.S Thu Feb 22 14:24:52 2001 +++ linux.new/arch/alpha/lib/clear_page.S Wed May 23 20:39:23 2001 @@ -6,9 +6,9 @@ .text .align 4 - .global clear_page - .ent clear_page -clear_page: + .global __clear_page + .ent __clear_page +__clear_page: .prologue 0 lda $0,128 @@ -36,4 +36,4 @@ unop nop - .end clear_page + .end __clear_page diff -urN linux/arch/alpha/lib/copy_page.S linux.new/arch/alpha/lib/copy_page.S --- linux/arch/alpha/lib/copy_page.SThu Feb 22 14:24:52 2001 +++ linux.new/arch/alpha/lib/copy_page.SWed May 23 21:05:31 2001 @@ -6,9 +6,9 @@ .text .align 4 - .global copy_page - .ent copy_page -copy_page: + .global __copy_page + .ent __copy_page +__copy_page: .prologue 0 lda $18,128 @@ -46,4 +46,4 @@ unop nop - .end copy_page + .end __copy_page diff -urN linux/arch/alpha/lib/ev6-clear_page.S linux.new/arch/alpha/lib/ev6-clear_page.S --- linux/arch/alpha/lib/ev6-clear_page.S Thu Feb 22 14:24:52 2001 +++ linux.new/arch/alpha/lib/ev6-clear_page.S Wed May 23 20:39:23 2001 @@ -6,9 +6,9 @@ .text .align 4 -.global clear_page
Re: add page argument to copy/clear_user_page
On Mon, 21 May 2001, Paul Mackerras wrote: > > As for the `to' argument, yes it is redundant since it is just kmap(page). And why not let "clear_page()" just do that itself? The only place that doesn't already do "kmap(page)" is basically get_zeroed_page(), and the only reason it doesn't do that is because the whole function is fundamentally not able to handle high memory pages (it returns a fixed address, not the "struct page *". But that function is also likely to not care about the extra five cycles or so of having to do the kmap() by making clear_page() (and copy_page()) always use "struct page *" and do kmap() internally. Because most people who care about performance are already using other functions (in fact, the functions that _can_ allocate high memory). And I hate redundancy, and having different functions for the same thing. > But copy/clear_user_page isn't the interface that gets called from the > MM stuff, copy/clear_user_highpage is, defined in include/linux/highmem.h. > These are two of a whole series of functions which all do kmap, do > something, kunmap. The thing is, copy/clear_page shouldn't exist at all (or rather, the "highpage" versions should be renamed to the non-highpage names, because the non-highmem case simply isn't interesting any more). The highmem special casing used to make sense back when highmem was a rare special case. These days, we should just get rid of the distinction as much as possible, Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: add page argument to copy/clear_user_page
Linus Torvalds writes:
> If you add the page argument, why leave the old arguments lingering there
> at all? They only create confusion, and add no information.
You mean the `to' pointer argument, or the `vaddr' argument? The
`vaddr' argument isn't redundant, it's the user virtual address where
the page is mapped, and sparc64 needs it in order to avoid D-cache
aliasing issues I believe. (Dave?)
As for the `to' argument, yes it is redundant since it is just kmap(page).
But copy/clear_user_page isn't the interface that gets called from the
MM stuff, copy/clear_user_highpage is, defined in include/linux/highmem.h.
These are two of a whole series of functions which all do kmap, do
something, kunmap.
IMHO having the kmap/kunmap calls in copy/clear_user_highpage in
include/linux/highmem.h is the best approach because it means that
most architectures can just #define copy/clear_user_page as
copy/clear_page in include/asm/page.h (as they do at the moment). It
means that the kmap/kunmap calls are in one place only instead of
being duplicated in every architecture.
But we could instead push the kmap/kunmap down into copy/clear_user_page.
Then we might as well rename them into copy/clear_user_highpage. Here
is how it might turn out in include/asm-i386/page.h (and asm-alpha,
asm-arm, asm-crus, asm-s390, asm-sh, asm-s390x...):
extern void clear_page(void *page);
extern void copy_page(void * _to, void * _from);
#define clear_user_highpage(page, vaddr)\
do {\
struct page *__page = page; \
clear_page(kmap(__page)); \
kunmap(__page); \
} while (0)
#define copy_user_highpage(to, from, vaddr) \
do {\
struct page *__to = to, *__from = from; \
copy_page(kmap(__to), kmap(__from));\
kunmap(__from); \
kunmap(__to); \
} while (0)
Doing it with inline functions would be cleaner but would mean that we
would need the declaration of kmap/kunmap in page.h. That would mean
that we would need to #include in include/asm/page.h
which is starting to get pretty messy and inviting circular
inclusions. We could move these declarations to another file in
include/asm - include/asm/highmem.h might seem the natural place but
it is only used if CONFIG_HIGHMEM is defined and not all ports have
it.
I assume nobody wants to do these functions out-of-line. :)
So on the whole the way I had it seems cleanest to me. But I can whip
up a patch to do the kmap/kunmap in the architecture-specific files
instead, if you prefer - if so, do you prefer the macro version or the
inline function version?
Paul.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: add page argument to copy/clear_user_page
On Sun, 20 May 2001, Paul Mackerras wrote: > > The patch below adds a page * argument to copy_user_page and > clear_user_page. If you add the page argument, why leave the old arguments lingering there at all? They only create confusion, and add no information. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
