On 10/12/18 12:35 AM, Balbir Singh wrote:
On Thu, Oct 11, 2018 at 11:00:10PM -0700, john.hubbard@xxxxxxxxx wrote:
From: John Hubbard <jhubbard@xxxxxxxxxx>
[...]>> +/*
+ * put_user_pages_dirty() - for each page in the @pages array, make
+ * that page (or its head page, if a compound page) dirty, if it was
+ * previously listed as clean. Then, release the page using
+ * put_user_page().
+ *
+ * Please see the put_user_page() documentation for details.
+ *
+ * set_page_dirty(), which does not lock the page, is used here.
+ * Therefore, it is the caller's responsibility to ensure that this is
+ * safe. If not, then put_user_pages_dirty_lock() should be called instead.
+ *
+ * @pages: array of pages to be marked dirty and released.
+ * @npages: number of pages in the @pages array.
+ *
+ */
+void put_user_pages_dirty(struct page **pages, unsigned long npages)
+{
+ unsigned long index;
+
+ for (index = 0; index < npages; index++) {
Do we need any checks on npages, npages <= (PUD_SHIFT - PAGE_SHIFT)?
Hi Balbir,
Thanks for combing through this series.
I'd go with "probably not", because the only check that can be done is
what you showed above: "did someone crazy pass in more pages than are
possible for this system?". I don't think checking for that helps here,
as that will show up loud and clear, in other ways.
The release_pages() implementation made the same judgment call to not
check npages, which also influenced me.
A VM_BUG_ON could be added but I'd prefer not to, as it seems to have
not enough benefit to be worth it.
+ struct page *page = compound_head(pages[index]);
+
+ if (!PageDirty(page))
+ set_page_dirty(page);
+
+ put_user_page(page);
+ }
+}
+EXPORT_SYMBOL(put_user_pages_dirty);
+
+/*
+ * put_user_pages_dirty_lock() - for each page in the @pages array, make
+ * that page (or its head page, if a compound page) dirty, if it was
+ * previously listed as clean. Then, release the page using
+ * put_user_page().
+ *
+ * Please see the put_user_page() documentation for details.
+ *
+ * This is just like put_user_pages_dirty(), except that it invokes
+ * set_page_dirty_lock(), instead of set_page_dirty().
+ *
+ * @pages: array of pages to be marked dirty and released.
+ * @npages: number of pages in the @pages array.
+ *
+ */
+void put_user_pages_dirty_lock(struct page **pages, unsigned long npages)
+{
+ unsigned long index;
+
+ for (index = 0; index < npages; index++) {
+ struct page *page = compound_head(pages[index]);
+
+ if (!PageDirty(page))
+ set_page_dirty_lock(page);
+
+ put_user_page(page);
+ }
+}
+EXPORT_SYMBOL(put_user_pages_dirty_lock);
+
This can be collapsed w.r.t put_user_pages_dirty, a function pointer indirection
for the locked vs unlocked case, not sure how that affects function optimization.
OK, good point. I initially wanted to avoid the overhead of a function
pointer, but considering that there are lots of other operations
happening, I think you're right: best to just get rid of the code
duplication. If later on we find that changing it back actually helps
any benchmarks, that can always be done.
See below for how I'm planning on fixing it, and it is a nice little
cleanup, so thanks for pointing that out.
+/*
+ * put_user_pages() - for each page in the @pages array, release the page
+ * using put_user_page().
+ *
+ * Please see the put_user_page() documentation for details.
+ *
+ * This is just like put_user_pages_dirty(), except that it invokes
+ * set_page_dirty_lock(), instead of set_page_dirty().
The comment is incorrect.
Yes, it sure is! Jan spotted it before, and I fixed it once, then
rebased off of the version right *before* the fix, so now I have to
delete that sentence again. It's hard to kill! :)
+ *
+ * @pages: array of pages to be marked dirty and released.
+ * @npages: number of pages in the @pages array.
+ *
+ */
+void put_user_pages(struct page **pages, unsigned long npages)
+{
+ unsigned long index;
+
+ for (index = 0; index < npages; index++)
+ put_user_page(pages[index]);
+}
Ditto in terms of code duplication
Here, I think you'll find that the end result, is sufficiently
de-duplicated, after applying the function pointer above. Here's what it
looks like without the comment blocks, below. I don't want to go any
further than this, because the only thing left is the "for" loops, and
macro-izing such a trivial thing is not really a win:
typedef int (*set_dirty_func)(struct page *page);
static void __put_user_pages_dirty(struct page **pages,
unsigned long npages,
set_dirty_func sdf)
{
unsigned long index;
for (index = 0; index < npages; index++) {
struct page *page = compound_head(pages[index]);
if (!PageDirty(page))
sdf(page);
put_user_page(page);
}
}
void put_user_pages_dirty(struct page **pages, unsigned long npages)
{
__put_user_pages_dirty(pages, npages, set_page_dirty);
}
EXPORT_SYMBOL(put_user_pages_dirty);
void put_user_pages_dirty_lock(struct page **pages, unsigned long npages)
{
__put_user_pages_dirty(pages, npages, set_page_dirty_lock);
}
EXPORT_SYMBOL(put_user_pages_dirty_lock);
void put_user_pages(struct page **pages, unsigned long npages)
{
unsigned long index;
for (index = 0; index < npages; index++)
put_user_page(pages[index]);
}
EXPORT_SYMBOL(put_user_pages);
--
thanks,
John Hubbard
NVIDIA
