On Tue, Oct 15, 2024 at 10:32:41PM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@xxxxxxxxx> [241015 22:18]: >> On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote: >> >* Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [241015 20:42]: >> >> * Wei Yang <richard.weiyang@xxxxxxxxx> [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 -- Wei Yang Help you, Help me
