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

Groups > linux.kernel > #1350866 > unrolled thread

Re: [PATCHv9 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: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-03-05 12:20 +0100
    Re: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller Tejun Heo <tj@kernel.org> - 2016-03-05 14:00 +0100
      Re: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-03-05 18:30 +0100
        Re: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-03-12 07:20 +0100
          Re: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller Tejun Heo <tj@kernel.org> - 2016-03-16 21:50 +0100

#1350866 — Re: [PATCHv9 1/3] rdmacg: Added rdma cgroup controller

Hi Tejun,

I would like to submit patch v10.
Can you please confirm that you are ok (or not) with the current
design and below changes should be good enough?
I am ok if you directly want to jump to review v10 too.

Changes from v9:
  * Included documentation of resources in v2.txt and v1.txt
  * Fixed issue of race condition of process migration during charging stage.
  * Fixed comments and code to adhere to CodingStyle.
  * Simplified and removed support to charge/uncharge multiple resource.
  * Fixed removed refcnt with usage_num that tracks how many
    resources are unused to trigger freeing the object.
  * Removed inline for query_limit and other function as its not necessary.

Design that remains same from v6 to v10.
  * spin lock is still fine grained at cgroup level instead of one
global shared lock among all cgroups.
     In future it can be optimized further to do per cpu or using
single lock if required.
  * file type enums are still present for max and current, as
read/write call to those files is already taken care by common
functions with required if/else.
  * Resource limit setting is as it is, because number of devices are
in range of 1 to 4 count in most use cases (as explained in
documentation), and its not hot path.

Parav



On Thu, Mar 3, 2016 at 8:19 AM, Parav Pandit <pandit.parav@gmail.com> wrote:
> Hi Tejun, Haggai,
>
> On Thu, Mar 3, 2016 at 1:28 AM, Parav Pandit <pandit.parav@gmail.com> wrote:
>>>> +     rpool->refcnt--;
>>>> +     if (rpool->refcnt == 0 && rpool->num_max_cnt == pool_info->table_len) {
>>>
>>> If the caller charges 2 and then uncharges 1 two times, the refcnt
>>> underflows?  Why not just track how many usages are zero?
>>>
>> This is certainly must fix bug. Changed refcnt to usage_sum and changed to do
>> usage_sum -= num during uncharging
>> and
>> usage_sum += num during charing.
>
> This is not sufficient as css_get() and put are done only once per
> call, which leads to similar problem as of refcnt.
> As I think more, I realised that this particular test is missing that
> resulted in this related bug, I realize that we don't have use case to
> have "num" field from the IB stack side.
> For bulk free IB stack will have to keep track of different or same
> rdmacg returned values to call uncharge() with right number of
> resources, all of that complexity just doesn't make sense and not
> required.
> So as first step to further simplify this, I am removing "num" input
> field from charge and uncharge API.

[toc] | [next] | [standalone]

#1350896

Hello, Parav.

On Sat, Mar 05, 2016 at 04:45:09PM +0530, Parav Pandit wrote:
> Design that remains same from v6 to v10.
>   * spin lock is still fine grained at cgroup level instead of one
> global shared lock among all cgroups.
>      In future it can be optimized further to do per cpu or using
> single lock if required.
>   * file type enums are still present for max and current, as
> read/write call to those files is already taken care by common
> functions with required if/else.
>   * Resource limit setting is as it is, because number of devices are
> in range of 1 to 4 count in most use cases (as explained in
> documentation), and its not hot path.

1 and 2 are not okay.  3 is fine but resource [un]charging is not hot
path?

Thanks.

-- 
tejun

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

#1350947

Hi Tejun,

On Sat, Mar 5, 2016 at 6:22 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Parav.
>
> On Sat, Mar 05, 2016 at 04:45:09PM +0530, Parav Pandit wrote:
>> Design that remains same from v6 to v10.
>>   * spin lock is still fine grained at cgroup level instead of one
>> global shared lock among all cgroups.
>>      In future it can be optimized further to do per cpu or using
>> single lock if required.
>>   * file type enums are still present for max and current, as
>> read/write call to those files is already taken care by common
>> functions with required if/else.
>>   * Resource limit setting is as it is, because number of devices are
>> in range of 1 to 4 count in most use cases (as explained in
>> documentation), and its not hot path.
>
> 1 and 2 are not okay.
For (1) shall I have one spin lock that is uses across multiple
hierarchy and multiple cgroup.
Essentially one global lock among all cgroup. During hierarchical
charging, continue to use same lock it at each level.
Would that work in this first release?

Can you please review the code for (2), I cannot think of any further
helper functions that I can write.
For both the file types, all the code is already common.
file types are used only to find out whether to reference max variable
or usage variable in structure.
Which can also be made as array, but I do not want to lose the code
readability for that little gain.
What exactly is the issue in current implementation? You just
mentioned that "its not good sign".
Its readable, simple and serves the purpose, what am I missing?

> 3 is fine but resource [un]charging is not hot path?
charge/uncharge is hot path from cgroup perspective.
Considering 1 to 4 devices in system rpool list would grow upto 4
entry deep at each cgroup level.
I believe this is good enough to start with. O complexity wise its
O(N). where N is number of devices in system.


>
> Thanks.
>
> --
> tejun

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

#1356357

Hi Tejun,

On Sat, Mar 5, 2016 at 10:50 PM, Parav Pandit <pandit.parav@gmail.com> wrote:
> Hi Tejun,
>
> On Sat, Mar 5, 2016 at 6:22 PM, Tejun Heo <tj@kernel.org> wrote:
>> Hello, Parav.
>>
>> On Sat, Mar 05, 2016 at 04:45:09PM +0530, Parav Pandit wrote:
>>> Design that remains same from v6 to v10.
>>>   * spin lock is still fine grained at cgroup level instead of one
>>> global shared lock among all cgroups.
>>>      In future it can be optimized further to do per cpu or using
>>> single lock if required.
>>>   * file type enums are still present for max and current, as
>>> read/write call to those files is already taken care by common
>>> functions with required if/else.
>>>   * Resource limit setting is as it is, because number of devices are
>>> in range of 1 to 4 count in most use cases (as explained in
>>> documentation), and its not hot path.
>>
>> 1 and 2 are not okay.
> For (1) shall I have one spin lock that is uses across multiple
> hierarchy and multiple cgroup.
> Essentially one global lock among all cgroup. During hierarchical
> charging, continue to use same lock it at each level.
> Would that work in this first release?
>

I am waiting for your reply.
Shall one lock for all cgroup is ok with you?

> Can you please review the code for (2), I cannot think of any further
> helper functions that I can write.
> For both the file types, all the code is already common.
> file types are used only to find out whether to reference max variable
> or usage variable in structure.
> Which can also be made as array, but I do not want to lose the code
> readability for that little gain.
> What exactly is the issue in current implementation? You just
> mentioned that "its not good sign".
> Its readable, simple and serves the purpose, what am I missing?
>
If this is ok. I will keep the code as it is, because it uses common
helper functions for max and current files.


>> 3 is fine but resource [un]charging is not hot path?
> charge/uncharge is hot path from cgroup perspective.
> Considering 1 to 4 devices in system rpool list would grow upto 4
> entry deep at each cgroup level.
> I believe this is good enough to start with. O complexity wise its
> O(N). where N is number of devices in system.
>
>
>>
>> Thanks.
>>
>> --
>> tejun

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

#1359355

Hello, Parav.

Sorry about the delay.

On Sat, Mar 12, 2016 at 11:49:03AM +0530, Parav Pandit wrote:
> > For (1) shall I have one spin lock that is uses across multiple
> > hierarchy and multiple cgroup.
> > Essentially one global lock among all cgroup. During hierarchical
> > charging, continue to use same lock it at each level.
> > Would that work in this first release?
> 
> I am waiting for your reply.
> Shall one lock for all cgroup is ok with you?

Yes, when you're locking up to the root each time, splitting locks at
the bottom doesn't really achieve anything.  It just makes things more
expensive.

> If this is ok. I will keep the code as it is, because it uses common
> helper functions for max and current files.

Hmmm... can you please try to refactor the common part to helpers?
It's not a big thing but there were both styles across different
controllers and helper based ones tend to be easier to follow.

> >> 3 is fine but resource [un]charging is not hot path?
> > charge/uncharge is hot path from cgroup perspective.
> > Considering 1 to 4 devices in system rpool list would grow upto 4
> > entry deep at each cgroup level.
> > I believe this is good enough to start with. O complexity wise its
> > O(N). where N is number of devices in system.

I see, but if that's the case, please drop the fine locking.  The fine
locking doesn't make much sense - as implemented it's slower and the
whole thing is not hot.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web