Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/include/public/io/blkif.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h index accdda4..db9c379 100644 --- a/xen/include/public/io/blkif.h +++ b/xen/include/public/io/blkif.h @@ -307,7 +307,7 @@ * the grants. * (8) The frontend driver has to allow the backend driver to map all grants * with write access, even when they should be mapped read-only, since - * further requests may reuse this grants and require write permisions. + * further requests may reuse this grants and require write permissions. */ /* -- 1.7.7.5 (Apple Git-26) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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). * *----------------------- Request Transport Parameters ------------------------ * @@ -258,11 +266,17 @@ * feature-persistent * Values: 0/1 (boolean) * Default Value: 0 - * Notes: 7, 8 + * Notes: 7, 8, 9 * * A value of "1" indicates that the frontend will reuse the same grants * for all transactions, allowing the backend to map them with write - * access (even when it should be read-only). + * access (even when it should be read-only). If the frontend hits the + * maximum number of allowed persistenlty mapped grants, it can fallback + * to non persistent mode. This will cause a performance degradation, + * since the the backend driver will still try to map those grants + * persistently. Since the persistent grants protocol is compatible with + * the previous protocol, a frontend driver can choose to work in + * persistent mode even when the backend doesn't support it. * *------------------------- Virtual Device Properties ------------------------- * @@ -308,6 +322,10 @@ * (8) The frontend driver has to allow the backend driver to map all grants * with write access, even when they should be mapped read-only, since * further requests may reuse this grants and require write permissions. + * (9) Linux implementation doesn't have a limit on the maximum number of + * grants that can be persisntly mapped in the frontend driver, but + * due to the frontent driver implementation it should never be bigger + * than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST. */ /* -- 1.7.7.5 (Apple Git-26) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h > index accdda4..db9c379 100644 > --- a/xen/include/public/io/blkif.h > +++ b/xen/include/public/io/blkif.h > @@ -307,7 +307,7 @@ > * the grants. > * (8) The frontend driver has to allow the backend driver to map all grants > * with write access, even when they should be mapped read-only, since > - * further requests may reuse this grants and require write permisions. > + * further requests may reuse this grants and require write permissions."these grants" probably? (alternatively "this grant" but I don't think that is what is meant).> */ > > /*_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monné
2012-Nov-28 15:35 UTC
Re: [PATCH 1/2] docs: fix persistent grants doc typo
On 27/11/12 12:29, 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 | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h >> index accdda4..db9c379 100644 >> --- a/xen/include/public/io/blkif.h >> +++ b/xen/include/public/io/blkif.h >> @@ -307,7 +307,7 @@ >> * the grants. >> * (8) The frontend driver has to allow the backend driver to map all grants >> * with write access, even when they should be mapped read-only, since >> - * further requests may reuse this grants and require write permisions. >> + * further requests may reuse this grants and require write permissions. > > "these grants" probably? (alternatively "this grant" but I don't think > that is what is meant).Yes, "these grants" is correct. Do I need to resend this, or can the committer change it?> >> */ >> >> /* > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, 2012-11-28 at 15:35 +0000, Roger Pau Monne wrote:> On 27/11/12 12:29, 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 | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h > >> index accdda4..db9c379 100644 > >> --- a/xen/include/public/io/blkif.h > >> +++ b/xen/include/public/io/blkif.h > >> @@ -307,7 +307,7 @@ > >> * the grants. > >> * (8) The frontend driver has to allow the backend driver to map all grants > >> * with write access, even when they should be mapped read-only, since > >> - * further requests may reuse this grants and require write permisions. > >> + * further requests may reuse this grants and require write permissions. > > > > "these grants" probably? (alternatively "this grant" but I don't think > > that is what is meant). > > Yes, "these grants" is correct. Do I need to resend this, or can the > committer change it?I'll do it when I commit.> > > > >> */ > >> > >> /* > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Nov-29 10:49 UTC
Re: [PATCH 2/2] docs: expand persistent grants protocol
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...> * > *----------------------- Request Transport Parameters ------------------------ > * > @@ -258,11 +266,17 @@ > * feature-persistent > * Values: 0/1 (boolean) > * Default Value: 0 > - * Notes: 7, 8 > + * Notes: 7, 8, 9 > * > * A value of "1" indicates that the frontend will reuse the same grants > * for all transactions, allowing the backend to map them with write > - * access (even when it should be read-only). > + * access (even when it should be read-only). If the frontend hits the > + * maximum number of allowed persistenlty mapped grants, it can fallbackpersistently> + * to non persistent mode. This will cause a performance degradation, > + * since the the backend driver will still try to map those grants > + * persistently. Since the persistent grants protocol is compatible with > + * the previous protocol, a frontend driver can choose to work in > + * persistent mode even when the backend doesn't support it. > * > *------------------------- Virtual Device Properties ------------------------- > * > @@ -308,6 +322,10 @@ > * (8) The frontend driver has to allow the backend driver to map all grants > * with write access, even when they should be mapped read-only, since > * further requests may reuse this grants and require write permissions. > + * (9) Linux implementation doesn't have a limit on the maximum number of > + * grants that can be persisntly mapped in the frontend driver, butpersistently> + * due to the frontent driver implementation it should never be bigger > + * than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST. > */ > > /*_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Wed, 2012-11-28 at 15:59 +0000, Ian Campbell wrote:> On Wed, 2012-11-28 at 15:35 +0000, Roger Pau Monne wrote: > > On 27/11/12 12:29, 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 | 2 +- > > >> 1 files changed, 1 insertions(+), 1 deletions(-) > > >> > > >> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h > > >> index accdda4..db9c379 100644 > > >> --- a/xen/include/public/io/blkif.h > > >> +++ b/xen/include/public/io/blkif.h > > >> @@ -307,7 +307,7 @@ > > >> * the grants. > > >> * (8) The frontend driver has to allow the backend driver to map all grants > > >> * with write access, even when they should be mapped read-only, since > > >> - * further requests may reuse this grants and require write permisions. > > >> + * further requests may reuse this grants and require write permissions. > > > > > > "these grants" probably? (alternatively "this grant" but I don't think > > > that is what is meant). > > > > Yes, "these grants" is correct. Do I need to resend this, or can the > > committer change it? > > I'll do it when I commit.Which I've now done.> > > > > > > > >> */ > > >> > > >> /* > > > > > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monné
2012-Nov-29 11:30 UTC
Re: [PATCH 2/2] docs: expand persistent grants protocol
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. 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.> >> * >> *----------------------- Request Transport Parameters ------------------------ >> * >> @@ -258,11 +266,17 @@ >> * feature-persistent >> * Values: 0/1 (boolean) >> * Default Value: 0 >> - * Notes: 7, 8 >> + * Notes: 7, 8, 9 >> * >> * A value of "1" indicates that the frontend will reuse the same grants >> * for all transactions, allowing the backend to map them with write >> - * access (even when it should be read-only). >> + * access (even when it should be read-only). If the frontend hits the >> + * maximum number of allowed persistenlty mapped grants, it can fallback > > persistently > >> + * to non persistent mode. This will cause a performance degradation, >> + * since the the backend driver will still try to map those grants >> + * persistently. Since the persistent grants protocol is compatible with >> + * the previous protocol, a frontend driver can choose to work in >> + * persistent mode even when the backend doesn't support it. >> * >> *------------------------- Virtual Device Properties ------------------------- >> * >> @@ -308,6 +322,10 @@ >> * (8) The frontend driver has to allow the backend driver to map all grants >> * with write access, even when they should be mapped read-only, since >> * further requests may reuse this grants and require write permissions. >> + * (9) Linux implementation doesn't have a limit on the maximum number of >> + * grants that can be persisntly mapped in the frontend driver, but > > persistently > >> + * due to the frontent driver implementation it should never be bigger >> + * than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST. >> */ >> >> /* > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Nov-29 11:34 UTC
Re: [PATCH 2/2] docs: expand persistent grants protocol
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.> 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. Ian, _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Roger Pau Monné
2012-Nov-29 12:43 UTC
Re: [PATCH 2/2] docs: expand persistent grants protocol
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