| Message ID | 20211208042256.1923824-8-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Folios for 5.17 | expand |
> + size_t offset = offset_in_folio(folio, start + __off); \ > + if (xas_retry(&xas, folio)) \ > continue; \ > + if (WARN_ON(xa_is_value(folio))) \ > break; \ > + if (WARN_ON(folio_test_hugetlb(folio))) \ > break; \ > + while (offset < folio_size(folio)) { \ Nit: I'd be tempted to use a for loop on offset here. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Dec 22, 2021 at 10:57:28PM -0800, Christoph Hellwig wrote: > > + size_t offset = offset_in_folio(folio, start + __off); \ > > + if (xas_retry(&xas, folio)) \ > > continue; \ > > + if (WARN_ON(xa_is_value(folio))) \ > > break; \ > > + if (WARN_ON(folio_test_hugetlb(folio))) \ > > break; \ > > + while (offset < folio_size(folio)) { \ > > Nit: I'd be tempted to use a for loop on offset here. Dave, this is your code originally ... any opinions? > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de>
Christoph Hellwig <hch@infradead.org> wrote: > > + size_t offset = offset_in_folio(folio, start + __off); \ > > + if (xas_retry(&xas, folio)) \ > > continue; \ > > + if (WARN_ON(xa_is_value(folio))) \ > > break; \ > > + if (WARN_ON(folio_test_hugetlb(folio))) \ > > break; \ > > + while (offset < folio_size(folio)) { \ > > Nit: I'd be tempted to use a for loop on offset here. A while-loop makes more sense here. The adjustment you need for offset (ie. len) is overwritten after offset is altered at the end of the loop: > + offset += len; \ > + len = PAGE_SIZE; \ > } \ So you'd have to move both of these into the for-incrementor expression, in addition to moving in the initialiser expression, making the thing less readable. David
On Thu, Dec 23, 2021 at 03:24:21PM +0000, David Howells wrote: > > > + while (offset < folio_size(folio)) { \ > > > > Nit: I'd be tempted to use a for loop on offset here. > > A while-loop makes more sense here. The adjustment you need for offset > (ie. len) is overwritten after offset is altered at the end of the loop: > > > + offset += len; \ > > + len = PAGE_SIZE; \ > > } \ > > So you'd have to move both of these into the for-incrementor expression, in > addition to moving in the initialiser expression, making the thing less > readable. Ok.
diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 66a740e6e153..03cf43b003a0 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -69,42 +69,39 @@ #define iterate_xarray(i, n, base, len, __off, STEP) { \ __label__ __out; \ size_t __off = 0; \ - struct page *head = NULL; \ + struct folio *folio; \ loff_t start = i->xarray_start + i->iov_offset; \ - unsigned offset = start % PAGE_SIZE; \ pgoff_t index = start / PAGE_SIZE; \ - int j; \ - \ XA_STATE(xas, i->xarray, index); \ \ + len = PAGE_SIZE - offset_in_page(start); \ rcu_read_lock(); \ - xas_for_each(&xas, head, ULONG_MAX) { \ + xas_for_each(&xas, folio, ULONG_MAX) { \ unsigned left; \ - if (xas_retry(&xas, head)) \ + size_t offset = offset_in_folio(folio, start + __off); \ + if (xas_retry(&xas, folio)) \ continue; \ - if (WARN_ON(xa_is_value(head))) \ + if (WARN_ON(xa_is_value(folio))) \ break; \ - if (WARN_ON(PageHuge(head))) \ + if (WARN_ON(folio_test_hugetlb(folio))) \ break; \ - for (j = (head->index < index) ? index - head->index : 0; \ - j < thp_nr_pages(head); j++) { \ - void *kaddr = kmap_local_page(head + j); \ - base = kaddr + offset; \ - len = PAGE_SIZE - offset; \ + while (offset < folio_size(folio)) { \ + base = kmap_local_folio(folio, offset); \ len = min(n, len); \ left = (STEP); \ - kunmap_local(kaddr); \ + kunmap_local(base); \ len -= left; \ __off += len; \ n -= len; \ if (left || n == 0) \ goto __out; \ - offset = 0; \ + offset += len; \ + len = PAGE_SIZE; \ } \ } \ __out: \ rcu_read_unlock(); \ - i->iov_offset += __off; \ + i->iov_offset += __off; \ n = __off; \ }
Take advantage of how kmap_local_folio() works to simplify the loop. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- lib/iov_iter.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)