"Liam R. Howlett" <[email protected]> writes: > > [...snip...] > >> >>The invariant in this maple tree is that contiguous ranges with the same >>attribute are stored as a single range. >> >>The goal of this first part is to get the entry at the index just after >>the requested range, and see what the attribute there is. If that >>attribute is what we're about to set, extend the requested range for >>storing to the end of that range. >> >>If there is another range higher than end + 1, with the invariant >>maintained, that attribute has to be different than the attribute stored >>at end. Hence, we only want to extend this requested range up till end. >> > > mas_find() will look for an entry at the given address for the first search, > and if it is not found it will continue to search upwards. Since you limit > the search to end, it will work as you want and there isn't a bug as I was > thinking in my sleep deprived state. > > Since you are searching for exactly one address (end), it might serve you > better to walk there. Maybe walking is a better API for what you are doing > here? >
Thanks again for this tip! I'll try the walk API in the next revision after v6 [1] [1] https://lore.kernel.org/all/[email protected]/T/ > >>> Do you have testing of these functions somewhere? >>> >> >>GMEM_CONVERSION_MULTIPAGE_TEST_INIT_SHARED(indexing, 4) tests setting >>attributes in ranges. If test_page is 2, >> >>1. [0, 4) starts off shared (4 is the number of pages in the guest_memfd) >>2. [2, 3) is converted to private >> => so the ranges should now be [0, 2), [2, 3), [3, 4) >>3. [2, 3) is converted back to shared >> => so the ranges should now be [0, 4) >> >>I verified this by inserting some trace_printk()s and inspecting manually. >> > > Thanks. I find the exclusive ranges a bit odd to think about in the maple > tree context, but this test case makes sense. This is especially odd to look > at a single index entry, at least for me. > > I generally have a set of test cases and append any bug reproduces to that > list so they are unlikely to reoccur. My testing is certainly different from > what you'll be doing, but this method has done well with the quality of code > improving over time, and limited (if any) regressions. > I've not worked directly with the maple tree tests but the xarray tests (similarly set up, I believe) are a joy to work with. > I actually insist that any fix has a test before I accept them. There are > two reasons for this: 1. Avoiding the regression. 2. People really understand > the bug if they can create a reproducer. > > I hope this helps. > > The maple tree tests are set up to directly test maple tree code, but KVM selftests test from the userspace interface, and it's hard to test this invariant from userspace. >>>> + if (entry && xa_to_value(entry) == attributes) >>>> + last = mas->last; >>>> + >>>> + if (start > 0) { >>>> + mas_set_range(mas, start - 1, start - 1); >>>> + entry = mas_find(mas, start - 1); >>>> + if (entry && xa_to_value(entry) == attributes) >>>> + start = mas->index; >>>> + } >>>> + >>>> + mas_set_range(mas, start, last); >>>> + return mas_preallocate(mas, xa_mk_value(attributes), GFP_KERNEL); >>>> +} >>>> + >>>> >>>> [...snip...] >>>>
