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

[Parav Pandit]

Hi Christoph,

Reopening thread for discussion so that I can proceed to generate next patch.

Recap:
rdma cgroup controller in the patch defines the framework so that RDMA
subsystem can define the resources.
This is similar to at least two functionality provided by core kernel.

(a) Block elevator defining framework and provides way to loadable
kernel modules to define actual IO scheduler (out of the 3 or 4) that
can be plugged-in since last many years.
(b) VFS core defining framework for external loadable modules to load
file system.

None of these core kernel functionality actually filter based on enum
or names - which IO scheduler(s), file system(s) are
allowed/disallowed.
rdma cgroup controller and rdma subsystem follows similar design approach.

Therefore can I go ahead with current approach?

Tejun also mentioned that he likes to see rdma resources to be defined
by rdma cgroup rather than rdma subsystem in below thread primarily to
reduce the complexity.
https://lkml.org/lkml/2016/4/4/507

However submitted patch is fairly small for defining resource in rdma
subsystem (instead of kernel).
This also allows to provide fixes and features done in rdma subsystem
in field at much faster pace and avoids complexity around back-porting
on various OS and their kernel flavors.
Please let me know your views.

Regards,
Parav Pandit



On Tue, Apr 19, 2016 at 2:26 PM, Parav Pandit  wrote:
> Hi Christoph,
>
> I was on travel. Sorry for the late inline response and question.
>
> Parav
>
>
>
> On Tue, Apr 5, 2016 at 10:57 PM, Christoph Hellwig  wrote:
>> On Tue, Apr 05, 2016 at 05:55:26AM -0700, Parav Pandit wrote:
>>> Just because we add one more rdma resource, we need to ask someone to
>>> upgrade kernel?
>>
>> Yes.  Just like when you need any other core kernel resource.
>
> By architecture Linux kernel allows
> (a) plugin of any new block level IO scheduler as kernel module.
> This is much more fundamental resource or functionality than rdma resource.
> (b) plugin of any new file system as kernel module.
>
> Changing both in field and out of box can do be more harmful than
> defining new rdma resource.
>
> RDMA Resource definition by IB core module is very similar to above
> two functionality, where elevator and VFS provides basic support
> framework and so rdma cgroup controller here.
>
> So can you please help me understand - which resource did you compare
> against in your above comment for "core kernel resource"?
> I compared it with similar functionality, flexibility given by (a)
> block IO Scheduler and (b) VFS subsystem to implement them as kernel
> module.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christoph Hellwig]

On Tue, Apr 05, 2016 at 05:55:26AM -0700, Parav Pandit wrote:
> Just because we add one more rdma resource, we need to ask someone to
> upgrade kernel?

Yes.  Just like when you need any other core kernel resource.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

On Tue, Apr 05, 2016 at 05:55:26AM -0700, Parav Pandit wrote:
> Hi Christoph,
> 
> On Tue, Apr 5, 2016 at 5:42 AM, Christoph Hellwig  wrote:
> > On Tue, Apr 05, 2016 at 05:39:21AM -0700, Parav Pandit wrote:
> >> I am not really trying to address OFED issues here. I am sure you
> >> understand that if ib_core.ko kernel module is in-kernel module than,
> >> for all the fixes/enhancements that goes to it would require people to
> >> upgrade to newer kernel, instead of just modules upgrade. Such heavy
> >> weight upgrade slows down the adoption which i am trying to avoid here
> >> by placing knobs in right kernel module's hand.
> >
> > What a load of rubbish.  The Linux kernel is one program and should be
> > upgraded together.
> 
> Just because we add one more rdma resource, we need to ask someone to
> upgrade kernel?

It doesn't make sense. Kernel and modules are always coming together,
the attempts to mix kernel and modules from different versions can lead
to many interesting debug scenarios.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tejun Heo]

Hello, Parav.

On Tue, Apr 05, 2016 at 07:25:11AM -0700, Parav Pandit wrote:
> > As it is a fairly isolated domain, to certain extent, it could be okay
> > to let it go.  At the same time, I know that these enterprise things
> > tend to go completely wayward and am worried about individual drivers
> > going crazy with custom attributes in a non-sensical way.
> 
> If its crazy at driver level, I am sure it will be equally crazy for
> any end-user too. Therefore no user would use those crazy resources
> either.

