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

[Suren Baghdasaryan]

On Fri, Feb 14, 2025 at 8:27 AM Vlastimil Babka <vbabka@xxxxxxx> 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@xxxxxxx>

Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

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