Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1342166 > unrolled thread

Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-02-24 17:20 +0100
    Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-02-25 14:40 +0100
      Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-02-25 15:30 +0100

#1342166 — Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller

On Wed, Feb 24, 2016 at 6:43 PM, Haggai Eran <haggaie@mellanox.com> wrote:
> Hi,
>
> Overall I the patch looks good to me. I have a few comments below.
>
Thanks for the review. Addressing most comments one.
Some comments inline.


> Its -> It's
Ok.

>> +void rdmacg_query_limit(struct rdmacg_device *device,
>> +                     int *limits, int max_count);
> You can drop the max_count parameter, and require the caller to
> always provide pool_info->table_len items, couldn't you?
>
Done.

>> +       can result into resource unavailibility to other consumers.
> unavailibility -> unavailability
Done.

>> +     struct rdmacg_resource_pool *rpool;
>> +     struct rdmacg_pool_info *pool_info = &device->pool_info;
>> +
>> +     spin_lock(&cg->rpool_list_lock);
>> +     rpool = find_cg_rpool_locked(cg, device);
> Is it possible for rpool to be NULL?
>
Unlikely, unless we have but in cgroup implementation.
It may be worth to add WARN_ON and return from here to avoid kernel crash.

>> +static int charge_cg_resource(struct rdma_cgroup *cg,
>> +                           struct rdmacg_device *device,
>> +                           int index, int num)
>> +{
>> +     struct rdmacg_resource_pool *rpool;
>> +     s64 new;
>> +     int ret = 0;
>> +
>> +retry:
>> +     spin_lock(&cg->rpool_list_lock);
>> +     rpool = find_cg_rpool_locked(cg, device);
>> +     if (!rpool) {
>> +             spin_unlock(&cg->rpool_list_lock);
>> +             ret = alloc_cg_rpool(cg, device);
>> +             if (ret)
>> +                     goto err;
>> +             else
>> +                     goto retry;
> Instead of retrying after allocation of a new rpool, why not just return the
> newly allocated rpool (or the existing one) from alloc_cg_rpool?

It can be done, but locking semantics just becomes difficult to
review/maintain with that where alloc_cg_rpool will unlock and lock
conditionally later on.
This path will be hit anyway on first allocation typically. Once
application is warm up, it will be unlikely to enter here.
I should change if(!rpool) to if (unlikely(!rpool)).


>
>> +     }
>> +     new = num + rpool->resources[index].usage;
>> +     if (new > rpool->resources[index].max) {
>> +             ret = -EAGAIN;
>> +     } else {
>> +             rpool->refcnt++;
>> +             rpool->resources[index].usage = new;
>> +     }
>> +     spin_unlock(&cg->rpool_list_lock);
>> +err:
>> +     return ret;
>> +}
>
>> +static ssize_t rdmacg_resource_set_max(struct kernfs_open_file *of,
>> +                                    char *buf, size_t nbytes, loff_t off)
>> +{
>> +     struct rdma_cgroup *cg = css_rdmacg(of_css(of));
>> +     const char *dev_name;
>> +     struct rdmacg_resource_pool *rpool;
>> +     struct rdmacg_device *device;
>> +     char *options = strstrip(buf);
>> +     struct rdmacg_pool_info *pool_info;
>> +     u64 enables = 0;
> This limits the number of resources to 64. Sounds fine to me, but I think
> there should be a check somewhere (maybe in rdmacg_register_device()?) to
> make sure someone doesn't pass too many resources.
Right. Such check is in place in rdmacg_register_device which return
EINVAL when more than 64 resources are requested.

>> +     spin_lock(&cg->rpool_list_lock);
>> +     rpool = find_cg_rpool_locked(cg, device);
>> +     if (!rpool) {
>> +             spin_unlock(&cg->rpool_list_lock);
>> +             ret = alloc_cg_rpool(cg, device);
>> +             if (ret)
>> +                     goto opt_err;
>> +             else
>> +                     goto retry;
> You can avoid the retry here too. Perhaps this can go into a function.
>
In v5 I had wrapper around code which used to similar hiding using
get_cg_rpool and put_cg_rpool helper functions.
But Tejun was of opinion that I should have locks outside of all those
functions. With that approach, this is done.
So I think its ok. to have it this way.

>> +     }
>> +
>> +     /* now set the new limits of the rpool */
>> +     while (enables) {
>> +             /* if user set the limit, enables bit is set */
>> +             if (enables & BIT(i)) {
>> +                     enables &= ~BIT(i);
>> +                     set_resource_limit(rpool, i, new_limits[i]);
>> +             }
>> +             i++;
>> +     }
>> +     if (rpool->refcnt == 0 &&
>> +         rpool->num_max_cnt == pool_info->table_len) {
>> +             /*
>> +              * No user of the rpool and all entries are
>> +              * set to max, so safe to delete this rpool.
>> +              */
>> +             list_del(&rpool->cg_list);
>> +             spin_unlock(&cg->rpool_list_lock);
>> +             free_cg_rpool(rpool);
>> +     } else {
>> +             spin_unlock(&cg->rpool_list_lock);
>> +     }
> You should consider putting this piece of code in a function (the
> check of the reference counts and release of the rpool).
>
Yes. I did. Same as above comment. Also this function will have to
unlock. Its usually better to lock/unlock from same function level,
instead of locking at one level and unlocking from inside the
function.
Or
I should have
cg_rpool_cond_free_unlock() for above code (check of the reference
counts and release of the rpool)?

>> +static int print_rpool_values(struct seq_file *sf,
> This can return void.
Done.

[toc] | [next] | [standalone]

#1343162

On Thu, Feb 25, 2016 at 5:33 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>>>> +retry:
>>>> +     spin_lock(&cg->rpool_list_lock);
>>>> +     rpool = find_cg_rpool_locked(cg, device);
>>>> +     if (!rpool) {
>>>> +             spin_unlock(&cg->rpool_list_lock);
>>>> +             ret = alloc_cg_rpool(cg, device);
>>>> +             if (ret)
>>>> +                     goto err;
>>>> +             else
>>>> +                     goto retry;
>>> Instead of retrying after allocation of a new rpool, why not just return the
>>> newly allocated rpool (or the existing one) from alloc_cg_rpool?
>>
>> It can be done, but locking semantics just becomes difficult to
>> review/maintain with that where alloc_cg_rpool will unlock and lock
>> conditionally later on.
> Maybe I'm missing something, but couldn't you simply lock rpool_list_lock
> inside alloc_cg_rpool()? It already does that around its call to
> find_cg_rpool_locked() and the insertion to cg_list.

No. ref_count and usage counters are updated at level where lock is
taken in charge_cg_resource().
If I move locking rpool_list_lock inside alloc_cg_rpool, unlocking
will continue outside, alloc_cg_rpool() when its found or allocated.
As you acknowledged in below comment that this makes confusing to
lock/unlock from different context, I think current implementation
achieves both.
(a) take lock from single context
(b) keep functionality of find and alloc in two separate individual functions

>
>> This path will be hit anyway on first allocation typically. Once
>> application is warm up, it will be unlikely to enter here.
>> I should change if(!rpool) to if (unlikely(!rpool)).
> Theoretically the new allocated rpool can be released again by the time you
> get to the second call to find_cg_rpool_locked().
>
Thats ok, because if that occurs find_cg_rpool_locked() won't find the
entry and will try to allocate again.
Things work fine in that case.

> I thought that was about functions that only locked the lock, called the
> find function, and released the lock. What I'm suggesting is to have one
> function that does "lock + find + allocate if needed + unlock",

I had similar function in past which does,
"lock + find + allocate if needed + + inc_ref_cnt + unlock", (get_cg_rpool)
update usage_counter atomically, because other thread/process might update too.
check atomic_dec_cnt - on reaching zero, "lock + del_entry + unlock + free".

Tejun asked to simplify this to,

"lock + find + allocate if needed + inc_ref_cnt_without_atomic" + unlock".
which I did in this patch v6.

> and another
> function that does (under caller's lock) "check ref count + check max count +
> release rpool".
This can be done. Have one dumb basic question for thiat.
Can we call kfree() with spin_lock held? All these years I tend to
avoid doing so.

[toc] | [prev] | [next] | [standalone]

#1343195

> Can we call kfree() with spin_lock held? All these years I tend to
> avoid doing so.
Also it doesn't look correct to hold the lock while freeing the memory
which is totally unrelated to the lock.
With that I think current code appears ok with exception that its
duplicated at two place for code readability around lock.
What say?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web