| Date | Tue, 21 May 2013 14:28:13 -0700 | From | Dave Hansen <> | Subject | Re: [PATCHv4 20/39] thp, mm: naive support of thp in generic read/write routines |
| |
On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote: > + if (PageTransHuge(page)) > + offset = pos & ~HPAGE_PMD_MASK; > + > pagefault_disable(); > - copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes); > + copied = iov_iter_copy_from_user_atomic( > + page + (offset >> PAGE_CACHE_SHIFT), > + i, offset & ~PAGE_CACHE_MASK, bytes); > pagefault_enable(); > flush_dcache_page(page);
I think there's enough voodoo in there to warrant a comment or adding some temporary variables. There are three things going on that you wan to convey:
1. Offset is normally <PAGE_SIZE, but you make it <HPAGE_PMD_SIZE if you are dealing with a huge page 2. (offset >> PAGE_CACHE_SHIFT) is always 0 for small pages since offset < PAGE_SIZE 3. "offset & ~PAGE_CACHE_MASK" does nothing for small-page offsets, but it turns a large-page offset back in to a small-page-offset.
I think you can do it with something like this:
int subpage_nr = 0; off_t smallpage_offset = offset; if (PageTransHuge(page)) { // we transform 'offset' to be offset in to the huge // page instead of inside the PAGE_SIZE page offset = pos & ~HPAGE_PMD_MASK; subpage_nr = (offset >> PAGE_CACHE_SHIFT); } > + copied = iov_iter_copy_from_user_atomic( > + page + subpage_nr, > + i, smallpage_offset, bytes);
> @@ -2437,6 +2453,7 @@ again: > * because not all segments in the iov can be copied at > * once without a pagefault. > */ > + offset = pos & ~PAGE_CACHE_MASK;
Urg, and now it's *BACK* in to a small-page offset?
This means that 'offset' has two _different_ meanings and it morphs between them during the function a couple of times. That seems very error-prone to me.
> bytes = min_t(unsigned long, PAGE_CACHE_SIZE - offset, > iov_iter_single_seg_count(i)); > goto again; >
|