| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 9 Dec 2021 10:21:22 +0200
From: Aharon Landau <aharonl@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>, Leon Romanovsky <leon@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH rdma-next 2/7] RDMA/mlx5: Replace cache list with Xarray
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
Powered by blists - more mailing lists