Re: [PATCH 2/2] docs: expand persistent grants protocol

["Roger Pau Monné" <roger.pau@citrix.com>]

On 29/11/12 12:34, Ian Campbell wrote:
> On Thu, 2012-11-29 at 11:30 +0000, Roger Pau Monne wrote:
>> On 29/11/12 11:49, Ian Campbell wrote:
>>> On Tue, 2012-11-27 at 10:03 +0000, Roger Pau Monne wrote:
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>>  xen/include/public/io/blkif.h |   24 +++++++++++++++++++++---
>>>>  1 files changed, 21 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
>>>> index db9c379..5a4b9ae 100644
>>>> --- a/xen/include/public/io/blkif.h
>>>> +++ b/xen/include/public/io/blkif.h
>>>> @@ -137,7 +137,15 @@
>>>>   *      can map persistently depends on the implementation, but ideally it
>>>>   *      should be RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST. Using this
>>>>   *      feature the backend doesn't need to unmap each grant, preventing
>>>> - *      costly TLB flushes.
>>>> + *      costly TLB flushes. The backend driver should only map grants
>>>> + *      persistently if the frontend supports it. If a backend driver chooses
>>>> + *      to use the persistent protocol when the frontend doesn't support it,
>>>> + *      it will probably hit the maximum number of persistently mapped grants
>>>> + *      (due to the fact that the frontend won't be reusing the same grants),
>>>> + *      and fall back to non-persistent mode. Backend implementations may
>>>> + *      shrink or expand the number of persistently mapped grants without
>>>> + *      notifying the frontend depending on memory constraints (this might
>>>> + *      cause a performance degradation).
>>>
>>> Is there a recommended/required reuse strategy on either end which
>>> minimises this? You don't want to be in a situation where the backend's
>>> "cache" is full of non-persistent single-shot mappings but the frontend
>>> is now reusing a good set of persistent pages, which are getting
>>> repeatedly mapped/unmapped because the cache is full...
>>
>> Since the only backend implementation is the Linux one, and we set the
>> maximum number of persistenly mapped grants to RING_SIZE *
>> BLKIF_MAX_SEGMENTS_PER_REQUEST, we don't have any kind of support for
>> shrinking/expanding. If a frontend is using more than this number of
>> grants it is considered that the frontend is broken/hostile.
>>
>> If in the future some kind of shrinking/expanding is implemented, I
>> guess the natural way to do it would be to add a counter to keep track
>> of how many times a grant is used, and try to maintain the grants most
>> commonly used mapped.
> 
> You mean expire the mappings on an LRU basis? That seems sensible and
> would be compatible with a LIFO strategy in the f.e.
> 
> NB this doesn't require shrinking/expanding in the backend, but can also
> happen with a broken f.e. as you observe. We should still try and do
> something sane with a f.e. which follows our recommendations though.

We should recommend that all implementations use a LIFO queue in the
frontend in case a backend decides to limit the number of mapped grants
to something less than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST, in
which case the recommendation for shrinking should be to use a LRU
strategy in the backend.

> 
>> On the blkfront side, we are using a LIFO strategy to store persistently
>> mapped grants, so we are trying to always reuse the same subset of
>> persistently mapped grants when blkfront is not under heavy I/O load.
> 
> I think this is worth documenting as an actual implementation
> recommendation, to improve interoperability. If someone happened to
> implement a FIFO instead then they would get some sort of pathological
> behaviour.
> 
> It may even be worth documenting that the b.e. should use an LRU scheme.

Thanks for the review, I'm going to add the recommendations we talked
about to the protocol and resubmit.


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