On 7 May 2026 12:56:11 GMT-04:00, Ackerley Tng <[email protected]> wrote:
>"Liam R. Howlett" <[email protected]> writes:
>
>> On 26/04/28 04:25PM, Ackerley Tng via B4 Relay wrote:
>>>
>>> [...snip...]
>>>
>>> +/*
>>> + * Preallocate memory for attributes to be stored on a maple tree, pointed
>>> to
>>> + * by mas. Adjacent ranges with attributes identical to the new attributes
>>> + * will be merged. Also sets mas's bounds up for storing attributes.
>>> + *
>>> + * This maintains the invariant that ranges with the same attributes will
>>> + * always be merged.
>>> + */
>>> +static int kvm_gmem_mas_preallocate(struct ma_state *mas, u64 attributes,
>>> + pgoff_t start, size_t nr_pages)
>>> +{
>>> + pgoff_t end = start + nr_pages;
>>> + pgoff_t last = end - 1;
>>> + void *entry;
>>> +
>>> + /* Try extending range. entry is NULL on overflow/wrap-around. */
>>> + mas_set_range(mas, end, end);
>>> + entry = mas_find(mas, end);
>
>Thank you for your reviews!
>
>>
>> Please read the documentation as I believe you have a bug here. What
>> happens if there is another range stored higher than end + 1?
>>
>
>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?
>> 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 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.
>>> + 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...]
>>>
