Messages in this thread | | | Date | Thu, 9 Dec 2021 10:21:22 +0200 | Subject | Re: [PATCH rdma-next 2/7] RDMA/mlx5: Replace cache list with Xarray | From | Aharon Landau <> |
| |
On 12/8/2021 6:46 PM, Jason Gunthorpe wrote: >> @@ -166,14 +169,14 @@ static void create_mkey_callback(int status, struct mlx5_async_work *context) >> >> WRITE_ONCE(dev->cache.last_add, jiffies); >> >> - spin_lock_irqsave(&ent->lock, flags); >> - list_add_tail(&mr->list, &ent->head); >> - ent->available_mrs++; >> + xa_lock_irqsave(&ent->mkeys, flags); >> + xa_ent = __xa_store(&ent->mkeys, ent->stored++, mr, GFP_ATOMIC); >> + WARN_ON(xa_ent != NULL); >> + ent->pending--; >> ent->total_mrs++; >> /* If we are doing fill_to_high_water then keep going. */ >> queue_adjust_cache_locked(ent); >> - ent->pending--; >> - spin_unlock_irqrestore(&ent->lock, flags); >> + xa_unlock_irqrestore(&ent->mkeys, flags); >> } >> >> static struct mlx5_ib_mr *alloc_cache_mr(struct mlx5_cache_ent *ent, void *mkc) >> @@ -196,6 +199,25 @@ static struct mlx5_ib_mr *alloc_cache_mr(struct mlx5_cache_ent *ent, void *mkc) >> return mr; >> } >> >> +static int _push_reserve_mkey(struct mlx5_cache_ent *ent) >> +{ >> + unsigned long to_reserve; >> + int rc; >> + >> + while (true) { >> + to_reserve = ent->reserved; >> + rc = xa_err(__xa_cmpxchg(&ent->mkeys, to_reserve, NULL, >> + XA_ZERO_ENTRY, GFP_KERNEL)); >> + if (rc) >> + return rc; > What about when old != NULL ? > >> + if (to_reserve != ent->reserved) >> + continue; > There is an edge case where where reserved could have shrunk alot > while the lock was released, and xa_cmpxchg could succeed. The above > if will protect things, however a ZERO_ENTRY will have been written to > some weird place in the XA. It needs a > > if (old == NULL) // ie we stored something someplace weird > __xa_erase(&ent->mkeys, to_reserve) > >> + ent->reserved++; >> + break; >> + } >> + return 0; >> +} >> + >> /* Asynchronously schedule new MRs to be populated in the cache. */ >> static int add_keys(struct mlx5_cache_ent *ent, unsigned int num) >> { >> @@ -217,23 +239,32 @@ static int add_keys(struct mlx5_cache_ent *ent, unsigned int num) >> err = -ENOMEM; >> break; >> } >> - spin_lock_irq(&ent->lock); >> + xa_lock_irq(&ent->mkeys); >> if (ent->pending >= MAX_PENDING_REG_MR) { >> + xa_unlock_irq(&ent->mkeys); >> err = -EAGAIN; >> - spin_unlock_irq(&ent->lock); >> + kfree(mr); >> + break; >> + } >> + >> + err = _push_reserve_mkey(ent); > The test of ent->pending is out of date now since this can drop the > lock > > It feels like pending and (reserved - stored) are really the same > thing, so maybe just directly limit the number of reserved and test it > after The mlx5_ib_dereg_mr is reserving entries as well. Should I limit create_mkey_cb due to pending deregs? >> @@ -287,39 +318,37 @@ static void remove_cache_mr_locked(struct mlx5_cache_ent *ent) >> { >> struct mlx5_ib_mr *mr; >> >> - lockdep_assert_held(&ent->lock); >> - if (list_empty(&ent->head)) >> + if (!ent->stored) >> return; >> - mr = list_first_entry(&ent->head, struct mlx5_ib_mr, list); >> - list_del(&mr->list); >> - ent->available_mrs--; >> + mr = __xa_store(&ent->mkeys, --ent->stored, NULL, GFP_KERNEL); >> + WARN_ON(mr == NULL || xa_is_err(mr)); > Add a if (reserved != stored) before the below? I initiated the xarray using XA_FLAGS_ALLOC, therefore, the __xa_store above will mark the entry as ZERO_ENTRY. > >> + WARN_ON(__xa_erase(&ent->mkeys, --ent->reserved) != NULL); > Also please avoid writing WARN_ON(something with side effects) > > old = __xa_erase(&ent->mkeys, --ent->reserved); > WARN_ON(old != NULL); > > Same for all places > >> static int resize_available_mrs(struct mlx5_cache_ent *ent, unsigned int target, >> bool limit_fill) >> + __acquires(&ent->lock) __releases(&ent->lock) > Why? > >> { >> int err; >> >> - lockdep_assert_held(&ent->lock); >> - > Why? > >> static void clean_keys(struct mlx5_ib_dev *dev, int c) >> { >> struct mlx5_mr_cache *cache = &dev->cache; >> struct mlx5_cache_ent *ent = &cache->ent[c]; >> - struct mlx5_ib_mr *tmp_mr; >> struct mlx5_ib_mr *mr; >> - LIST_HEAD(del_list); >> + unsigned long index; >> >> cancel_delayed_work(&ent->dwork); >> - while (1) { >> - spin_lock_irq(&ent->lock); >> - if (list_empty(&ent->head)) { >> - spin_unlock_irq(&ent->lock); >> - break; >> - } >> - mr = list_first_entry(&ent->head, struct mlx5_ib_mr, list); >> - list_move(&mr->list, &del_list); >> - ent->available_mrs--; >> + xa_for_each(&ent->mkeys, index, mr) { > This isn't quite the same thing, the above tolerates concurrent add, > this does not. > > It should be more like > > while (ent->stored) { > mr = __xa_erase(stored--); > >> @@ -1886,6 +1901,17 @@ mlx5_free_priv_descs(struct mlx5_ib_mr *mr) >> } >> } >> >> +static int push_reserve_mkey(struct mlx5_cache_ent *ent) >> +{ >> + int ret; >> + >> + xa_lock_irq(&ent->mkeys); >> + ret = _push_reserve_mkey(ent); >> + xa_unlock_irq(&ent->mkeys); >> + >> + return ret; >> +} > Put this close to _push_reserve_mkey() please > > Jason
| |