| Message ID | 594A5A7902000078001650BA@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/06/17 10:37, Jan Beulich wrote: > When no memory is available in the hypervisor, rather than immediately > failing the request try to steal a handle from another vCPU. "request, try" > > Reported-by: George Dunlap <george.dunlap@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -397,7 +397,7 @@ get_maptrack_handle( > struct vcpu *curr = current; > unsigned int i, head; > grant_handle_t handle; > - struct grant_mapping *new_mt; > + struct grant_mapping *new_mt = NULL; > > handle = __get_maptrack_handle(lgt, curr); > if ( likely(handle != -1) ) > @@ -408,8 +408,13 @@ get_maptrack_handle( > /* > * If we've run out of frames, try stealing an entry from another > * VCPU (in case the guest isn't mapping across its VCPUs evenly). > + * Also use this path in case we're out of memory, to avoid spurious > + * failures. > */ > - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) > + if ( nr_maptrack_frames(lgt) < max_maptrack_frames ) > + new_mt = alloc_xenheap_page(); > + > + if ( !new_mt ) > { > /* > * Can drop the lock since no other VCPU can be adding a new * frame once they've run out. */ It doesn't look like this comment is true any more, which brings the locking correctness into question. ~Andrew > @@ -432,12 +437,6 @@ get_maptrack_handle( > return steal_maptrack_handle(lgt, curr); > } > > - new_mt = alloc_xenheap_page(); > - if ( !new_mt ) > - { > - spin_unlock(&lgt->maptrack_lock); > - return -1; > - } > clear_page(new_mt); > > /* > > >
>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote: > On 21/06/17 10:37, Jan Beulich wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -397,7 +397,7 @@ get_maptrack_handle( >> struct vcpu *curr = current; >> unsigned int i, head; >> grant_handle_t handle; >> - struct grant_mapping *new_mt; >> + struct grant_mapping *new_mt = NULL; >> >> handle = __get_maptrack_handle(lgt, curr); >> if ( likely(handle != -1) ) >> @@ -408,8 +408,13 @@ get_maptrack_handle( >> /* >> * If we've run out of frames, try stealing an entry from another >> * VCPU (in case the guest isn't mapping across its VCPUs evenly). >> + * Also use this path in case we're out of memory, to avoid spurious >> + * failures. >> */ >> - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) >> + if ( nr_maptrack_frames(lgt) < max_maptrack_frames ) >> + new_mt = alloc_xenheap_page(); >> + >> + if ( !new_mt ) >> { >> /* >> * Can drop the lock since no other VCPU can be adding a new > > * frame once they've run out. > */ > > It doesn't look like this comment is true any more, which brings the > locking correctness into question. Oh, indeed. I'll need to revive the locking change I had done here and then dropped because we did realize we didn't need it for XSA-218. Jan
>>> On 21.06.17 at 14:02, <andrew.cooper3@citrix.com> wrote: > On 21/06/17 10:37, Jan Beulich wrote: >> @@ -408,8 +408,13 @@ get_maptrack_handle( >> /* >> * If we've run out of frames, try stealing an entry from another >> * VCPU (in case the guest isn't mapping across its VCPUs evenly). >> + * Also use this path in case we're out of memory, to avoid spurious >> + * failures. >> */ >> - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) >> + if ( nr_maptrack_frames(lgt) < max_maptrack_frames ) >> + new_mt = alloc_xenheap_page(); >> + >> + if ( !new_mt ) >> { >> /* >> * Can drop the lock since no other VCPU can be adding a new > > * frame once they've run out. > */ > > It doesn't look like this comment is true any more, which brings the > locking correctness into question. The whole function acts on current only, so races aren't possible at all. The comment therefore is misleading, and I now think the maptrack_lock is pointless altogether. Jan
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -397,7 +397,7 @@ get_maptrack_handle( struct vcpu *curr = current; unsigned int i, head; grant_handle_t handle; - struct grant_mapping *new_mt; + struct grant_mapping *new_mt = NULL; handle = __get_maptrack_handle(lgt, curr); if ( likely(handle != -1) ) @@ -408,8 +408,13 @@ get_maptrack_handle( /* * If we've run out of frames, try stealing an entry from another * VCPU (in case the guest isn't mapping across its VCPUs evenly). + * Also use this path in case we're out of memory, to avoid spurious + * failures. */ - if ( nr_maptrack_frames(lgt) >= max_maptrack_frames ) + if ( nr_maptrack_frames(lgt) < max_maptrack_frames ) + new_mt = alloc_xenheap_page(); + + if ( !new_mt ) { /* * Can drop the lock since no other VCPU can be adding a new @@ -432,12 +437,6 @@ get_maptrack_handle( return steal_maptrack_handle(lgt, curr); } - new_mt = alloc_xenheap_page(); - if ( !new_mt ) - { - spin_unlock(&lgt->maptrack_lock); - return -1; - } clear_page(new_mt); /*