Re: [PATCH rdma-next 2/7] RDMA/mlx5: Replace cache list with Xarray

[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