From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: [PATCHv8 2/3] gnttab: per-active entry locking
Date: Tue, 19 May 2015 14:46:43 +0100
Message-ID: <555B3EC3.9060702@citrix.com>
References: <1431440115-27936-1-git-send-email-david.vrabel@citrix.com>
	<1431440115-27936-3-git-send-email-david.vrabel@citrix.com>
	<555B1007020000780007B772@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <david.vrabel@citrix.com>) id 1YuhrG-0001Ja-3a
	for xen-devel@lists.xenproject.org; Tue, 19 May 2015 13:46:58 +0000
In-Reply-To: <555B1007020000780007B772@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, David Vrabel <david.vrabel@citrix.com>
Cc: Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Christoph Egger <chegger@amazon.de>, Tim Deegan <tim@xen.org>, Matt Wilson <msw@amazon.com>, xen-devel@lists.xenproject.org, Malcolm Crossley <malcolm.crossley@citrix.com>
List-Id: xen-devel@lists.xenproject.org

On 19/05/15 09:27, Jan Beulich wrote:
>>>> On 12.05.15 at 16:15, <david.vrabel@citrix.com> wrote:
>> @@ -546,15 +554,28 @@ static void mapcount(
>>  
>>      *wrc = *rdc = 0;
>>  
>> +    /* 
>> +     * N.B.: while taking the local maptrack spinlock prevents any
>> +     * mapping changes, the remote active entries could be changing
>> +     * while we are counting. The caller has to hold the grant table
>> +     * write lock, or some other mechanism should be used to prevent
>> +     * concurrent changes during this operation.  This is tricky
>> +     * because we can't promote a read lock into a write lock.
>> +     */
>> +    ASSERT(rw_is_locked(&rd->grant_table->lock));
> 
> So both callers only hold the read lock - what is this "other mechanism"
> that they employ?

This comment is just nonsense.  We must hold the grant table write lock
for mapcount() to be safe -- the maptrack lock doesn't help and as you
say, this "other mechanism" just doesn't exist.

I think the callers of mapcount() need to be refactored so iommu updates
and the mapcount() call are done with the write lock held only (the
maptrack nor active entry locks are relevant here).

David