On 3/23/21 1:14 AM, Michal Hocko wrote:

On Tue 23-03-21 09:01:02, Peter Zijlstra wrote:

quoted

On Tue, Mar 23, 2021 at 08:50:53AM +0100, Michal Hocko wrote:

quoted

quoted

quoted

quoted

+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.

quoted

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

quoted

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