So, the above paragraph simply isn't true.  It's not difficult to
twist things bit so that things work in a hackish and often horrible
way and we know this happens quite a bit, especially in enterprise
settings.

> Intent is certainly not for the individual drivers as we agreed in
> past.

You and I agreeing to that doesn't really matter all that much.

> IB stack maintainers would be reviewing new enum addition
> anyway, whether its verb or hw resource (unlikely).

Well, if the additions are unlikely...

> > In the original review message, I mentioned creating an interface
> > which creates the hierarchy of objects as necessary and returns the
> > target pool with lock held, can you please give it a shot?  Something
> > like the following.
> 
> o.k. I will attempt. Looks doable.
> I am on travel so it will take few more days for me to turn around
> with below approach with tested code.

So, if you go with single mutex, you don't really need get_and_lock
semantics, you can just call the get function with mutex held.  Please
do whichever looks best.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav Pandit]

On Tue, Apr 5, 2016 at 7:07 AM, Tejun Heo  wrote:
> Just one more thing.
>
> On Tue, Apr 05, 2016 at 10:01:07AM -0400, Tejun Heo wrote:
> ...
>> > pool_info in spin lock context, made me allocate memory to get all
>> > values upfront through allocation.
>> > Now that the lock is going away, I can do what you have described above.
>
> So, this might be okay depending on the use case but it often becomes
> painful to require sleeping context for freeing resources.  If you're
> certain that requiring sleeping context is okay for all paths, using a
> single mutex is fine but *usually* it isn't a great idea.
>
At present charge and uncharge are from sleeping context.

>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tejun Heo]

Just one more thing.

On Tue, Apr 05, 2016 at 10:01:07AM -0400, Tejun Heo wrote:
...
> > pool_info in spin lock context, made me allocate memory to get all
> > values upfront through allocation.
> > Now that the lock is going away, I can do what you have described above.

So, this might be okay depending on the use case but it often becomes
painful to require sleeping context for freeing resources.  If you're
certain that requiring sleeping context is okay for all paths, using a
single mutex is fine but *usually* it isn't a great idea.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tejun Heo]

Hello, Parav.

On Mon, Apr 04, 2016 at 07:22:38PM -0700, Parav Pandit wrote:
> > Is it actually customary to have rdma core module updated more
> > frequently separate from the kernel?  Out-of-tree modules being
> > updated separately happens quite a bit but this is an in-kernel
> > module, which usually is tightly coupled with the rest of the kernel.
> >
> Yes.
> rdma core module is compiled as kernel module.
> Its often updated for new features, fixes.
> So kernel version can be one but RDMA core module(s) get updated more
> frequently than kernel.

As it is a fairly isolated domain, to certain extent, it could be okay
to let it go.  At the same time, I know that these enterprise things
tend to go completely wayward and am worried about individual drivers
going crazy with custom attributes in a non-sensical way.  The
interface this patch is proposing easily allows that and that at the
cost of internal engineering flexibility.  I don't really want to be
caught up in a situation where we're forced to include broken usages
because that's what's already out in the wild.  I personally would
much prefer the resources to be defined rigidly.  Let's see how the
discussion with Christoph evolves.

> > I don't remember the details well but the code was vastly more complex
> > back then and the object lifetime management was muddy at best.  If I
> > reviewed in a contradicting way, my apologies, but given the current
> > code, it'd be better to have objects creation upfront.
> 
> Do you mean,
> try_charge() should
> lock()
> run loop to allocate in hierarchy, if not allocated.
> run loop again to charge.
> unlock()
> 
> If so, I prefer to run the loop once.

In the original review message, I mentioned creating an interface
which creates the hierarchy of objects as necessary and returns the
target pool with lock held, can you please give it a shot?  Something
like the following.

pool *get_pool(...)
{
lock;
if (target pool exists)
return pool w/ lock held;

create the pool hierarchically (might involve unlock);
if (succeeded)
return pool w/ lock held;
return NULL w/o lock;
}

> > It isn't necessarily about speed.  It makes clear that the parent
> > always should exist and makes the code easier to read and update.
> 
> It doesn't have to exist. It can get allocated when charging occurs.
> Otherwise even if rdma resources are not used, it ends up allocating
> rpool in hierarchy. (We talked this before)

