Re: [Xen-devel] [PATCH v2 4/4] xen-blkif: drop struct blkif_request_segment_aligned
On 07.02.14 04:38, Matthew Rushton wrote: >> This was wrongly introduced in commit 402b27f9, the only difference >> between blkif_request_segment_aligned and blkif_request_segment is >> that the former has a named padding, while both share the same >> memory layout. >> >> Also correct a few minor glitches in the description, including for it >> to no longer assume PAGE_SIZE == 4096. > > Tested-by: Matt Rushton > > *Corrected subject line from last email and resent. I tested the set and > everything looks solid. I also reviewed patch 2 and 3. > Tested-by: Christoph Egger Reviewed-by: Christoph Egger Christoph -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH v2 3/4] xen-blkback: fix shutdown race
On 07.02.14 04:23, Matthew Rushton wrote: > On 04/02/14 10:26, Roger Pau Monne wrote: >> Introduce a new variable to keep track of the number of in-flight >> requests. We need to make sure that when xen_blkif_put is called the >> request has already been freed and we can safely free xen_blkif, which >> was not the case before. > > Tested-by: Matt Rushton > Reviewed-by: Matt Rushton Tested-by: Christoph Egger Reviewed-by: Christoph Egger Christoph -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH v2 2/4] xen-blkback: fix memory leaks
On 07.02.14 04:15, Matthew Rushton wrote: > On 04/02/14 10:26, Roger Pau Monne wrote: >> I've at least identified two possible memory leaks in blkback, both >> related to the shutdown path of a VBD: >> >> - blkback doesn't wait for any pending purge work to finish before >> cleaning the list of free_pages. The purge work will call >> put_free_pages and thus we might end up with pages being added to >> the free_pages list after we have emptied it. Fix this by making >> sure there's no pending purge work before exiting >> xen_blkif_schedule, and moving the free_page cleanup code to >> xen_blkif_free. >> - blkback doesn't wait for pending requests to end before cleaning >> persistent grants and the list of free_pages. Again this can add >> pages to the free_pages list or persistent grants to the >> persistent_gnts red-black tree. Fixed by moving the persistent >> grants and free_pages cleanup code to xen_blkif_free. >> >> Also, add some checks in xen_blkif_free to make sure we are cleaning >> everything. > > Tested-by: Matt Rushton > Reviewed-by: Matt Rushton Tested-by: Christoph Egger Reviewed-by: Christoph Egger Christoph -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH v2 2/4] xen-blkback: fix memory leaks
On 07.02.14 04:15, Matthew Rushton wrote: On 04/02/14 10:26, Roger Pau Monne wrote: I've at least identified two possible memory leaks in blkback, both related to the shutdown path of a VBD: - blkback doesn't wait for any pending purge work to finish before cleaning the list of free_pages. The purge work will call put_free_pages and thus we might end up with pages being added to the free_pages list after we have emptied it. Fix this by making sure there's no pending purge work before exiting xen_blkif_schedule, and moving the free_page cleanup code to xen_blkif_free. - blkback doesn't wait for pending requests to end before cleaning persistent grants and the list of free_pages. Again this can add pages to the free_pages list or persistent grants to the persistent_gnts red-black tree. Fixed by moving the persistent grants and free_pages cleanup code to xen_blkif_free. Also, add some checks in xen_blkif_free to make sure we are cleaning everything. Tested-by: Matt Rushton mrush...@amazon.com Reviewed-by: Matt Rushton mrush...@amazon.com Tested-by: Christoph Egger cheg...@amazon.de Reviewed-by: Christoph Egger cheg...@amazon.de Christoph -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH v2 3/4] xen-blkback: fix shutdown race
On 07.02.14 04:23, Matthew Rushton wrote: On 04/02/14 10:26, Roger Pau Monne wrote: Introduce a new variable to keep track of the number of in-flight requests. We need to make sure that when xen_blkif_put is called the request has already been freed and we can safely free xen_blkif, which was not the case before. Tested-by: Matt Rushton mrush...@amazon.com Reviewed-by: Matt Rushton mrush...@amazon.com Tested-by: Christoph Egger cheg...@amazon.de Reviewed-by: Christoph Egger cheg...@amazon.de Christoph -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH v2 4/4] xen-blkif: drop struct blkif_request_segment_aligned
On 07.02.14 04:38, Matthew Rushton wrote: This was wrongly introduced in commit 402b27f9, the only difference between blkif_request_segment_aligned and blkif_request_segment is that the former has a named padding, while both share the same memory layout. Also correct a few minor glitches in the description, including for it to no longer assume PAGE_SIZE == 4096. Tested-by: Matt Rushton mrush...@amazon.com *Corrected subject line from last email and resent. I tested the set and everything looks solid. I also reviewed patch 2 and 3. Tested-by: Christoph Egger cheg...@amazon.de Reviewed-by: Christoph Egger cheg...@amazon.de Christoph -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH] xen-blkback: fix memory leak when persistent grants are used
On 23.01.14 20:28, Matt Wilson wrote: > From: Matt Rushton > > Currently shrink_free_pagepool() is called before the pages used for > persistent grants are released via free_persistent_gnts(). This > results in a memory leak when a VBD that uses persistent grants is > torn down. This memory leak was introduced with commit c6cc142dac52e62e1e8a2aff5de1300202b96c66 xen-blkback: use balloon pages for all mappings Christoph > Cc: Konrad Rzeszutek Wilk > Cc: "Roger Pau Monné" > Cc: Ian Campbell > Cc: David Vrabel > Cc: linux-kernel@vger.kernel.org > Cc: xen-de...@lists.xen.org > Cc: Anthony Liguori > Signed-off-by: Matt Rushton > Signed-off-by: Matt Wilson > --- > drivers/block/xen-blkback/blkback.c |6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/xen-blkback/blkback.c > b/drivers/block/xen-blkback/blkback.c > index 6620b73..30ef7b3 100644 > --- a/drivers/block/xen-blkback/blkback.c > +++ b/drivers/block/xen-blkback/blkback.c > @@ -625,9 +625,6 @@ purge_gnt_list: > print_stats(blkif); > } > > - /* Since we are shutting down remove all pages from the buffer */ > - shrink_free_pagepool(blkif, 0 /* All */); > - > /* Free all persistent grant pages */ > if (!RB_EMPTY_ROOT(>persistent_gnts)) > free_persistent_gnts(blkif, >persistent_gnts, > @@ -636,6 +633,9 @@ purge_gnt_list: > BUG_ON(!RB_EMPTY_ROOT(>persistent_gnts)); > blkif->persistent_gnt_c = 0; > > + /* Since we are shutting down remove all pages from the buffer */ > + shrink_free_pagepool(blkif, 0 /* All */); > + > if (log_stats) > print_stats(blkif); > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH] xen-blkback: fix memory leak when persistent grants are used
On 23.01.14 20:28, Matt Wilson wrote: From: Matt Rushton mrush...@amazon.com Currently shrink_free_pagepool() is called before the pages used for persistent grants are released via free_persistent_gnts(). This results in a memory leak when a VBD that uses persistent grants is torn down. This memory leak was introduced with commit c6cc142dac52e62e1e8a2aff5de1300202b96c66 xen-blkback: use balloon pages for all mappings Christoph Cc: Konrad Rzeszutek Wilk konrad.w...@oracle.com Cc: Roger Pau Monné roger@citrix.com Cc: Ian Campbell ian.campb...@citrix.com Cc: David Vrabel david.vra...@citrix.com Cc: linux-kernel@vger.kernel.org Cc: xen-de...@lists.xen.org Cc: Anthony Liguori aligu...@amazon.com Signed-off-by: Matt Rushton mrush...@amazon.com Signed-off-by: Matt Wilson m...@amazon.com --- drivers/block/xen-blkback/blkback.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c index 6620b73..30ef7b3 100644 --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -625,9 +625,6 @@ purge_gnt_list: print_stats(blkif); } - /* Since we are shutting down remove all pages from the buffer */ - shrink_free_pagepool(blkif, 0 /* All */); - /* Free all persistent grant pages */ if (!RB_EMPTY_ROOT(blkif-persistent_gnts)) free_persistent_gnts(blkif, blkif-persistent_gnts, @@ -636,6 +633,9 @@ purge_gnt_list: BUG_ON(!RB_EMPTY_ROOT(blkif-persistent_gnts)); blkif-persistent_gnt_c = 0; + /* Since we are shutting down remove all pages from the buffer */ + shrink_free_pagepool(blkif, 0 /* All */); + if (log_stats) print_stats(blkif); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Xen-devel] [PATCH RFC 4/4] xen-block: introduce a new request type to unmap grants
On 10.07.13 11:19, Roger Pau Monné wrote:
> On 08/07/13 21:41, Konrad Rzeszutek Wilk wrote:
>> On Mon, Jul 08, 2013 at 03:03:27PM +0200, Roger Pau Monne wrote:
>>> Right now blkfront has no way to unmap grant refs, if using persistent
>>> grants once a grant is used blkfront cannot assure if blkback will
>>> have this grant mapped or not. To solve this problem, a new request
>>> type (BLKIF_OP_UNMAP) that allows requesting blkback to unmap certain
>>> grants is introduced.
>>
>> I don't think this is the right way of doing it. It is a new operation
>> (BLKIF_OP_UNMAP) that has nothing to do with READ/WRITE. All it is
>> is just some way for the frontend to say: unmap this grant if you can.
>>
>> As such I would think a better mechanism would be to have a new
>> grant mechanism that can say: 'I am done with this grant you can
>> remove it' - that is called to the hypervisor. The hypervisor
>> can then figure out whether it is free or not and lazily delete it.
>> (And the guest would be notified when it is freed).
>
> I have a patch that I think implements something quite similar to what
> you describe, but it doesn't require any new patch to the hypervisor
> side. From blkfront we can check what grants blkback has chosen to
> persistently map and only keep those.
>
> This is different from my previous approach, were blkfront could
> specifically request blkback to unmap certain grants, but it still
> prevents blkfront from hoarding all grants (unless blkback is
> persistently mapping every possible grant). With this patch the number
> of persistent grants in blkfront will be the same as in blkback, so
> basically the backend can control how many grants will be persistently
> mapped.
According to your blog
http://blog.xen.org/index.php/2012/11/23/improving-block-protocol-scalability-with-persistent-grants/
persistent grants in the frontend gives an benefit even when the backend does
not support
persistent grants. Is this still the case with this patch?
Christoph
> ---
> From 1ede72ba10a7ec13d57ba6d2af54e86a099d7125 Mon Sep 17 00:00:00 2001
> From: Roger Pau Monne
> Date: Wed, 10 Jul 2013 10:22:19 +0200
> Subject: [PATCH RFC] xen-blkfront: revoke foreign access for grants not
> mapped by the backend
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> 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é
> Cc: Konrad Rzeszutek Wilk
> ---
> 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 3d445c0..6ba88c1 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1022,13 +1022,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(>grants_used[i]->node, >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(>grants_used[i]->node,
> >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(>grants_used[i]->node,
> >persistent_gnts);
> + }
> }
> if (s->req.operation == BLKIF_OP_INDIRECT) {
> for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
> - list_add(>indirect_grants[i]->node,
> >persistent_gnts);
> -
Re: [Xen-devel] [PATCH RFC 4/4] xen-block: introduce a new request type to unmap grants
On 10.07.13 11:19, Roger Pau Monné wrote:
On 08/07/13 21:41, Konrad Rzeszutek Wilk wrote:
On Mon, Jul 08, 2013 at 03:03:27PM +0200, Roger Pau Monne wrote:
Right now blkfront has no way to unmap grant refs, if using persistent
grants once a grant is used blkfront cannot assure if blkback will
have this grant mapped or not. To solve this problem, a new request
type (BLKIF_OP_UNMAP) that allows requesting blkback to unmap certain
grants is introduced.
I don't think this is the right way of doing it. It is a new operation
(BLKIF_OP_UNMAP) that has nothing to do with READ/WRITE. All it is
is just some way for the frontend to say: unmap this grant if you can.
As such I would think a better mechanism would be to have a new
grant mechanism that can say: 'I am done with this grant you can
remove it' - that is called to the hypervisor. The hypervisor
can then figure out whether it is free or not and lazily delete it.
(And the guest would be notified when it is freed).
I have a patch that I think implements something quite similar to what
you describe, but it doesn't require any new patch to the hypervisor
side. From blkfront we can check what grants blkback has chosen to
persistently map and only keep those.
This is different from my previous approach, were blkfront could
specifically request blkback to unmap certain grants, but it still
prevents blkfront from hoarding all grants (unless blkback is
persistently mapping every possible grant). With this patch the number
of persistent grants in blkfront will be the same as in blkback, so
basically the backend can control how many grants will be persistently
mapped.
According to your blog
http://blog.xen.org/index.php/2012/11/23/improving-block-protocol-scalability-with-persistent-grants/
persistent grants in the frontend gives an benefit even when the backend does
not support
persistent grants. Is this still the case with this patch?
Christoph
---
From 1ede72ba10a7ec13d57ba6d2af54e86a099d7125 Mon Sep 17 00:00:00 2001
From: Roger Pau Monne roger@citrix.com
Date: Wed, 10 Jul 2013 10:22:19 +0200
Subject: [PATCH RFC] xen-blkfront: revoke foreign access for grants not
mapped by the backend
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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@citrix.com
Cc: Konrad Rzeszutek Wilk konrad.w...@oracle.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 3d445c0..6ba88c1 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1022,13 +1022,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
