* 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. Thanks, Liam
