Re: [PATCH 1/1] mm: implement page refcount locking via dedicated bit

["David Hildenbrand (Arm)" <david@kernel.org>]

This all made mu brain hurt a little :)

>  
>  /**
> @@ -176,6 +181,9 @@ static inline int page_ref_sub_and_test(struct page *page, int nr)
>  {
>  	int ret = atomic_sub_and_test(nr, &page->_refcount);
>  
> +	if (ret)
> +		ret = !atomic_cmpxchg_relaxed(&page->_refcount, 0, PAGEREF_LOCKED_BIT);
> +

It took me a while to figure out why this can't be just an atomic_or():
even though concurrent page_ref_add_unless_zero() would see a 0 after
incrementing it to 1 to back off, there could be yet another concurrent
page_ref_add_unless_zero() that would see the transition from 1 to 2 and
continue.

What is the performance impact on doing the additional
atomic_cmpxchg_relaxed() whenever we free a page, in particular, for
anonymous memory where we mostly have just a single reference that we
drop during munmap() etc?

>  	if (page_ref_tracepoint_active(page_ref_mod_and_test))
>  		__page_ref_mod_and_test(page, -nr, ret);
>  	return ret;
> @@ -204,6 +212,9 @@ static inline int page_ref_dec_and_test(struct page *page)
>  {
>  	int ret = atomic_dec_and_test(&page->_refcount);
>  
> +	if (ret)
> +		ret = !atomic_cmpxchg_relaxed(&page->_refcount, 0, PAGEREF_LOCKED_BIT);
> +
>  	if (page_ref_tracepoint_active(page_ref_mod_and_test))
>  		__page_ref_mod_and_test(page, -1, ret);
>  	return ret;
> @@ -228,14 +239,23 @@ static inline int folio_ref_dec_return(struct folio *folio)
>  	return page_ref_dec_return(&folio->page);
>  }
>  
> +#define _PAGEREF_LOCKED_LIMIT	((1 << 30) | PAGEREF_LOCKED_BIT)
> +
>  static inline bool page_ref_add_unless_zero(struct page *page, int nr)
>  {
>  	bool ret = false;
> +	int val;
>  
>  	rcu_read_lock();
>  	/* avoid writing to the vmemmap area being remapped */
> -	if (page_count_writable(page))
> -		ret = atomic_add_unless(&page->_refcount, nr, 0);
> +	if (page_count_writable(page)) {
> +		val = atomic_add_return(nr, &page->_refcount);
> +		ret = !(val & PAGEREF_LOCKED_BIT);
> +
> +		/* Undo atomic_add() if counter is locked and scary big */
> +		while (unlikely((unsigned int)val >= _PAGEREF_LOCKED_LIMIT))
> +			val = atomic_cmpxchg_relaxed(&page->_refcount, val, PAGEREF_LOCKED_BIT);

I assume we can't do an atomic_dec(), because we might have concurrent
unfreezing (or similar things) happening that overwrote whatever was in
there.

Is it really correct to replace _PAGEREF_LOCKED_LIMIT by
PAGEREF_LOCKED_BIT, dropping some unrelated references? I assume the
reasoning is that we treat any references with PAGEREF_LOCKED_BIT set as
irrelevant and can get overwritten any time.


I was wondering is whether page_ref_freeze() could actually leave the
references set, and only set the PAGEREF_LOCKED_BIT bit, whereby
page_ref_unfreeze() would only clear the PAGEREF_LOCKED_BIT bit.

Similarly, the set_page_refcounted() could add a reference and clear the
PAGEREF_LOCKED_BIT. That'd be more expensive on the allocation path ...
and not sure if that would really help to turn this
atomic_cmpxchg_relaxed() into an simpler atomic_dec() my brain could
more easily understand :)


I think this patch needs a lot more documentation around what the
PAGEREF_LOCKED_BIT means, and how this interacts with e.g., the
set_page_count() in set_page_refcounted().

In general, I like this!

-- 
Cheers,

David