Sure, create pools only for the used combinations but do that
hierarchically so that a child pool always has a parent.  I can
promise you that the code will read a lot better with that.

> > I don't know why you end up missing basic patterns so badly.  It's
> > making the review and iteration process pretty painful.  Please don't
> > be confrontational and try to read the review comments assuming good
> > will.
> >
> My understanding of seq_printf() being blocking call and accessing

seq_printf() can be called from any context; otherwise, it would be a
horrible interface to use, wouldn't it?

> pool_info in spin lock context, made me allocate memory to get all
> values upfront through allocation.
> Now that the lock is going away, I can do what you have described above.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav Pandit]

Hi Christoph,

On Tue, Apr 5, 2016 at 5:42 AM, Christoph Hellwig  wrote:
> On Tue, Apr 05, 2016 at 05:39:21AM -0700, Parav Pandit wrote:
>> I am not really trying to address OFED issues here. I am sure you
>> understand that if ib_core.ko kernel module is in-kernel module than,
>> for all the fixes/enhancements that goes to it would require people to
>> upgrade to newer kernel, instead of just modules upgrade. Such heavy
>> weight upgrade slows down the adoption which i am trying to avoid here
>> by placing knobs in right kernel module's hand.
>
> What a load of rubbish.  The Linux kernel is one program and should be
> upgraded together.

Just because we add one more rdma resource, we need to ask someone to
upgrade kernel?

Flexibility is coming at very minimal cost, so what exactly is the
issue in having that instead of forcing architecture to give away
that? Whole code is hardly 700 odd lines.

>
>> Its like making ib_core.ko from module to in kernel component, if I
>> understand correctly nobody wants to do that.
>
> We allow splitting subsystems out where it's easily doable to avoid the
> resources consumption if they're all built in.  For cgroups it's not
> really practical, but that doesn't mean you can upgrade invidual parts
> of a complex program without a lot of precaution.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav Pandit]

Hi Christoph,

On Tue, Apr 5, 2016 at 2:06 AM, Christoph Hellwig  wrote:
> On Mon, Apr 04, 2016 at 09:25:04PM -0400, Tejun Heo wrote:
>> Is it actually customary to have rdma core module updated more
>> frequently separate from the kernel?  Out-of-tree modules being
>> updated separately happens quite a bit but this is an in-kernel
>> module, which usually is tightly coupled with the rest of the kernel.
>
> People do it for all the wrong reasons - OFED and organization of morons
> wants people to use their modules, which causes lots of harm.  Anything
> that makes this harder is a good thing.
>
I am not really trying to address OFED issues here. I am sure you
understand that if ib_core.ko kernel module is in-kernel module than,
for all the fixes/enhancements that goes to it would require people to
upgrade to newer kernel, instead of just modules upgrade. Such heavy
weight upgrade slows down the adoption which i am trying to avoid here
by placing knobs in right kernel module's hand.
Its like making ib_core.ko from module to in kernel component, if I
understand correctly nobody wants to do that.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav Pandit]

Hi Tejun,

On Mon, Apr 4, 2016 at 6:25 PM, Tejun Heo  wrote:
> Hello,
>
> On Mon, Apr 04, 2016 at 03:50:54PM -0700, Parav Pandit wrote:
>> 2nd advantage is, it allows avoiding lot of rework, in bundling kernel
>> modules with different kernel versions. So it is certainly valuable
>> feature with almost nil complexity cost of code or memory that solves
>> two problems with approach.
>> If there two kernel versions with different type of resources and
>> kernel modules with different resources, it gets ugly to maintain that
>> kind of compatibility using compat.h. This approach avoid all such
>> cases.
>
> Is it actually customary to have rdma core module updated more
> frequently separate from the kernel?  Out-of-tree modules being
> updated separately happens quite a bit but this is an in-kernel
> module, which usually is tightly coupled with the rest of the kernel.
>
Yes.
rdma core module is compiled as kernel module.
Its often updated for new features, fixes.
So kernel version can be one but RDMA core module(s) get updated more
frequently than kernel.

