Re: [External] Re: [PATCH v10 03/11] mm/hugetlb: Free the vmemmap pages associated with each HugeTLB page

[Mike Kravetz]

On 12/21/20 10:00 AM, Oscar Salvador wrote:
> On Mon, Dec 21, 2020 at 11:52:30PM +0800, Muchun Song wrote:
>> On Mon, Dec 21, 2020 at 9:44 PM Oscar Salvador <osalvador@xxxxxxx> wrote:
>>>
>>> On Mon, Dec 21, 2020 at 07:25:15PM +0800, Muchun Song wrote:
>>>
>>>> Should we add a BUG_ON in vmemmap_remap_free() for now?
>>>>
>>>>         BUG_ON(reuse != start + PAGE_SIZE);
>>>
>>> I do not think we have to, plus we would be BUG_ing for some specific use
>>> case in "generic" function.
>>
>> The vmemmap_remap_range() walks page table range [start, end),
>> if reuse is equal to (start + PAGE_SIZE), the range can adjust to
>> [start - PAGE_SIZE, end). But if not, we need some work to
>> implement the "generic" function.
>>
>>   - adjust range to [min(start, reuse), end) and call
>>     vmemmap_remap_rangeand which skip the hole
>>     which is [reuse + PAGE_SIZE, start) or [end, reuse).
>>   - call vmemmap_remap_range(reuse, reuse + PAGE_SIZE)
>>     to get the reuse page.Then, call vmemmap_remap_range(start, end)
>>     again to remap.
>>
>> Which one do you prefer?
> 
> I would not overcomplicate things at this stage.
> Just follow my sugestion and add a BUG_ON as you said, that might be the
> easier way now.
> We can overthink this in the future when some other usecases come
> around, right?

I too like the suggestion of specifying the reuse address.  It is better
than than relying on 'start + PAGE_SIZE' or even 'start - PAGE_SIZE' as
in the previous version.

However, if we do allow this then we can not allow just any reuse address
without complicating the code.  Why?  Because the code would also need to
do a page table walk to validate reuse addr.  In the current code, that is
handled as long as reuse address is part of the range we are walking.

I see two assumptions in the current code:
1) reuse address is part of the range
2) remap_pte is found 'first' in table walk before we start remapping.

In the current use case, the 'reuse addr' is always going to be the start
of the page table range we walk.  Correct?  If so, perhaps it would just
be simpler for now to have range be
[reuse addr, last page mapped to reuse addr].  IOW, always have the range
start with reuse addr and all subsequent pages in the range are mapped to
reuse addr.  I know it is not very generic or flexible.  But, it might be
easier to understand than the adjustments (+- PAGE_SIZE) currently being
made in the code.

Just a thought.
-- 
Mike Kravetz