[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/3] xen/privcmd: Mark pages as dirty

On 2020-07-07 04:43, Jürgen Groß wrote:
On 07.07.20 13:30, Souptick Joarder wrote:
On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@xxxxxxxx> wrote:
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 33677ea..f6c1543 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned 
int nr_pages)
       unsigned int i;

-     for (i = 0; i < nr_pages; i++)
+     for (i = 0; i < nr_pages; i++) {
+             if (!PageDirty(pages[i]))
+                     set_page_dirty_lock(pages[i]);

With put_page() directly following I think you should be able to use
set_page_dirty() instead, as there is obviously a reference to the page

Patch [3/3] will convert above codes to use unpin_user_pages_dirty_lock()
which internally do the same check. So I thought to keep linux-stable and
linux-next code in sync. John had a similar concern [1] and later agreed to keep
this check.

Shall I keep this check ?  No ?

It doesn't matter *too* much, because patch 3/3 fixes up everything by
changing it all to unpin_user_pages_dirty_lock(). However, there is something
to be said for having correct interim patches, too. :)  Details:


I wasn't referring to checking PageDirty(), but to the use of

Looking at the comment just before the implementation of
set_page_dirty_lock() suggests that it is fine to use set_page_dirty()
instead (so not calling lock_page()).

no no, that's a misreading of the comment. Unless this xen/privcmd code has
somehow taken a reference on page->mapping->host (which I do *not* think is
the case), then it is still racy to call set_page_dirty() here. Instead,
set_page_dirty_lock() should be used.

Only the transition from get_user_pages_fast() to pin_user_pages_fast()
requires to use the locked version IMO.

That's a different misunderstanding. :) pin_user_pages*() APIs are meant to be
functionally drop-in replacements for get_user_pages*(). Internally,
pin_user_pages*() functions do some additional tracking, but from a caller's
perspective, it should look the same. In other words, there is nothing
about pin_user_pages_fast() that requires set_page_dirty_lock() upon release.
The reason set_page_dirty_lock() was chosen is that there are very few
(none at all?) call sites that need to release and dirty a page, that also meet
the requirements to safely call set_page_dirty().

That's why there is a unpin_user_pages_dirty_lock(), but there is not a
corresponding unpin_user_pages_dirty() call: the latter has not been required
so far, even though the call site conversions are nearly done.

John Hubbard



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.