>
> I don't remember the details well but the code was vastly more complex
> back then and the object lifetime management was muddy at best.  If I
> reviewed in a contradicting way, my apologies, but given the current
> code, it'd be better to have objects creation upfront.

Do you mean,
try_charge() should
lock()
run loop to allocate in hierarchy, if not allocated.
run loop again to charge.
unlock()

If so, I prefer to run the loop once.

If you mean, objects creating upfront when cgroup is created, or when
device is created - than we have talked of that approach in past, that
its more complex than what is being done today. I would certainly want
to avoid that in this patch as above approach has two advantages.
(a) its simpler
(b) doesn't require extra parent pointer, which is already available from cgroup

>
>> > So, with each pool linking to its parent, instead of looking up at
>> > each level, it can just follow ->parent recursively.
>>
>> No. Lookup is being done for the rpool for that device and not the cgroup.
>> doing recursively using ->parent would require additional pointer of
>> rpool type to be maintained which is not necessary because we already
>> have cgroup level parent.
>
> Yes, I meant adding pool->parent field.  Hierarchical operations
> become far simpler with the invariant that parent always exists.

>
>> Adding ->parent would make it little faster compare to traversing 1 to
>> 4 node entry list. However this is not hot path, so keeping existing
>> code to traverse is more desirable than adding ->parent of rpool type,
>> which also saves memory for large number of cgroup instances.
>
> It isn't necessarily about speed.  It makes clear that the parent
> always should exist and makes the code easier to read and update.

It doesn't have to exist. It can get allocated when charging occurs.
Otherwise even if rdma resources are not used, it ends up allocating
rpool in hierarchy. (We talked this before)

>
>> >> + /*
>> >> +  * A negative count (or overflow) is invalid,
>> >> +  * it indicates a bug in the rdma controller.
>> >> +  */
>> >> + WARN_ON_ONCE(rpool->resources[index].usage < 0);
>> >> + rpool->usage_sum--;
>> >
>> > What guarantees that this count wouldn't overflow?  More on this
>> > later.
>>
>> Are you suggesting to add WARN_ON for usage_sum variable for debugging?
>> I believe 1st warning is sufficient?
>
> I mean that it isn't particularly difficult to overflow a s32 when
> adding up multiple usages into one.
Possible,but resources are not really in range of 2G range, at least
not any hardware that I am aware of.
But I can make it u64.

>
>> >> +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;
>> >> + int *new_limits;
>> >> + int i = 0, ret = 0;
>> >> +
>> >> + /* extract the device name first */
>> >> + dev_name = strsep(, " ");
>> >> + if (!dev_name) {
>> >> + ret = -EINVAL;
>> >> + goto err;
>> >> + }
>> >> +
>> >> + /* acquire lock to synchronize with hot plug devices */
>> >> + mutex_lock(_mutex);
>> >> +
>> >> + device = rdmacg_get_device_locked(dev_name);
>> >> + if (!device) {
>> >> + ret = -ENODEV;
>> >> + goto parse_err;
>> >> + }
>> >> +
>> >> + pool_info = >pool_info;
>> >> +
>> >> + new_limits = kcalloc(pool_info->table_len, sizeof(int), GFP_KERNEL);
>> >> + if (!new_limits) {
>> >> + ret = -ENOMEM;
>> >> + goto parse_err;
>> >> + }
>> 

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

[Tejun Heo]

Hello,

On Mon, Apr 04, 2016 at 03:50:54PM -0700, Parav Pandit wrote:
> 2nd advantage is, it allows avoiding lot of rework, in bundling kernel
> modules with different kernel versions. So it is certainly valuable
> feature with almost nil complexity cost of code or memory that solves
> two problems with approach.
> If there two kernel versions with different type of resources and
> kernel modules with different resources, it gets ugly to maintain that
> kind of compatibility using compat.h. This approach avoid all such
> cases.

Is it actually customary to have rdma core module updated more
frequently separate from the kernel?  Out-of-tree modules being
updated separately happens quite a bit but this is an in-kernel
module, which usually is tightly coupled with the rest of the kernel.

