| Message ID | 20241015233909.23592-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix mas_new_root() | expand |
* Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: > When overwriting the whole range with NULL, current behavior is not correct. > This is really strange. You have changed the code to be wrong then removed it.. The second patch removes what you changed in the first. It doesn't look right today but what you have done is also not right. > Wei Yang (2): > maple_tree: not necessary to check index/last again > maple_tree: one single entry couldn't represent the whole range > > lib/maple_tree.c | 9 --------- > 1 file changed, 9 deletions(-) > > -- > 2.34.1 > >
* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]: > * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: > > When overwriting the whole range with NULL, current behavior is not correct. > > > > This is really strange. You have changed the code to be wrong then > removed it.. The second patch removes what you changed in the first. > > It doesn't look right today but what you have done is also not right. Looking at this again, the code that you have changed is correct. I actually think the bug is the other way around. If we are represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we don't need a node to store that, and shouldn't. It's also not really a bug, but a missed optimisation. The ranges are stored correctly, we just use too much memory in one case. The dump isn't clear, but since we merge NULL entries, if there is a 0-0 -> NULL and 1-ULONG_MAX => NULL, then they will be one and the same. You could change the dump code as part of your fix. It's like the init of a tree (tree->ma_root = NULL). Please don't submit multiple patches to fix the same thing like this, it makes it look like you are trying to pad your patch count. I'm guessing you did this to keep them logically separate, but when you completely drop the entire block of code that was changed in the second patch it becomes a bit much (and hard to follow, I was trying to figure out what branch you were working off because it didn't look like the patch would apply to my branch). Please submit a testcase with any suspected bugs. If it is not possible to do the fix first, then do them at the same time. I often write the fix for a bug, then recreate the bug in a testcase and ensure that it fails without my fix. I am not sure the fixes tag is correct in the patch either, since so much has changed around this. You could test the older code to see once you write a testcase. But the bug is using a node to store 0-ULONG_MAX => NULL. > > > > Wei Yang (2): > > maple_tree: not necessary to check index/last again > > maple_tree: one single entry couldn't represent the whole range > > > > lib/maple_tree.c | 9 --------- > > 1 file changed, 9 deletions(-) > > > > -- > > 2.34.1 > > > >
On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote: >* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]: >> * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: >> > When overwriting the whole range with NULL, current behavior is not correct. >> > >> >> This is really strange. You have changed the code to be wrong then >> removed it.. The second patch removes what you changed in the first. >> >> It doesn't look right today but what you have done is also not right. > >Looking at this again, the code that you have changed is correct. > >I actually think the bug is the other way around. If we are >represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we >don't need a node to store that, and shouldn't. > >It's also not really a bug, but a missed optimisation. The ranges are >stored correctly, we just use too much memory in one case. > >The dump isn't clear, but since we merge NULL entries, if there is a 0-0 >-> NULL and 1-ULONG_MAX => NULL, then they will be one and the same. >You could change the dump code as part of your fix. > >It's like the init of a tree (tree->ma_root = NULL). Agree with your above statement, this depends how we want to handle this. The change here is to make the behavior consistent. Want to confirm with you: the fix in this patch is fine with your, right? > >Please don't submit multiple patches to fix the same thing like this, it >makes it look like you are trying to pad your patch count. I'm guessing >you did this to keep them logically separate, but when you completely >drop the entire block of code that was changed in the second patch it >becomes a bit much (and hard to follow, I was trying to figure out what >branch you were working off because it didn't look like the patch would >apply to my branch). Sure, will merge it. > >Please submit a testcase with any suspected bugs. If it is not possible >to do the fix first, then do them at the same time. I often write the >fix for a bug, then recreate the bug in a testcase and ensure that it >fails without my fix. > Since user won't detect the difference, so a case to see whether the root is a node looks good to you? >I am not sure the fixes tag is correct in the patch either, since so >much has changed around this. You could test the older code to see once >you write a testcase. But the bug is using a node to store 0-ULONG_MAX >=> NULL. > So I should drop the fix tag? >> >> >> > Wei Yang (2): >> > maple_tree: not necessary to check index/last again >> > maple_tree: one single entry couldn't represent the whole range >> > >> > lib/maple_tree.c | 9 --------- >> > 1 file changed, 9 deletions(-) >> > >> > -- >> > 2.34.1 >> > >> >
* Wei Yang <richard.weiyang@gmail.com> [241015 22:18]: > On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote: > >* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]: > >> * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: > >> > When overwriting the whole range with NULL, current behavior is not correct. > >> > > >> > >> This is really strange. You have changed the code to be wrong then > >> removed it.. The second patch removes what you changed in the first. > >> > >> It doesn't look right today but what you have done is also not right. > > > >Looking at this again, the code that you have changed is correct. > > > >I actually think the bug is the other way around. If we are > >represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we > >don't need a node to store that, and shouldn't. > > > >It's also not really a bug, but a missed optimisation. The ranges are > >stored correctly, we just use too much memory in one case. > > > >The dump isn't clear, but since we merge NULL entries, if there is a 0-0 > >-> NULL and 1-ULONG_MAX => NULL, then they will be one and the same. > >You could change the dump code as part of your fix. > > > >It's like the init of a tree (tree->ma_root = NULL). > > Agree with your above statement, this depends how we want to handle this. The > change here is to make the behavior consistent. > > Want to confirm with you: the fix in this patch is fine with your, right? No, it's not fine. You are removing an optimisation. The issue is the other side of things where a node is used to store a single range from 0 to ULONX_MAX pointing to NULL in a 256B node. And, potentially, the debug dump of the tree should be more clear. > > > > >Please don't submit multiple patches to fix the same thing like this, it > >makes it look like you are trying to pad your patch count. I'm guessing > >you did this to keep them logically separate, but when you completely > >drop the entire block of code that was changed in the second patch it > >becomes a bit much (and hard to follow, I was trying to figure out what > >branch you were working off because it didn't look like the patch would > >apply to my branch). > > Sure, will merge it. Merge changes like this in the future, but the second patch in this series is wrong. > > > > >Please submit a testcase with any suspected bugs. If it is not possible > >to do the fix first, then do them at the same time. I often write the > >fix for a bug, then recreate the bug in a testcase and ensure that it > >fails without my fix. > > > > Since user won't detect the difference, so a case to see whether the root is a > node looks good to you? Write a test to find out if the resulting 0 - ULONG_MAX store of NULL results in a node. If it is in a node, then the test should fail. > > >I am not sure the fixes tag is correct in the patch either, since so > >much has changed around this. You could test the older code to see once > >you write a testcase. But the bug is using a node to store 0-ULONG_MAX > >=> NULL. > > > > So I should drop the fix tag? Yes, it's not a bug/problem - it's just inefficient use of space when someone tries to store 0 - ULONG_MAX, so there really isn't a reason to backport. Thanks, Liam
On Tue, Oct 15, 2024 at 10:32:41PM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [241015 22:18]: >> On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote: >> >* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]: >> >> * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: >> >> > When overwriting the whole range with NULL, current behavior is not correct. >> >> > >> >> >> >> This is really strange. You have changed the code to be wrong then >> >> removed it.. The second patch removes what you changed in the first. >> >> >> >> It doesn't look right today but what you have done is also not right. >> > >> >Looking at this again, the code that you have changed is correct. >> > >> >I actually think the bug is the other way around. If we are >> >represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we >> >don't need a node to store that, and shouldn't. >> > >> >It's also not really a bug, but a missed optimisation. The ranges are >> >stored correctly, we just use too much memory in one case. >> > >> >The dump isn't clear, but since we merge NULL entries, if there is a 0-0 >> >-> NULL and 1-ULONG_MAX => NULL, then they will be one and the same. >> >You could change the dump code as part of your fix. >> > >> >It's like the init of a tree (tree->ma_root = NULL). >> >> Agree with your above statement, this depends how we want to handle this. The >> change here is to make the behavior consistent. >> >> Want to confirm with you: the fix in this patch is fine with your, right? > >No, it's not fine. You are removing an optimisation. The issue is the >other side of things where a node is used to store a single range from 0 >to ULONX_MAX pointing to NULL in a 256B node. > >And, potentially, the debug dump of the tree should be more clear. > >> >> > >> >Please don't submit multiple patches to fix the same thing like this, it >> >makes it look like you are trying to pad your patch count. I'm guessing >> >you did this to keep them logically separate, but when you completely >> >drop the entire block of code that was changed in the second patch it >> >becomes a bit much (and hard to follow, I was trying to figure out what >> >branch you were working off because it didn't look like the patch would >> >apply to my branch). >> >> Sure, will merge it. > >Merge changes like this in the future, but the second patch in this >series is wrong. > >> >> > >> >Please submit a testcase with any suspected bugs. If it is not possible >> >to do the fix first, then do them at the same time. I often write the >> >fix for a bug, then recreate the bug in a testcase and ensure that it >> >fails without my fix. >> > >> >> Since user won't detect the difference, so a case to see whether the root is a >> node looks good to you? > >Write a test to find out if the resulting 0 - ULONG_MAX store of NULL >results in a node. If it is in a node, then the test should fail. > >> >> >I am not sure the fixes tag is correct in the patch either, since so >> >much has changed around this. You could test the older code to see once >> >you write a testcase. But the bug is using a node to store 0-ULONG_MAX >> >=> NULL. >> > >> >> So I should drop the fix tag? > >Yes, it's not a bug/problem - it's just inefficient use of space when >someone tries to store 0 - ULONG_MAX, so there really isn't a reason to >backport. > Ok, I see your preference. So current behavior of this is not expected, right? mas_set_range(mas, 0, ULONG_MAX) mas_store(mas, NULL) This operation to an empty tree will create a node now. Looks like a problem? >Thanks, >Liam
On Tue, Oct 15, 2024 at 10:32:41PM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [241015 22:18]: >> On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote: >> >* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]: >> >> * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]: >> >> > When overwriting the whole range with NULL, current behavior is not correct. >> >> > >> >> >> >> This is really strange. You have changed the code to be wrong then >> >> removed it.. The second patch removes what you changed in the first. >> >> >> >> It doesn't look right today but what you have done is also not right. >> > >> >Looking at this again, the code that you have changed is correct. >> > >> >I actually think the bug is the other way around. If we are >> >represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we >> >don't need a node to store that, and shouldn't. >> > >> >It's also not really a bug, but a missed optimisation. The ranges are >> >stored correctly, we just use too much memory in one case. >> > >> >The dump isn't clear, but since we merge NULL entries, if there is a 0-0 >> >-> NULL and 1-ULONG_MAX => NULL, then they will be one and the same. >> >You could change the dump code as part of your fix. >> > >> >It's like the init of a tree (tree->ma_root = NULL). >> >> Agree with your above statement, this depends how we want to handle this. The >> change here is to make the behavior consistent. >> >> Want to confirm with you: the fix in this patch is fine with your, right? > >No, it's not fine. You are removing an optimisation. The issue is the >other side of things where a node is used to store a single range from 0 >to ULONX_MAX pointing to NULL in a 256B node. > >And, potentially, the debug dump of the tree should be more clear. > >> >> > >> >Please don't submit multiple patches to fix the same thing like this, it >> >makes it look like you are trying to pad your patch count. I'm guessing >> >you did this to keep them logically separate, but when you completely >> >drop the entire block of code that was changed in the second patch it >> >becomes a bit much (and hard to follow, I was trying to figure out what >> >branch you were working off because it didn't look like the patch would >> >apply to my branch). >> >> Sure, will merge it. > >Merge changes like this in the future, but the second patch in this >series is wrong. > >> >> > >> >Please submit a testcase with any suspected bugs. If it is not possible >> >to do the fix first, then do them at the same time. I often write the >> >fix for a bug, then recreate the bug in a testcase and ensure that it >> >fails without my fix. >> > >> >> Since user won't detect the difference, so a case to see whether the root is a >> node looks good to you? > >Write a test to find out if the resulting 0 - ULONG_MAX store of NULL >results in a node. If it is in a node, then the test should fail. > Want to confirm: Store [0, ULONG_MAX] of NULL should result an empty tree or an single entry represent [0, 0] as current mas_new_root()? >> >> >I am not sure the fixes tag is correct in the patch either, since so >> >much has changed around this. You could test the older code to see once >> >you write a testcase. But the bug is using a node to store 0-ULONG_MAX >> >=> NULL. >> > >> >> So I should drop the fix tag? > >Yes, it's not a bug/problem - it's just inefficient use of space when >someone tries to store 0 - ULONG_MAX, so there really isn't a reason to >backport. > >Thanks, >Liam