On 4/30/07, Christoph Lameter <[email protected]> wrote:
There are a couple of places where kmap_atomic is surrounding two
memory operations. Usually only one of them is performed. So it is
possible to also use zero_user_page there.
I do like the patch, but would prefer if you'd give a better/correct
rationale here. "Usually only one of them is performed" is not exactly
correct to say, as it is perfectly (and frequently so) possible for
both of (block_end > to) and (block_start < from) to be true for the
same page for the prepare_write cases.
A simple "Replace open-coded kmap_atomic() and kunmap_atomic()
surrounding two memory clear operations with zero_user_page(), as both
memory operations act on the same page" would have been better.
Perhaps you were more worried with the additional overhead of two
successive kmap_atomic() and kunmap_atomic() calls for the same page
in the two resulting zero_user_page()'s (if both conditions evaluate
to true for the same page), but that would still be a price to pay to
replace the current open-coding.
--- linux-2.6.21-rc7-mm1.orig/fs/libfs.c 2007-04-25 00:24:10.000000000 -0700
+++ linux-2.6.21-rc7-mm1/fs/libfs.c 2007-04-25 00:25:56.000000000 -0700
@@ -337,12 +337,12 @@ int simple_prepare_write(struct file *fi
unsigned from, unsigned to)
{
if (!PageUptodate(page)) {
- if (to - from != PAGE_CACHE_SIZE) {
- void *kaddr = kmap_atomic(page, KM_USER0);
- memset(kaddr, 0, from);
- memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
- flush_dcache_page(page);
- kunmap_atomic(kaddr, KM_USER0);
+ if (to - from != PAGE_CACHE_SIZE)
+ if (from)
+ zero_user_page(page, 0, from, KM_USER0);
+ if (to < PAGE_CACHE_SIZE)
+ zero_user_page(page, to,
+ PAGE_CACHE_SIZE - to, KM_USER0);
Why the two additional condition checks? The previous code didn't have
(or need) them, so this patch clearly does something more than simply
replacing open-coding with zero_user_page().
Either you've fixed an issue (in which case this should've been a
different patch with the accompanying explanation) or else I don't see
what we gain with the additional if's. Again, we still do incur the
overhead of two successive kmap_atomic() / kunmap_atomic() calls for
the same page in order to replace the open-coding.
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
