Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Ian Jackson <Ian.Jackson@eu.citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 09/11] gnttab: avoid spurious maptrack handle allocation failures
Date: Thu, 22 Jun 2017 08:16:07 -0600	[thread overview]
Message-ID: <594BED470200007800165CF4@prv-mh.provo.novell.com> (raw)
In-Reply-To: <7222befd-aaf6-a609-18b2-70b4a376393b@citrix.com>

>>> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel