| Message ID | a74d4a8de609dfba8b561b7ba0795b22e754fa0b.1578503483.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 08.01.2020 18:14, Tamas K Lengyel wrote: > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -652,19 +652,18 @@ static int page_make_sharable(struct domain *d, > return -EBUSY; > } > > - /* Change page type and count atomically */ > - if ( !get_page_and_type(page, d, PGT_shared_page) ) > + /* Check if page is already typed and bail early if it is */ > + if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) > { > spin_unlock(&d->page_alloc_lock); > - return -EINVAL; > + return -EEXIST; > } > > - /* Check it wasn't already sharable and undo if it was */ > - if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) > + /* Change page type and count atomically */ > + if ( !get_page_and_type(page, d, PGT_shared_page) ) > { > spin_unlock(&d->page_alloc_lock); > - put_page_and_type(page); > - return -EEXIST; > + return -EINVAL; > } It would seem to me that either the original or the new code cannot have worked / work: The original variant checked the count _after_ having incremented it, i.e. it expected a 0->1 transition. The new code checks that the count is 1 _before_ doing the get. However, even if this was changed to if ( page->u.inuse.type_info & PGT_count_mask ) I would recommend against the change: Aiui you build upon the fact that a transition to PGT_shared_page can happen only here, and this code holds d->page_alloc_lock. But imo this is making the code more fragile. In fact I can't easily see why the other two cases where PGT_shared_page gets passed to get_page_and_type() can't also effect a 0->1 transition. I can only guess from their BUG_ON()-s that they assume a reference was already acquired somewhere else. Jan
On Mon, Jan 20, 2020 at 9:34 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 08.01.2020 18:14, Tamas K Lengyel wrote: > > --- a/xen/arch/x86/mm/mem_sharing.c > > +++ b/xen/arch/x86/mm/mem_sharing.c > > @@ -652,19 +652,18 @@ static int page_make_sharable(struct domain *d, > > return -EBUSY; > > } > > > > - /* Change page type and count atomically */ > > - if ( !get_page_and_type(page, d, PGT_shared_page) ) > > + /* Check if page is already typed and bail early if it is */ > > + if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) > > { > > spin_unlock(&d->page_alloc_lock); > > - return -EINVAL; > > + return -EEXIST; > > } > > > > - /* Check it wasn't already sharable and undo if it was */ > > - if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) > > + /* Change page type and count atomically */ > > + if ( !get_page_and_type(page, d, PGT_shared_page) ) > > { > > spin_unlock(&d->page_alloc_lock); > > - put_page_and_type(page); > > - return -EEXIST; > > + return -EINVAL; > > } > > It would seem to me that either the original or the new code cannot > have worked / work: The original variant checked the count _after_ > having incremented it, i.e. it expected a 0->1 transition. The new > code checks that the count is 1 _before_ doing the get. > > However, even if this was changed to > > if ( page->u.inuse.type_info & PGT_count_mask ) > > I would recommend against the change: Aiui you build upon the fact > that a transition to PGT_shared_page can happen only here, and this > code holds d->page_alloc_lock. But imo this is making the code more > fragile. In fact I can't easily see why the other two cases where > PGT_shared_page gets passed to get_page_and_type() can't also > effect a 0->1 transition. I can only guess from their BUG_ON()-s > that they assume a reference was already acquired somewhere else. Hm, right, it certainly looks like this patch isn't needed. It has been a while now and I don't recall why exactly I was moving the type count check, it might have just been while I was experimenting and it never got reverted. Thanks, Tamas
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index baa3e35ded..ecbe40545d 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -652,19 +652,18 @@ static int page_make_sharable(struct domain *d, return -EBUSY; } - /* Change page type and count atomically */ - if ( !get_page_and_type(page, d, PGT_shared_page) ) + /* Check if page is already typed and bail early if it is */ + if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) { spin_unlock(&d->page_alloc_lock); - return -EINVAL; + return -EEXIST; } - /* Check it wasn't already sharable and undo if it was */ - if ( (page->u.inuse.type_info & PGT_count_mask) != 1 ) + /* Change page type and count atomically */ + if ( !get_page_and_type(page, d, PGT_shared_page) ) { spin_unlock(&d->page_alloc_lock); - put_page_and_type(page); - return -EEXIST; + return -EINVAL; } /*
Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/mem_sharing.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)