Re: [PATCH RFC v2 07/10] slab: determine barn status racily outside of lock

[Harry Yoo <harry.yoo@oracle.com>]

On Fri, Feb 14, 2025 at 05:27:43PM +0100, Vlastimil Babka wrote:
> The possibility of many barn operations is determined by the current
> number of full or empty sheaves. Taking the barn->lock just to find out
> that e.g. there are no empty sheaves results in unnecessary overhead and
> lock contention. Thus perform these checks outside of the lock with a
> data_race() annotated variable read and fail quickly without taking the
> lock.
> 
> Checks for sheaf availability that racily succeed have to be obviously
> repeated under the lock for correctness, but we can skip repeating
> checks if there are too many sheaves on the given list as the limits
> don't need to be strict.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

in kmem_cache_return_sheaf:
> if (!pcs->spare) {                                                      
> 	pcs->spare = sheaf;                                             
>	sheaf = NULL;                                                   
> } else if (pcs->barn->nr_full >= MAX_FULL_SHEAVES) {                    
>	/* racy check */                                                
>	barn = pcs->barn;                                               
>	keep = true;                                                    
> }  

By the way this code also needs data_race()?

-- 
Cheers,
Harry

> ---
>  mm/slub.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index c1df7cf22267f28f743404531bef921e25fac086..72e6437f1d74bfacbb1cd7642af42929c48cc66a 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2685,9 +2685,12 @@ static struct slab_sheaf *barn_get_empty_sheaf(struct node_barn *barn)
>  	struct slab_sheaf *empty = NULL;
>  	unsigned long flags;
>  
> +	if (!data_race(barn->nr_empty))
> +		return NULL;
> +
>  	spin_lock_irqsave(&barn->lock, flags);
>  
> -	if (barn->nr_empty) {
> +	if (likely(barn->nr_empty)) {
>  		empty = list_first_entry(&barn->sheaves_empty,
>  					 struct slab_sheaf, barn_list);
>  		list_del(&empty->barn_list);
> @@ -2703,38 +2706,36 @@ static int barn_put_empty_sheaf(struct node_barn *barn,
>  				struct slab_sheaf *sheaf, bool ignore_limit)
>  {
>  	unsigned long flags;
> -	int ret = 0;
> +
> +	/* we don't repeat the check under barn->lock as it's not critical */
> +	if (!ignore_limit && data_race(barn->nr_empty) >= MAX_EMPTY_SHEAVES)
> +		return -E2BIG;
>  
>  	spin_lock_irqsave(&barn->lock, flags);
>  
> -	if (!ignore_limit && barn->nr_empty >= MAX_EMPTY_SHEAVES) {
> -		ret = -E2BIG;
> -	} else {
> -		list_add(&sheaf->barn_list, &barn->sheaves_empty);
> -		barn->nr_empty++;
> -	}
> +	list_add(&sheaf->barn_list, &barn->sheaves_empty);
> +	barn->nr_empty++;
>  
>  	spin_unlock_irqrestore(&barn->lock, flags);
> -	return ret;
> +	return 0;
>  }
>  
>  static int barn_put_full_sheaf(struct node_barn *barn, struct slab_sheaf *sheaf,
>  			       bool ignore_limit)
>  {
>  	unsigned long flags;
> -	int ret = 0;
> +
> +	/* we don't repeat the check under barn->lock as it's not critical */
> +	if (!ignore_limit && data_race(barn->nr_full) >= MAX_FULL_SHEAVES)
> +		return -E2BIG;
>  
>  	spin_lock_irqsave(&barn->lock, flags);
>  
> -	if (!ignore_limit && barn->nr_full >= MAX_FULL_SHEAVES) {
> -		ret = -E2BIG;
> -	} else {
> -		list_add(&sheaf->barn_list, &barn->sheaves_full);
> -		barn->nr_full++;
> -	}
> +	list_add(&sheaf->barn_list, &barn->sheaves_full);
> +	barn->nr_full++;
>  
>  	spin_unlock_irqrestore(&barn->lock, flags);
> -	return ret;
> +	return 0;
>  }
>  
>  static struct slab_sheaf *barn_get_full_or_empty_sheaf(struct node_barn *barn)
> @@ -2742,6 +2743,9 @@ static struct slab_sheaf *barn_get_full_or_empty_sheaf(struct node_barn *barn)
>  	struct slab_sheaf *sheaf = NULL;
>  	unsigned long flags;
>  
> +	if (!data_race(barn->nr_full) && !data_race(barn->nr_empty))
> +		return NULL;
> +
>  	spin_lock_irqsave(&barn->lock, flags);
>  
>  	if (barn->nr_full) {
> @@ -2772,9 +2776,12 @@ barn_replace_empty_sheaf(struct node_barn *barn, struct slab_sheaf *empty)
>  	struct slab_sheaf *full = NULL;
>  	unsigned long flags;
>  
> +	if (!data_race(barn->nr_full))
> +		return NULL;
> +
>  	spin_lock_irqsave(&barn->lock, flags);
>  
> -	if (barn->nr_full) {
> +	if (likely(barn->nr_full)) {
>  		full = list_first_entry(&barn->sheaves_full, struct slab_sheaf,
>  					barn_list);
>  		list_del(&full->barn_list);
> @@ -2797,19 +2804,23 @@ barn_replace_full_sheaf(struct node_barn *barn, struct slab_sheaf *full)
>  	struct slab_sheaf *empty;
>  	unsigned long flags;
>  
> +	/* we don't repeat this check under barn->lock as it's not critical */
> +	if (data_race(barn->nr_full) >= MAX_FULL_SHEAVES)
> +		return ERR_PTR(-E2BIG);
> +	if (!data_race(barn->nr_empty))
> +		return ERR_PTR(-ENOMEM);
> +
>  	spin_lock_irqsave(&barn->lock, flags);
>  
> -	if (barn->nr_full >= MAX_FULL_SHEAVES) {
> -		empty = ERR_PTR(-E2BIG);
> -	} else if (!barn->nr_empty) {
> -		empty = ERR_PTR(-ENOMEM);
> -	} else {
> +	if (likely(barn->nr_empty)) {
>  		empty = list_first_entry(&barn->sheaves_empty, struct slab_sheaf,
>  					 barn_list);
>  		list_del(&empty->barn_list);
>  		list_add(&full->barn_list, &barn->sheaves_full);
>  		barn->nr_empty--;
>  		barn->nr_full++;
> +	} else {
> +		empty = ERR_PTR(-ENOMEM);
>  	}
>  
>  	spin_unlock_irqrestore(&barn->lock, flags);
> 
> -- 
> 2.48.1
>