> > * It's usually a good idea to have hierarchical objects to be created
> >   all the way to the root when a leaf node is requested and link
> >   parent at each level.  That way, when a descendant object is looked
> >   up, the caller can always deref its ->parent pointer till it reaches
> >   the top.
> 
> This is interesting comment. I cannot recall, but I think its v4 or v5
> was exactly doing same.
> Allocating resource pool first in one loop and charging them in another.
> In event of failure, uncharging them in 3rd.
> And you gave comment that time is to move away from that approach for
> simplicity!
> That I had refcnt on each object.

I don't remember the details well but the code was vastly more complex
back then and the object lifetime management was muddy at best.  If I
reviewed in a contradicting way, my apologies, but given the current
code, it'd be better to have objects creation upfront.

> > So, with each pool linking to its parent, instead of looking up at
> > each level, it can just follow ->parent recursively.
> 
> No. Lookup is being done for the rpool for that device and not the cgroup.
> doing recursively using ->parent would require additional pointer of
> rpool type to be maintained which is not necessary because we already
> have cgroup level parent.

Yes, I meant adding pool->parent field.  Hierarchical operations
become far simpler with the invariant that parent always exists.

> Adding ->parent would make it little faster compare to traversing 1 to
> 4 node entry list. However this is not hot path, so keeping existing
> code to traverse is more desirable than adding ->parent of rpool type,
> which also saves memory for large number of cgroup instances.

It isn't necessarily about speed.  It makes clear that the parent
always should exist and makes the code easier to read and update.

> >> + /*
> >> +  * A negative count (or overflow) is invalid,
> >> +  * it indicates a bug in the rdma controller.
> >> +  */
> >> + WARN_ON_ONCE(rpool->resources[index].usage < 0);
> >> + rpool->usage_sum--;
> >
> > What guarantees that this count wouldn't overflow?  More on this
> > later.
> 
> Are you suggesting to add WARN_ON for usage_sum variable for debugging?
> I believe 1st warning is sufficient?

I mean that it isn't particularly difficult to overflow a s32 when
adding up multiple usages into one.

> >> +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;
> >> + int *new_limits;
> >> + int i = 0, ret = 0;
> >> +
> >> + /* extract the device name first */
> >> + dev_name = strsep(, " ");
> >> + if (!dev_name) {
> >> + ret = -EINVAL;
> >> + goto err;
> >> + }
> >> +
> >> + /* acquire lock to synchronize with hot plug devices */
> >> + mutex_lock(_mutex);
> >> +
> >> + device = rdmacg_get_device_locked(dev_name);
> >> + if (!device) {
> >> + ret = -ENODEV;
> >> + goto parse_err;
> >> + }
> >> +
> >> + pool_info = >pool_info;
> >> +
> >> + new_limits = kcalloc(pool_info->table_len, sizeof(int), GFP_KERNEL);
> >> + if (!new_limits) {
> >> + ret = -ENOMEM;
> >> + goto parse_err;
> >> + }
> >
> > Hmm... except for new_limits allocation and alloc_cg_rpool()
> > invocation, both of which can be done at the head of the function,
> > nothing seems to require a mutex here.  If we move them to the head of
> > the function, can we get rid of dev_mutex entirely?
> 
> No. As described in comment where lock is taken,
> it ensures that if device is being removed while configuration is
> going on, it protects against those race condition.
> So will keep it as it is.

Yeah, sure, it needs protection.  I was wondering why it needed to be
a separate mutex.  The only sleeping 

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

[Parav Pandit]

Hi Tejun,

On Mon, Apr 4, 2016 at 12:36 PM, Tejun Heo  wrote:
> Hello, Parav.
>
> Sorry about the delay.
>
Thanks for the review. Comments and answers inline.

