| Message ID | 20200129143831.1369-3-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | purge free_shared_domheap_page() | expand |
On 29.01.2020 15:38, Paul Durrant wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -727,8 +727,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > (j * (1UL << exch.out.extent_order))); > > spin_lock(&d->page_alloc_lock); > - drop_dom_ref = (dec_count && > - !domain_adjust_tot_pages(d, -dec_count)); > + drop_dom_ref = !domain_adjust_tot_pages(d, -dec_count); And it's only now that I see it in this shape that it becomes clear to me why the change above shouldn't be done, and why in your other patch code should be written similar to the above: The abstract model requires that the domain reference be dropped only when ->tot_pages _transitions_ to zero. No drop should occur if the count was already zero. Granted this may be technically impossible in the specific case here, but the code would still better reflect this general model, to prevent it getting (mis-)cloned into other places. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 29 January 2020 15:08 > To: Durrant, Paul <pdurrant@amazon.co.uk> > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; > Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini > <sstabellini@kernel.org>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v6 2/4] mm: modify domain_adjust_tot_pages() to better > handle a zero adjustment > > On 29.01.2020 15:38, Paul Durrant wrote: > > --- a/xen/common/memory.c > > +++ b/xen/common/memory.c > > @@ -727,8 +727,7 @@ static long > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > > (j * (1UL << exch.out.extent_order))); > > > > spin_lock(&d->page_alloc_lock); > > - drop_dom_ref = (dec_count && > > - !domain_adjust_tot_pages(d, - > dec_count)); > > + drop_dom_ref = !domain_adjust_tot_pages(d, -dec_count); > > And it's only now that I see it in this shape that it becomes > clear to me why the change above shouldn't be done, and why in > your other patch code should be written similar to the above: > The abstract model requires that the domain reference be > dropped only when ->tot_pages _transitions_ to zero. No drop > should occur if the count was already zero. Granted this may > be technically impossible in the specific case here, but the > code would still better reflect this general model, to prevent > it getting (mis-)cloned into other places. > Ok, I guess I'll drop this and then make sure that free_domheap_pages() avoids an erroneous ref drop. Paul
diff --git a/xen/common/memory.c b/xen/common/memory.c index c7d2bac452..a4a5374d26 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -727,8 +727,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) (j * (1UL << exch.out.extent_order))); spin_lock(&d->page_alloc_lock); - drop_dom_ref = (dec_count && - !domain_adjust_tot_pages(d, -dec_count)); + drop_dom_ref = !domain_adjust_tot_pages(d, -dec_count); spin_unlock(&d->page_alloc_lock); if ( drop_dom_ref ) diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 919a270587..135e15bae0 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain *d, long pages) { long dom_before, dom_after, dom_claimed, sys_before, sys_after; + if ( !pages ) + goto out; + ASSERT(spin_is_locked(&d->page_alloc_lock)); d->tot_pages += pages;
Currently the function will pointlessly acquire and release the global 'heap_lock' in this case. NOTE: No caller yet calls domain_adjust_tot_pages() with a zero 'pages' argument, but a subsequent patch will make this possible. Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien@xen.org> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Wei Liu <wl@xen.org> v6: - Modify memory_exchange() v5: - Split out from the subsequent 'make MEMF_no_refcount pages safe to assign' patch as requested by Jan --- xen/common/memory.c | 3 +-- xen/common/page_alloc.c | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-)