Re: [PATCH v3 7/9] xen/blkback: separate ring information out of struct xen_blkif

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Bob Liu <bob.liu@oracle.com>
Cc: <xen-devel@lists.xen.org>, <david.vrabel@citrix.com>,
	<linux-kernel@vger.kernel.org>, <konrad.wilk@oracle.com>,
	<felipe.franciosi@citrix.com>, <axboe@fb.com>,
	<hch@infradead.org>, <avanzini.arianna@gmail.com>,
	<rafal.mielniczuk@citrix.com>, <boris.ostrovsky@oracle.com>,
	<jonathan.davies@citrix.com>
Subject: Re: [PATCH v3 7/9] xen/blkback: separate ring information out of struct xen_blkif
Date: Mon, 19 Oct 2015 11:36:23 +0200	[thread overview]
Message-ID: <5624B997.4020907@citrix.com> (raw)
In-Reply-To: <56188F4A.3060802@oracle.com>

El 10/10/15 a les 6.08, Bob Liu ha escrit:
> On 10/05/2015 10:55 PM, Roger Pau Monné wrote:
>> The same for the pool of persistent grants, it should be per-device and
>> not per-ring.
>>
>> And I think this issue is far worse than the others, because a frontend
>> might use a persistent grant on different queues, forcing the backend
>> map the grant several times for each queue, this is not acceptable IMO.
>>
> 
> Hi Roger,
> 
> I realize it would make things complicate if making persistent grant per-device instead of per-queue.
> Extra locks are required to protect the per-device pool on both blkfront and blkback.

Yes, I realize this, but without having at least a prototype it's hard
to tell if contention is going to be a problem or not. We already use a
red-black tree to store persistent grants, which should be quite fast
when performing searches.

IMHO, we are doing things backwards, we should have investigated first
if using per-device was a problem, and if it indeed was a problem then
move to per-queue. Using per-device just required adding locks around
the functions to get/put grants and friends, leaving the data structures
untouched (per-device).

> AFAIR, there was a discussion before about dropping persistent grant map at all.
> The only reason we left this feature was backward compatibility.
> So that I think we should not complicate xen-block code any more because of a going to be dropped feature.
> 
> How about disable feature-persistent if multi-queue was used?

This is not what we have done in the past, also there's no way for
blkback to tell the frontend that persistent grants and multiqueue
cannot be used at the same time. Blkback puts all supported features on
xenstore before knowing anything about the frontend.

Also, if you want to do it per-queue instead of per-device the limits
need to be properly adjusted, not just the persistent grants one, but
also the queue of cached free pages. This also implies that each queue
it's going to have it's own LRU and purge task.

Roger.