>> +struct rdma_cgroup {
>> + struct cgroup_subsys_state  css;
>> +
>> + /* protects resource pool list */
>> + spinlock_t  rpool_list_lock;
>
> Is this lock still being used?
Nop. I should have cleaned this up, as we have the global lock. It
must got added in patch recreation time.
I will remove it.

>
>> + /*
>> +  * head to keep track of all resource pools
>> +  * that belongs to this cgroup.
>> +  */
>> + struct list_headrpool_head;
>> +};
>> +
>> +struct rdmacg_pool_info {
>> + const char  **resource_name_table;
>> + int table_len;
>
> It's a bit bothering that resource type defs are outside the
> controller in a way the controller can't know and assumed to have the
> same type and range.  Architecting kernel code so that allow building
> modules separately can modify behaviors usually isn't a priority and

Module cannot really change the behavior just by adding/removing
new/old resource type.
Behavior is defined by the controller as it does today.

> styling things like this makes implementing specific one-off behaviors
> cumbersome.
I agree that other controllers are not doing this way, for some good
reason in those controllers.
But its not right to impose the same restriction here.
Code is really straight forward where pool_info array length is
dynamic, instead of static structure definition.
By addressing some of the comments to change from spin lock to mutex,
will further reduce the allocation complexity.
I believe that should be sufficient.

We have discussed advantages this approach in previous patches along
with consensus from other rdma experts.
In v9 you asked to document them clearly in the Documentation
indicating that its alright to define by the module as its done in v9
and v10 patch.

>
> It now looks okay but let's say in the future someone wants to use a
> different type and/or ranges for one of the resources, given the above
> framework, that person is most likely to blow up pool_info - first
> with types and min, max values and maybe later with callback handlers
> and so on, when the only thing which would have been necessary is an
> one-off handling of that specific setting in the interface functions.
>
We can always go back to change the scope of rdma cgroup at that point
to restrict for what you have described above. At present thats not
the scope and we have reduced the requirements to what is implemented
today.

Additionally if the min,max setting might arrive for a resource in
future, than its likely that it would be applicable to multiple
resources than one resource. And in that case putting them inside the
pool_info would be appropriate as well.
rdma cgroup will have to define the new interface at that point anyway.
We have to let this interface be usable first with current coded
functionality and flexibility that is not harmful.

> Making things modular comes at the cost of complexity and ad-hoc
> flexiblity.

At this point, complexity of this architecture is not really much.
Only overheads are kmalloc and kfree are done on resource pool whose
size is decided dynamically instead of a value defined in header file
in rdma_cgroup.h.
(Instead its in ib_verbs.h)
Its fair enough to have that flexibility.

>  There are cases where such overhead is warranted but as I
> mentioned before, I don't think this is one of them.  Please look back
> on how this patchset developed.  It has been a continuous process of
> cutting down complexities which didn't need to be there which is the
> opposite of what we usually want to do - buliding up sophiscation and
> complexity as necessary from something simple.  Please cut down this
> part too.

Tejun,
To repeat the basic motivation of this functionality/architecture is:
kernel upgrades are done over period of years at customers location.
Upgrading kernel for few rdma resources is really silly.
Current implementation anyway doesn't let modules define any crazy count either.
Currently only 64 different types of resources can be defined anyway.
New resource definition anyway goes through reviews by maintainers.
So as we talked in past, I would like to continue with this approach.

2nd advantage is, it allows avoiding lot of rework, in bundling kernel
modules with different kernel versions. So it is certainly valuable
feature with almost nil complexity cost of code or memory that solves
two problems with approach.
If there two kernel versions with different type of resources and
kernel modules with different resources, it gets ugly to maintain that
kind of compatibility using compat.h. This approach avoid all such
cases.

>
>> +};
>> +
>> +struct rdmacg_device {
>> + struct rdmacg_pool_info pool_info;
>> + struct list_headrdmacg_list;
>> + struct list_head  

[PATCHv10 1/3] rdmacg: Added rdma cgroup controller

[Parav Pandit]

Added rdma cgroup controller that does accounting, limit enforcement
on rdma/IB verbs and hw resources.

Added rdma cgroup header file which defines its APIs to perform
charing/uncharing functionality and device registration which will
participate in controller functions of accounting and limit
enforcements. It also define rdmacg_device structure to bind IB stack
and RDMA cgroup controller.

RDMA resources are tracked using resource pool. Resource pool is per
device, per cgroup entity which allows setting up accounting limits
on per device basis.

Resources are not defined by the RDMA cgroup, instead they are defined
by the external module IB stack. This allows extending IB stack
without changing kernel, as IB stack is going through changes
and enhancements.

Resource pool is created/destroyed dynamically whenever
charging/uncharging occurs respectively and whenever user
configuration is done. Its a tradeoff of memory vs little more code
space that creates resource pool whenever necessary,
instead of creating them during cgroup creation and device registration
time.

Signed-off-by: Parav Pandit 
---
 include/linux/cgroup_rdma.h   |  52 +++
 include/linux/cgroup_subsys.h |   4 +
 init/Kconfig  |  10 +
 kernel/Makefile   |   1 +
 kernel/cgroup_rdma.c  | 746 ++
 5 files changed, 813 insertions(+)
 create mode 100644 include/linux/cgroup_rdma.h
 create mode 100644 kernel/cgroup_rdma.c

diff --git a/include/linux/cgroup_rdma.h b/include/linux/cgroup_rdma.h
new file mode 100644
index 000..baec8f0
--- /dev/null
+++ b/include/linux/cgroup_rdma.h
@@ -0,0 +1,52 @@
+#ifndef _CGROUP_RDMA_H
+#define _CGROUP_RDMA_H
+
+#include 
+
+#ifdef CONFIG_CGROUP_RDMA
+
+struct rdma_cgroup {
+   struct cgroup_subsys_state  css;
+
+   /* protects resource pool list */
+   spinlock_t  rpool_list_lock;
+   /*
+* head to keep track of all resource pools
+* that belongs to this cgroup.
+*/
+   struct list_headrpool_head;
+};
+
+struct rdmacg_pool_info {
+   const char  **resource_name_table;
+   int table_len;
+};
+
+struct rdmacg_device {
+   struct rdmacg_pool_info pool_info;
+   struct list_headrdmacg_list;
+   struct list_headrpool_head;
+   /* protects resource pool list */
+   spinlock_t  rpool_lock;
+   char*name;
+};
+
+/*
+ * APIs for RDMA/IB stack to publish when a device wants to
+ * participate in resource accounting
+ */
+int rdmacg_register_device(struct rdmacg_device *device);
+void rdmacg_unregister_device(struct rdmacg_device *device);
+
+/* APIs for RDMA/IB stack to charge/uncharge pool specific resources */
+int rdmacg_try_charge(struct rdma_cgroup **rdmacg,
+ struct rdmacg_device *device,
+ int resource_index);
+void rdmacg_uncharge(struct rdma_cgroup *cg,
+struct rdmacg_device *device,
+int resource_index);
+void rdmacg_query_limit(struct rdmacg_device *device,
+   int *limits);
+
+#endif /* CONFIG_CGROUP_RDMA */
+#endif /* _CGROUP_RDMA_H */
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index 0df0336a..d0e597c 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -56,6 +56,10 @@ SUBSYS(hugetlb)
 SUBSYS(pids)
 #endif
 
+#if IS_ENABLED(CONFIG_CGROUP_RDMA)
+SUBSYS(rdma)
+#endif
+
 /*
  * The following subsystems are not supported on the default hierarchy.
  */
diff --git a/init/Kconfig b/init/Kconfig
index 2232080..0d3efe0 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1054,6 +1054,16 @@ config CGROUP_PIDS
  since the PIDs limit only affects a process's ability to fork, not to
  attach to a cgroup.
 
+config CGROUP_RDMA
+   bool "RDMA controller"
+   help
+ Provides enforcement of RDMA resources defined by IB stack.
+ It is fairly easy for consumers to exhaust RDMA resources, which
+ can result into resource unavailability to other consumers.
+ RDMA controller is designed to stop this from happening.
+ Attaching processes with active RDMA resources to the cgroup
+ hierarchy is allowed even if can cross the hierarchy's limit.
+
 config CGROUP_FREEZER
bool "Freezer controller"
help
diff --git a/kernel/Makefile b/kernel/Makefile
index 53abf00..26e413c 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_COMPAT) += compat.o
 obj-$(CONFIG_CGROUPS) += cgroup.o
 obj-$(CONFIG_CGROUP_FREEZER) += cgroup_freezer.o
 obj-$(CONFIG_CGROUP_PIDS) += cgroup_pids.o
+obj-$(CONFIG_CGROUP_RDMA) += cgroup_rdma.o
 obj-$(CONFIG_CPUSETS) += cpuset.o
 obj-$(CONFIG_UTS_NS) += utsname.o
 obj-$(CONFIG_USER_NS) += user_namespace.o
diff --git a/kernel/cgroup_rdma.c b/kernel/cgroup_rdma.c
new file