Re: [RFC PATCH 2/8] hugetlb: recompute min_count when dropping hugetlb_lock

[Mike Kravetz <mike.kravetz@oracle.com>]

On 3/23/21 1:14 AM, Michal Hocko wrote:
> On Tue 23-03-21 09:01:02, Peter Zijlstra wrote:
>> On Tue, Mar 23, 2021 at 08:50:53AM +0100, Michal Hocko wrote:
>>
>>>>>> +static inline unsigned long min_hp_count(struct hstate *h, unsigned long count)
>>>>>> +{
>>>>>> +	unsigned long min_count;
>>>>>> +
>>>>>> +	min_count = h->resv_huge_pages + h->nr_huge_pages - h->free_huge_pages;
>>>>>> +	return max(count, min_count);
>>>>>
>>>>> Just out of curiousity, is compiler allowed to inline this piece of code
>>>>> and then cache the value? In other words do we need to make these
>>>>> READ_ONCE or otherwise enforce the no-caching behavior?
>>>>
>>>> I honestly do not know if the compiler is allowed to do that.  The
>>>> assembly code generated by my compiler does not cache the value, but
>>>> that does not guarantee anything.  I can add READ_ONCE to make the
>>>> function look something like:
>>>>
>>>> static inline unsigned long min_hp_count(struct hstate *h, unsigned long count)
>>>> {
>>>> 	unsigned long min_count;
>>>>
>>>> 	min_count = READ_ONCE(h->resv_huge_pages) + READ_ONCE(h->nr_huge_pages)
>>>> 					- READ_ONCE(h->free_huge_pages);
>>>> 	return max(count, min_count);
>>>> }
>>>
>>> Maybe just forcing to never inline the function should be sufficient.
>>> This is not a hot path to micro optimize for no function call. But there
>>> are much more qualified people on the CC list on this matter who could
>>> clarify. Peter?
>>
>> I'm not sure I understand the code right.
> 
> We need to ensure the function is evaluated each time it is called
> because it will be used after a lock is dropped and reacquired so
> numbers could have changed. The point of wrapping this into a function
> is to reduce the code duplication IIUC.
> 
>> But inline or not doesn't
>> matter, LTO completely ruins that game. Just like if it was a static
>> function, then the compiler is free to inline it, even if the function
>> lacks an inline attribute.
> 
> OK
> 
>> Basically, without READ_ONCE() the compiler is allowed to entirely elide
>> the load (and use a previous load), or to duplicate the load and do it
>> again later (reaching a different result).
>>
>> Similarly, the compiler is allowed to byte-wise load the variable in any
>> random order and re-assemble.
>>
>> If any of that is a problem, you have to use READ_ONCE().
> 
> Thanks for the confirmation!
> 

Here is another thought.
In patch 5 you suggest removing all pages from hugetlb with the lock
held, and adding them to a list.  Then, drop the lock and free all
pages on the list.  If we do this, then the value computed here (min_count)
can not change while we are looping.  So, this patch would be unnecessary.
That is another argument in favor of batching the frees.

Unless there is something wrong in my thinking, I am going to take that
approach and drop this patch.
-- 
Mike Kravetz