| Message ID | 20240626160631.3636515-2-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] maple_tree: fix alloc node fail issue | expand |
On Wed, Jun 26, 2024 at 12:06:31PM -0400, Liam R. Howlett wrote: >From: Jiazi Li <jqqlijiazi@gmail.com> > >Add some maple_tree alloc node tese case. > >Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com> >Signed-off-by: Jiazi Li <jqqlijiazi@gmail.com> >Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> >--- > tools/testing/radix-tree/maple.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > >diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c >index 11f1efdf83f9..b4b5fd9f294d 100644 >--- a/tools/testing/radix-tree/maple.c >+++ b/tools/testing/radix-tree/maple.c >@@ -462,6 +462,28 @@ static noinline void __init check_new_node(struct maple_tree *mt) > MT_BUG_ON(mt, mas_allocated(&mas) != 10 + MAPLE_ALLOC_SLOTS - 1); > mas_destroy(&mas); > >+ mas.node = MA_ERROR(-ENOMEM); >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS + 1); /* Request */ >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ I am not sure why mas_nomem() is here. Without this one, we still can trigger the original bug. >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS + 1); >+ mas.node = MA_ERROR(-ENOMEM); >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 2); /* Request */ >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >+ mas.status = ma_start; >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 2); >+ mas_destroy(&mas); >+ >+ mas.node = MA_ERROR(-ENOMEM); >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 1); /* Request */ >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 1); >+ mas.node = MA_ERROR(-ENOMEM); >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 3 + 2); /* Request */ >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >+ mas.status = ma_start; >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 3 + 2); >+ mas_destroy(&mas); >+ > mtree_unlock(mt); > } > >-- >2.43.0 >
* Wei Yang <richard.weiyang@gmail.com> [241010 21:18]: > On Wed, Jun 26, 2024 at 12:06:31PM -0400, Liam R. Howlett wrote: > >From: Jiazi Li <jqqlijiazi@gmail.com> > > > >Add some maple_tree alloc node tese case. > > > >Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com> > >Signed-off-by: Jiazi Li <jqqlijiazi@gmail.com> > >Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > >--- > > tools/testing/radix-tree/maple.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > >diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c > >index 11f1efdf83f9..b4b5fd9f294d 100644 > >--- a/tools/testing/radix-tree/maple.c > >+++ b/tools/testing/radix-tree/maple.c > >@@ -462,6 +462,28 @@ static noinline void __init check_new_node(struct maple_tree *mt) > > MT_BUG_ON(mt, mas_allocated(&mas) != 10 + MAPLE_ALLOC_SLOTS - 1); > > mas_destroy(&mas); > > > >+ mas.node = MA_ERROR(-ENOMEM); > >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS + 1); /* Request */ > >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ > > I am not sure why mas_nomem() is here. > > Without this one, we still can trigger the original bug. It will fill the maple state allocation. Might not be needed but doesn't hurt. > > >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS + 1); > >+ mas.node = MA_ERROR(-ENOMEM); > >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 2); /* Request */ > >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ > >+ mas.status = ma_start; > >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 2); > >+ mas_destroy(&mas); > >+ > >+ mas.node = MA_ERROR(-ENOMEM); > >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 1); /* Request */ > >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ > >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 1); > >+ mas.node = MA_ERROR(-ENOMEM); > >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 3 + 2); /* Request */ > >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ > >+ mas.status = ma_start; > >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 3 + 2); > >+ mas_destroy(&mas); > >+ > > mtree_unlock(mt); > > } > > > >-- > >2.43.0 > > > > -- > Wei Yang > Help you, Help me
On Mon, Oct 14, 2024 at 09:15:07PM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [241010 21:18]: >> On Wed, Jun 26, 2024 at 12:06:31PM -0400, Liam R. Howlett wrote: >> >From: Jiazi Li <jqqlijiazi@gmail.com> >> > >> >Add some maple_tree alloc node tese case. >> > >> >Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com> >> >Signed-off-by: Jiazi Li <jqqlijiazi@gmail.com> >> >Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> >> >--- >> > tools/testing/radix-tree/maple.c | 22 ++++++++++++++++++++++ >> > 1 file changed, 22 insertions(+) >> > >> >diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c >> >index 11f1efdf83f9..b4b5fd9f294d 100644 >> >--- a/tools/testing/radix-tree/maple.c >> >+++ b/tools/testing/radix-tree/maple.c >> >@@ -462,6 +462,28 @@ static noinline void __init check_new_node(struct maple_tree *mt) >> > MT_BUG_ON(mt, mas_allocated(&mas) != 10 + MAPLE_ALLOC_SLOTS - 1); >> > mas_destroy(&mas); >> > >> >+ mas.node = MA_ERROR(-ENOMEM); >> >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS + 1); /* Request */ >> >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >> >> I am not sure why mas_nomem() is here. >> >> Without this one, we still can trigger the original bug. > >It will fill the maple state allocation. Might not be needed but >doesn't hurt. > I took another look at it. We really need mas_nomem() here, since we call mt_set_non_kernel(0) at the beginning of check_new_node(). So mas_node_count() just set the request count and mas_nomem() does the real allocation. Any reason to design test case like this? >> >> >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS + 1); >> >+ mas.node = MA_ERROR(-ENOMEM); >> >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 2); /* Request */ >> >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >> >+ mas.status = ma_start; >> >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 2); >> >+ mas_destroy(&mas); >> >+ >> >+ mas.node = MA_ERROR(-ENOMEM); >> >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 1); /* Request */ >> >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >> >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 1); >> >+ mas.node = MA_ERROR(-ENOMEM); >> >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 3 + 2); /* Request */ >> >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ >> >+ mas.status = ma_start; >> >+ MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 3 + 2); >> >+ mas_destroy(&mas); >> >+ >> > mtree_unlock(mt); >> > } >> > >> >-- >> >2.43.0 >> > >> >> -- >> Wei Yang >> Help you, Help me
* Wei Yang <richard.weiyang@gmail.com> [241015 09:31]: > On Mon, Oct 14, 2024 at 09:15:07PM -0400, Liam R. Howlett wrote: > >* Wei Yang <richard.weiyang@gmail.com> [241010 21:18]: > >> On Wed, Jun 26, 2024 at 12:06:31PM -0400, Liam R. Howlett wrote: > >> >From: Jiazi Li <jqqlijiazi@gmail.com> > >> > > >> >Add some maple_tree alloc node tese case. > >> > > >> >Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com> > >> >Signed-off-by: Jiazi Li <jqqlijiazi@gmail.com> > >> >Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com> > >> >--- > >> > tools/testing/radix-tree/maple.c | 22 ++++++++++++++++++++++ > >> > 1 file changed, 22 insertions(+) > >> > > >> >diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c > >> >index 11f1efdf83f9..b4b5fd9f294d 100644 > >> >--- a/tools/testing/radix-tree/maple.c > >> >+++ b/tools/testing/radix-tree/maple.c > >> >@@ -462,6 +462,28 @@ static noinline void __init check_new_node(struct maple_tree *mt) > >> > MT_BUG_ON(mt, mas_allocated(&mas) != 10 + MAPLE_ALLOC_SLOTS - 1); > >> > mas_destroy(&mas); > >> > > >> >+ mas.node = MA_ERROR(-ENOMEM); > >> >+ mas_node_count(&mas, MAPLE_ALLOC_SLOTS + 1); /* Request */ > >> >+ mas_nomem(&mas, GFP_KERNEL); /* Fill request */ > >> > >> I am not sure why mas_nomem() is here. > >> > >> Without this one, we still can trigger the original bug. > > > >It will fill the maple state allocation. Might not be needed but > >doesn't hurt. > > > > I took another look at it. We really need mas_nomem() here, since we call > mt_set_non_kernel(0) at the beginning of check_new_node(). > > So mas_node_count() just set the request count and mas_nomem() does the real > allocation. > > Any reason to design test case like this? There are two types of testing that we do: ones that can call internal functions to poke at stuff (like this one), and ones that can't which is more like what users can do (lives in lib/test_maple_tree.c). Both are important as this checks what we think will happen while the other tests the integration of all our work together. As for this particular test, the two parts work together most of the time, but if we run out of memory then we need to test them in parts to make sure both are doing what we think. It also means that we can easily set up tests for scenarios that are harder to synthesize using the external api - but they can happen. I'm not sure if this is the case for this test, but there were tests already setting the allocations into certain positions and this is now just another one of those tests. I hope this helps explain some more of the testing work here as it is extremely important. I'm very pleased to have this setup to recreate issues for both fixing issues and ensuring they do not return - especially on large refactoring, new features or algorithms. Thanks, Liam
diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c index 11f1efdf83f9..b4b5fd9f294d 100644 --- a/tools/testing/radix-tree/maple.c +++ b/tools/testing/radix-tree/maple.c @@ -462,6 +462,28 @@ static noinline void __init check_new_node(struct maple_tree *mt) MT_BUG_ON(mt, mas_allocated(&mas) != 10 + MAPLE_ALLOC_SLOTS - 1); mas_destroy(&mas); + mas.node = MA_ERROR(-ENOMEM); + mas_node_count(&mas, MAPLE_ALLOC_SLOTS + 1); /* Request */ + mas_nomem(&mas, GFP_KERNEL); /* Fill request */ + MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS + 1); + mas.node = MA_ERROR(-ENOMEM); + mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 2); /* Request */ + mas_nomem(&mas, GFP_KERNEL); /* Fill request */ + mas.status = ma_start; + MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 2); + mas_destroy(&mas); + + mas.node = MA_ERROR(-ENOMEM); + mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 2 + 1); /* Request */ + mas_nomem(&mas, GFP_KERNEL); /* Fill request */ + MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 2 + 1); + mas.node = MA_ERROR(-ENOMEM); + mas_node_count(&mas, MAPLE_ALLOC_SLOTS * 3 + 2); /* Request */ + mas_nomem(&mas, GFP_KERNEL); /* Fill request */ + mas.status = ma_start; + MT_BUG_ON(mt, mas_allocated(&mas) != MAPLE_ALLOC_SLOTS * 3 + 2); + mas_destroy(&mas); + mtree_unlock(mt); }