On Fri, 24 Feb 2012 14:34:31 +0100 Daniel Vetter daniel@ffwll.ch wrote:
--- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -408,6 +408,7 @@ extern void add_page_wait_queue(struct page *page, wait_queue_t *waiter); static inline int fault_in_pages_writeable(char __user *uaddr, int size) { int ret;
char __user *end = uaddr + size - 1;
if (unlikely(size == 0)) return 0;
@@ -416,17 +417,20 @@ static inline int fault_in_pages_writeable(char __user *uaddr, int size) * Writing zeroes into userspace here is OK, because we know that if * the zero gets there, we'll be overwriting it. */
- ret = __put_user(0, uaddr);
- while (uaddr <= end) {
ret = __put_user(0, uaddr);if (ret != 0)return ret;uaddr += PAGE_SIZE;- }
The callsites in filemap.c are pretty hot paths, which is why this thing remains explicitly inlined. I think it would be worth adding a bit of code here to avoid adding a pointless test-n-branch and larger cache footprint to read() and write().
A way of doing that is to add another argument to these functions, say "bool multipage". Change the code to do
if (multipage) { while (uaddr <= end) { ... } }
and change the callsites to pass in constant "true" or "false". Then compile it up and manually check that the compiler completely removed the offending code from the filemap.c callsites.
Wanna have a think about that? If it all looks OK then please be sure to add code comments explaining why we did this.
I wasn't really happy with the added branch either, but failed to come up with a trick to avoid it. Imho adding new _multipage variants of these functions instead of adding a constant argument is simpler because the functions don't really share much thanks to the block below. I'll see what it looks like (and obviously add a comment explaining what's going on).
well... that's just syntactic sugar:
static inline int __fault_in_pages_writeable(char __user *uaddr, int size, bool multipage) { ... }
static inline int fault_in_pages_writeable(char __user *uaddr, int size) { return __fault_in_pages_writeable(uaddr, size, false); }
static inline int fault_in_multipages_writeable(char __user *uaddr, int size) { return __fault_in_pages_writeable(uaddr, size, true); }
which I don't think is worth bothering with given the very small number of callsites.