Roger Pau Monne
2013-Jul-31 15:00 UTC
[PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
There's no need to keep the foreign access in a grant if it is not
persistently mapped by the backend. This allows us to free grants that
are not mapped by the backend, thus preventing blkfront from hoarding
all grants.
The main effect of this is that blkfront will only persistently map
the same grants as the backend, and it will always try to use grants
that are already mapped by the backend. Also the number of persistent
grants in blkfront is the same as in blkback (and is controlled by the
value in blkback).
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++++----
1 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 187a437..bc9fc7d 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s,
struct blkfront_info *info,
}
/* Add the persistent grant into the list of free grants */
for (i = 0; i < nseg; i++) {
- list_add(&s->grants_used[i]->node, &info->persistent_gnts);
- info->persistent_gnts_c++;
+ if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+ /*
+ * If the grant is still mapped by the backend (the
+ * backend has chosen to make this grant persistent)
+ * we add it at the head of the list, so it will be
+ * reused first.
+ */
+ list_add(&s->grants_used[i]->node, &info->persistent_gnts);
+ info->persistent_gnts_c++;
+ } else {
+ /*
+ * If the grant is not mapped by the backend we end the
+ * foreign access and add it to the tail of the list,
+ * so it will not be picked again unless we run out of
+ * persistent grants.
+ */
+ gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
+ s->grants_used[i]->gref = GRANT_INVALID_REF;
+ list_add_tail(&s->grants_used[i]->node,
&info->persistent_gnts);
+ }
}
if (s->req.operation == BLKIF_OP_INDIRECT) {
for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
- list_add(&s->indirect_grants[i]->node,
&info->persistent_gnts);
- info->persistent_gnts_c++;
+ if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
+ list_add(&s->indirect_grants[i]->node,
&info->persistent_gnts);
+ info->persistent_gnts_c++;
+ } else {
+ gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
+ s->indirect_grants[i]->gref = GRANT_INVALID_REF;
+ list_add_tail(&s->indirect_grants[i]->node,
+ &info->persistent_gnts);
+ }
}
}
}
--
1.7.7.5 (Apple Git-26)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Matt Wilson
2013-Aug-01 06:35 UTC
Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
On Wed, Jul 31, 2013 at 05:00:44PM +0200, Roger Pau Monne wrote:> There''s no need to keep the foreign access in a grant if it is not > persistently mapped by the backend. This allows us to free grants that > are not mapped by the backend, thus preventing blkfront from hoarding > all grants. > > The main effect of this is that blkfront will only persistently map > the same grants as the backend, and it will always try to use grants > that are already mapped by the backend. Also the number of persistent > grants in blkfront is the same as in blkback (and is controlled by the > value in blkback). > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>Acked-by: Matt Wilson <msw@amazon.com>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: David Vrabel <david.vrabel@citrix.com> > --- > drivers/block/xen-blkfront.c | 33 +++++++++++++++++++++++++++++---- > 1 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 187a437..bc9fc7d 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info, > } > /* Add the persistent grant into the list of free grants */ > for (i = 0; i < nseg; i++) { > - list_add(&s->grants_used[i]->node, &info->persistent_gnts); > - info->persistent_gnts_c++; > + if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { > + /* > + * If the grant is still mapped by the backend (the > + * backend has chosen to make this grant persistent) > + * we add it at the head of the list, so it will be > + * reused first. > + */ > + list_add(&s->grants_used[i]->node, &info->persistent_gnts); > + info->persistent_gnts_c++; > + } else { > + /* > + * If the grant is not mapped by the backend we end the > + * foreign access and add it to the tail of the list, > + * so it will not be picked again unless we run out of > + * persistent grants. > + */ > + gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL); > + s->grants_used[i]->gref = GRANT_INVALID_REF; > + list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts); > + } > } > if (s->req.operation == BLKIF_OP_INDIRECT) { > for (i = 0; i < INDIRECT_GREFS(nseg); i++) { > - list_add(&s->indirect_grants[i]->node, &info->persistent_gnts); > - info->persistent_gnts_c++; > + if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) { > + list_add(&s->indirect_grants[i]->node, &info->persistent_gnts); > + info->persistent_gnts_c++; > + } else { > + gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL); > + s->indirect_grants[i]->gref = GRANT_INVALID_REF; > + list_add_tail(&s->indirect_grants[i]->node, > + &info->persistent_gnts); > + } > } > } > }
David Vrabel
2013-Aug-01 10:10 UTC
Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
On 31/07/13 16:00, Roger Pau Monne wrote:> There's no need to keep the foreign access in a grant if it is not > persistently mapped by the backend. This allows us to free grants that > are not mapped by the backend, thus preventing blkfront from hoarding > all grants. > > The main effect of this is that blkfront will only persistently map > the same grants as the backend, and it will always try to use grants > that are already mapped by the backend. Also the number of persistent > grants in blkfront is the same as in blkback (and is controlled by the > value in blkback). > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>Reviewed-by: David Vrabel <david.vrabel@citrix.com> But please see the documentation updates needed below.> --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info, > } > /* Add the persistent grant into the list of free grants */ > for (i = 0; i < nseg; i++) { > - list_add(&s->grants_used[i]->node, &info->persistent_gnts); > - info->persistent_gnts_c++; > + if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { > + /* > + * If the grant is still mapped by the backend (the > + * backend has chosen to make this grant persistent) > + * we add it at the head of the list, so it will be > + * reused first. > + */ > + list_add(&s->grants_used[i]->node, &info->persistent_gnts); > + info->persistent_gnts_c++; > + } else { > + /* > + * If the grant is not mapped by the backend we end the > + * foreign access and add it to the tail of the list, > + * so it will not be picked again unless we run out of > + * persistent grants. > + */ > + gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL); > + s->grants_used[i]->gref = GRANT_INVALID_REF; > + list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts); > + }Because you only reclaim grants when a request is completed, can you add text similar to the following to xen/include/public/io/blkif.h (and the Linux copy). feature-persistent: ... When the backend driver needs to unmap a persistent grant it should do so prior to completing a request that used that grant reference. If a persistent grant is unmapped at any other time, the frontend driver may not notice and may be unable to reclaim the grant reference. Thanks. David _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Aug-09 15:08 UTC
Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:> On 31/07/13 16:00, Roger Pau Monne wrote: > > There''s no need to keep the foreign access in a grant if it is not > > persistently mapped by the backend. This allows us to free grants that > > are not mapped by the backend, thus preventing blkfront from hoarding > > all grants. > > > > The main effect of this is that blkfront will only persistently map > > the same grants as the backend, and it will always try to use grants > > that are already mapped by the backend. Also the number of persistent > > grants in blkfront is the same as in blkback (and is controlled by the > > value in blkback). > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: David Vrabel <david.vrabel@citrix.com>Roger, Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the comments and the Ack from Matt and Roger''s Review-by tag addressed? Thanks.> > But please see the documentation updates needed below. > > > --- a/drivers/block/xen-blkfront.c > > +++ b/drivers/block/xen-blkfront.c > > @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info, > > } > > /* Add the persistent grant into the list of free grants */ > > for (i = 0; i < nseg; i++) { > > - list_add(&s->grants_used[i]->node, &info->persistent_gnts); > > - info->persistent_gnts_c++; > > + if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { > > + /* > > + * If the grant is still mapped by the backend (the > > + * backend has chosen to make this grant persistent) > > + * we add it at the head of the list, so it will be > > + * reused first. > > + */ > > + list_add(&s->grants_used[i]->node, &info->persistent_gnts); > > + info->persistent_gnts_c++; > > + } else { > > + /* > > + * If the grant is not mapped by the backend we end the > > + * foreign access and add it to the tail of the list, > > + * so it will not be picked again unless we run out of > > + * persistent grants. > > + */ > > + gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL); > > + s->grants_used[i]->gref = GRANT_INVALID_REF; > > + list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts); > > + } > > Because you only reclaim grants when a request is completed, can you add > text similar to the following to xen/include/public/io/blkif.h (and the > Linux copy). > > feature-persistent: > ... > When the backend driver needs to unmap a persistent grant it should > do so prior to completing a request that used that grant reference. > If a persistent grant is unmapped at any other time, the frontend > driver may not notice and may be unable to reclaim the grant > reference. > > Thanks. > > David > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Roger Pau Monné
2013-Aug-12 10:58 UTC
Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote:> On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote: >> On 31/07/13 16:00, Roger Pau Monne wrote: >>> There''s no need to keep the foreign access in a grant if it is not >>> persistently mapped by the backend. This allows us to free grants that >>> are not mapped by the backend, thus preventing blkfront from hoarding >>> all grants. >>> >>> The main effect of this is that blkfront will only persistently map >>> the same grants as the backend, and it will always try to use grants >>> that are already mapped by the backend. Also the number of persistent >>> grants in blkfront is the same as in blkback (and is controlled by the >>> value in blkback). >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Reviewed-by: David Vrabel <david.vrabel@citrix.com> > > Roger, > > Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the > comments and the Ack from Matt and Roger''s Review-by tag addressed?Done (I guess you meant the Reviewed-by tag from David), see below however.> Thanks. >> >> But please see the documentation updates needed below. >> >>> --- a/drivers/block/xen-blkfront.c >>> +++ b/drivers/block/xen-blkfront.c >>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info, >>> } >>> /* Add the persistent grant into the list of free grants */ >>> for (i = 0; i < nseg; i++) { >>> - list_add(&s->grants_used[i]->node, &info->persistent_gnts); >>> - info->persistent_gnts_c++; >>> + if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { >>> + /* >>> + * If the grant is still mapped by the backend (the >>> + * backend has chosen to make this grant persistent) >>> + * we add it at the head of the list, so it will be >>> + * reused first. >>> + */ >>> + list_add(&s->grants_used[i]->node, &info->persistent_gnts); >>> + info->persistent_gnts_c++; >>> + } else { >>> + /* >>> + * If the grant is not mapped by the backend we end the >>> + * foreign access and add it to the tail of the list, >>> + * so it will not be picked again unless we run out of >>> + * persistent grants. >>> + */ >>> + gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL); >>> + s->grants_used[i]->gref = GRANT_INVALID_REF; >>> + list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts); >>> + } >> >> Because you only reclaim grants when a request is completed, can you add >> text similar to the following to xen/include/public/io/blkif.h (and the >> Linux copy). >> >> feature-persistent: >> ... >> When the backend driver needs to unmap a persistent grant it should >> do so prior to completing a request that used that grant reference. >> If a persistent grant is unmapped at any other time, the frontend >> driver may not notice and may be unable to reclaim the grant >> reference. >>I''ve sent a patch to update the documentation about persistent grants in Xen source tree, but the Linux copy of blkif.h lacks any information about the blkif protocol extensions, so I have not added it there. If nobody else does it first (before I come back from vacation), I will send a patch to add the documentation about all the blkif extensions to the Linux copy of blkif.h (and of course add your comment about persistent grants unmap). Roger.
Konrad Rzeszutek Wilk
2013-Aug-12 12:02 UTC
Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend
On Mon, Aug 12, 2013 at 12:58:39PM +0200, Roger Pau Monné wrote:> On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote: > > On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote: > >> On 31/07/13 16:00, Roger Pau Monne wrote: > >>> There''s no need to keep the foreign access in a grant if it is not > >>> persistently mapped by the backend. This allows us to free grants that > >>> are not mapped by the backend, thus preventing blkfront from hoarding > >>> all grants. > >>> > >>> The main effect of this is that blkfront will only persistently map > >>> the same grants as the backend, and it will always try to use grants > >>> that are already mapped by the backend. Also the number of persistent > >>> grants in blkfront is the same as in blkback (and is controlled by the > >>> value in blkback). > >>> > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> > >> Reviewed-by: David Vrabel <david.vrabel@citrix.com> > > > > Roger, > > > > Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the > > comments and the Ack from Matt and Roger''s Review-by tag addressed? > > Done (I guess you meant the Reviewed-by tag from David), see below however. > > > Thanks. > >> > >> But please see the documentation updates needed below. > >> > >>> --- a/drivers/block/xen-blkfront.c > >>> +++ b/drivers/block/xen-blkfront.c > >>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info, > >>> } > >>> /* Add the persistent grant into the list of free grants */ > >>> for (i = 0; i < nseg; i++) { > >>> - list_add(&s->grants_used[i]->node, &info->persistent_gnts); > >>> - info->persistent_gnts_c++; > >>> + if (gnttab_query_foreign_access(s->grants_used[i]->gref)) { > >>> + /* > >>> + * If the grant is still mapped by the backend (the > >>> + * backend has chosen to make this grant persistent) > >>> + * we add it at the head of the list, so it will be > >>> + * reused first. > >>> + */ > >>> + list_add(&s->grants_used[i]->node, &info->persistent_gnts); > >>> + info->persistent_gnts_c++; > >>> + } else { > >>> + /* > >>> + * If the grant is not mapped by the backend we end the > >>> + * foreign access and add it to the tail of the list, > >>> + * so it will not be picked again unless we run out of > >>> + * persistent grants. > >>> + */ > >>> + gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL); > >>> + s->grants_used[i]->gref = GRANT_INVALID_REF; > >>> + list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts); > >>> + } > >> > >> Because you only reclaim grants when a request is completed, can you add > >> text similar to the following to xen/include/public/io/blkif.h (and the > >> Linux copy). > >> > >> feature-persistent: > >> ... > >> When the backend driver needs to unmap a persistent grant it should > >> do so prior to completing a request that used that grant reference. > >> If a persistent grant is unmapped at any other time, the frontend > >> driver may not notice and may be unable to reclaim the grant > >> reference. > >> > > I''ve sent a patch to update the documentation about persistent grants in > Xen source tree, but the Linux copy of blkif.h lacks any information > about the blkif protocol extensions, so I have not added it there. If > nobody else does it first (before I come back from vacation), I will > send a patch to add the documentation about all the blkif extensions to > the Linux copy of blkif.h (and of course add your comment about > persistent grants unmap).Please do. It would be good to resync that header file. Thanks.