| Message ID | 1454167387-31260-2-git-send-email-pandit.parav@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello, On Sat, Jan 30, 2016 at 08:53:05PM +0530, Parav Pandit wrote: > +#ifdef CONFIG_CGROUP_RDMA > +#define RDMACG_MAX_RESOURCE_INDEX (64) The list of resources are known at compile time. Why is this separate fixed number necessary? > +struct match_token; There's no circular dependency issue here. Include the appropriate header. > +struct rdmacg_device; > + > +struct rdmacg_pool_info { > + struct match_token *resource_table; > + int resource_count; > +}; > + > +struct rdmacg_resource_pool_ops { > + struct rdmacg_pool_info* > + (*get_resource_pool_tokens)(struct rdmacg_device *); > +}; Why does it need external op table? The types of resources are gonna be fixed at compile time. The only thing necessary is each device declaring which resources are to be used. > +struct rdmacg_device { > + struct rdmacg_resource_pool_ops > + *rpool_ops[RDMACG_RESOURCE_POOL_TYPE_MAX]; > + struct list_head rdmacg_list; > + char *name; > +}; > + > +/* APIs for RDMA/IB stack to publish when a device wants to > + * participate in resource accounting > + */ Please read CodingStyle. > +config CGROUP_RDMA > + bool "RDMA controller" > + help > + Provides enforcement of RDMA resources at RDMA/IB verb level and > + enforcement of any RDMA/IB capable hardware advertized resources. ^^^^^^^^^? > + Its fairly easy for applications to exhaust RDMA resources, which ^^^ > + can result into kernel consumers or other application consumers of ^ in ^ just say "consumers"? > + RDMA resources left with no resources. RDMA controller is designed ^ The sentence doesn't read well. > + to stop this from happening. > + Attaching existing processes with active RDMA resources to the cgroup > + hierarchy will be allowed even if can cross the hierarchy's limit. ^^^^^? > +#define RDMACG_USR_CMD_REMOVE "remove" Why is this necessary? > +/* resource tracker per resource for rdma cgroup */ > +struct cg_resource { > + int max; > + atomic_t usage; > +}; rdmacg_resource? Also, wouldn't it be better to use u64? > +/** > + * pool type indicating either it got created as part of default > + * operation or user has configured the group. > + * Depends on the creator of the pool, its decided to free up > + * later or not. > + */ > +enum rpool_creator { > + RDMACG_RPOOL_CREATOR_DEFAULT, > + RDMACG_RPOOL_CREATOR_USR, > +}; Why does this matter? > +/** > + * resource pool object which represents, per cgroup, per device, > + * per resource pool_type resources. There are multiple instance > + * of this object per cgroup, therefore it cannot be embedded within > + * rdma_cgroup structure. Its maintained as list. > + */ > +struct cg_resource_pool { > + struct list_head cg_list; > + struct rdmacg_device *device; > + enum rdmacg_resource_pool_type type; > + > + struct cg_resource *resources; > + > + atomic_t refcnt; /* count active user tasks of this pool */ > + atomic_t creator; /* user created or default type */ Why the hell would creator need to be an atomic? You're just using set and read on the field. What's going on? > +static struct rdmacg_resource_pool_ops* > + get_pool_ops(struct rdmacg_device *device, > + enum rdmacg_resource_pool_type pool_type) static struct rdmacg_resource_pool_ops * get_pool_ops(...) > +{ > + return device->rpool_ops[pool_type]; > +} ... > +static void _dealloc_cg_rpool(struct rdma_cgroup *cg, > + struct cg_resource_pool *rpool) > +{ Ugh... please refrain from single underscore prefixed names. It's best to give it a proper name. e.g. if the function assumes lock is grabbed by the user use _locked postfix and so on. > + spin_lock(&cg->cg_list_lock); > + > + /* if its started getting used by other task, before we take the > + * spin lock, then skip freeing it. > + */ Again, CodingStyle. > +static void dealloc_cg_rpool(struct rdma_cgroup *cg, > + struct cg_resource_pool *rpool) > +{ > + /* Don't free the resource pool which is created by the > + * user, otherwise we lose the configured limits. We don't > + * gain much either by splitting storage of limit and usage. > + * So keep it around until user deletes the limits. > + */ > + if (atomic_read(&rpool->creator) == RDMACG_RPOOL_CREATOR_DEFAULT) > + _dealloc_cg_rpool(cg, rpool); > +} The config is tied to the device. If the device goes away, all its pools go away. If the device stays, all configs stay. > +static struct cg_resource_pool* > + find_cg_rpool(struct rdma_cgroup *cg, > + struct rdmacg_device *device, > + enum rdmacg_resource_pool_type type) > + > +{ > + struct cg_resource_pool *pool; > + lockdep_assert_held(...) > + list_for_each_entry(pool, &cg->rpool_head, cg_list) > + if (pool->device == device && pool->type == type) > + return pool; > + > + return NULL; > +} > + > +static struct cg_resource_pool* > + _get_cg_rpool(struct rdma_cgroup *cg, > + struct rdmacg_device *device, > + enum rdmacg_resource_pool_type type) > +{ > + struct cg_resource_pool *rpool; > + > + spin_lock(&cg->cg_list_lock); > + rpool = find_cg_rpool(cg, device, type); > + if (rpool) > + goto found; > +found: That's one extremely silly way to write noop. > + spin_unlock(&cg->cg_list_lock); > + return rpool; > +} This function doesn't make any sense. Push locking to the caller and use find_cg_rpool(). > +static struct cg_resource_pool* > + get_cg_rpool(struct rdma_cgroup *cg, > + struct rdmacg_device *device, > + enum rdmacg_resource_pool_type type) > +{ > + struct cg_resource_pool *rpool, *other_rpool; > + struct rdmacg_pool_info *pool_info; > + struct rdmacg_resource_pool_ops *ops; > + int ret = 0; > + > + spin_lock(&cg->cg_list_lock); > + rpool = find_cg_rpool(cg, device, type); > + if (rpool) { > + atomic_inc(&rpool->refcnt); > + spin_unlock(&cg->cg_list_lock); > + return rpool; > + } Why does it need refcnting? Things stay if the device is there. Things go away if the device goes away. No? Can device go away while resources are allocated? > + spin_unlock(&cg->cg_list_lock); > + > + /* ops cannot be NULL at this stage, as caller made to charge/get > + * the resource pool being aware of such need and invoking with > + * because it has setup resource pool ops. > + */ > + ops = get_pool_ops(device, type); > + pool_info = ops->get_resource_pool_tokens(device); > + if (!pool_info) { > + ret = -EINVAL; > + goto err; > + } Please just do enum { RDMA_RES_VERB_A, RDMA_RES_VERB_B, ... RDMA_RES_VERB_MAX, RDMA_RES_HW_BASE = RDMA_RES_VERB_MAX, RDMA_RES_HW_A = RDMA_RES_HW_BASE, RDMA_RES_HW_B, ... RDMA_RES_HW_MAX, }; static const char rdma_res_name[] = { [RDMA_RES_VERB_A] = "...", ... }; And then let each device specify bitmap of resources that it supports on registration. > + if (pool_info->resource_count == 0 || > + pool_info->resource_count > RDMACG_MAX_RESOURCE_INDEX) { > + ret = -EINVAL; > + goto err; > + } > + > + /* allocate resource pool */ > + rpool = alloc_cg_rpool(cg, device, pool_info->resource_count, type); > + if (IS_ERR_OR_NULL(rpool)) > + return rpool; > + > + /* cgroup lock is held to synchronize with multiple > + * resource pool creation in parallel. > + */ > + spin_lock(&cg->cg_list_lock); > + other_rpool = find_cg_rpool(cg, device, type); > + /* if other task added resource pool for this device for this cgroup > + * free up which was recently created and use the one we found. > + */ > + if (other_rpool) { > + atomic_inc(&other_rpool->refcnt); > + spin_unlock(&cg->cg_list_lock); > + _free_cg_rpool(rpool); > + return other_rpool; > + } > + > + atomic_inc(&rpool->refcnt); > + list_add_tail(&rpool->cg_list, &cg->rpool_head); > + > + spin_unlock(&cg->cg_list_lock); > + return rpool; > + > +err: > + spin_unlock(&cg->cg_list_lock); > + return ERR_PTR(ret); > +} You're grabbing the lock anyway. Why bother with atomics at all? Just grab the lock on entry, look up the entry and then do normal integer ops. The whole thing is still way more complex than it needs to be. Please slim it down. It really shouldn't take half as much code. Keep it simple, please. Thanks.
Hi Tejun, On Sun, Jan 31, 2016 at 12:00 AM, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Sat, Jan 30, 2016 at 08:53:05PM +0530, Parav Pandit wrote: >> +#ifdef CONFIG_CGROUP_RDMA >> +#define RDMACG_MAX_RESOURCE_INDEX (64) > > The list of resources are known at compile time. Why is this separate > fixed number necessary? > Its not necessary. It was carry forwarded from older design. I will remove in v4. >> +struct match_token; > > There's no circular dependency issue here. Include the appropriate > header. o.k. I will fix it. > >> +struct rdmacg_device; >> + >> +struct rdmacg_pool_info { >> + struct match_token *resource_table; >> + int resource_count; >> +}; >> + >> +struct rdmacg_resource_pool_ops { >> + struct rdmacg_pool_info* >> + (*get_resource_pool_tokens)(struct rdmacg_device *); >> +}; > > Why does it need external op table? The types of resources are gonna > be fixed at compile time. IB modules are kernel loadable modules which are defining the resource. So array size and their names are not defined at compile time of rdma cgroup code. Kernel code also cannot depend on loadable module file either via inclusion as that in v2 patch. That leads to crash when loadable modules are upgraded. V1 patch had IB resources defined in the header file of rdmacg, which I believe is very restrictive model with evolving rdma stack and features. Therefore it will be kept as defined in v3 patch in IB headers (non compile time for rdma cgroup). So support infrastructure APIs will continue. > The only thing necessary is each device > declaring which resources are to be used. > Thats what rpool_ops structure does, allowing to query name strings and type of it by utilizing the match tokens. >> +struct rdmacg_device { >> + struct rdmacg_resource_pool_ops >> + *rpool_ops[RDMACG_RESOURCE_POOL_TYPE_MAX]; >> + struct list_head rdmacg_list; >> + char *name; >> +}; >> + >> +/* APIs for RDMA/IB stack to publish when a device wants to >> + * participate in resource accounting >> + */ > > Please read CodingStyle. > Sorry about it. I will add the initial blank line. From driver background I was avoiding it. >> +config CGROUP_RDMA >> + bool "RDMA controller" >> + help >> + Provides enforcement of RDMA resources at RDMA/IB verb level and >> + enforcement of any RDMA/IB capable hardware advertized resources. > ^^^^^^^^^? >> + Its fairly easy for applications to exhaust RDMA resources, which > ^^^ >> + can result into kernel consumers or other application consumers of > ^ in ^ just say "consumers"? >> + RDMA resources left with no resources. RDMA controller is designed > ^ The sentence doesn't read well. >> + to stop this from happening. >> + Attaching existing processes with active RDMA resources to the cgroup >> + hierarchy will be allowed even if can cross the hierarchy's limit. > ^^^^^? > Fixed them. Please review them in next patch. >> +#define RDMACG_USR_CMD_REMOVE "remove" > > Why is this necessary? > User can unset the configured limit by writing "remove" for a given device, instead of writing max values for all the resources. As I explained in cover note and other comment, when its marked for remove, the resource pool is marked as of default type so that it can be freed. When all resources are freed, we don't free the rpool because it holds the configured limit. >> +/* resource tracker per resource for rdma cgroup */ >> +struct cg_resource { >> + int max; >> + atomic_t usage; >> +}; > > rdmacg_resource? Also, wouldn't it be better to use u64? > I will change to rdmacg_resource. As present we have 24-bit wide resources. 64-bit might not needed in near future. If there are inputs comes from Intel/Mellanox for this I will bump to 64-bit. Otherwise I prefer to keep it 32-bit. >> +/** >> + * pool type indicating either it got created as part of default >> + * operation or user has configured the group. >> + * Depends on the creator of the pool, its decided to free up >> + * later or not. >> + */ >> +enum rpool_creator { >> + RDMACG_RPOOL_CREATOR_DEFAULT, >> + RDMACG_RPOOL_CREATOR_USR, >> +}; > > Why does this matter? As you got in later comment and as I explained above, basically resource marked as of user type is not freed, until either device goes away or either user wants to clear the configuration. > >> +/** >> + * resource pool object which represents, per cgroup, per device, >> + * per resource pool_type resources. There are multiple instance >> + * of this object per cgroup, therefore it cannot be embedded within >> + * rdma_cgroup structure. Its maintained as list. >> + */ >> +struct cg_resource_pool { >> + struct list_head cg_list; >> + struct rdmacg_device *device; >> + enum rdmacg_resource_pool_type type; >> + >> + struct cg_resource *resources; >> + >> + atomic_t refcnt; /* count active user tasks of this pool */ >> + atomic_t creator; /* user created or default type */ > > Why the hell would creator need to be an atomic? You're just using > set and read on the field. What's going on? Yes, I can make creator non atomic. > static struct rdmacg_resource_pool_ops * > get_pool_ops(...) ok. Will fix it. > >> +{ >> + return device->rpool_ops[pool_type]; >> +} > ... >> +static void _dealloc_cg_rpool(struct rdma_cgroup *cg, >> + struct cg_resource_pool *rpool) >> +{ > > Ugh... please refrain from single underscore prefixed names. It's > best to give it a proper name. e.g. if the function assumes lock is > grabbed by the user use _locked postfix and so on. > o.k. For this particular one, I merged them to single function. >> + spin_lock(&cg->cg_list_lock); >> + >> + /* if its started getting used by other task, before we take the >> + * spin lock, then skip freeing it. >> + */ > > Again, CodingStyle. I will fix it. > >> +static void dealloc_cg_rpool(struct rdma_cgroup *cg, >> + struct cg_resource_pool *rpool) >> +{ >> + /* Don't free the resource pool which is created by the >> + * user, otherwise we lose the configured limits. We don't >> + * gain much either by splitting storage of limit and usage. >> + * So keep it around until user deletes the limits. >> + */ >> + if (atomic_read(&rpool->creator) == RDMACG_RPOOL_CREATOR_DEFAULT) >> + _dealloc_cg_rpool(cg, rpool); >> +} > > The config is tied to the device. If the device goes away, all its > pools go away. If the device stays, all configs stay. > config stays, until the resources are allocated. If device is there, we don't create resource pool for all the created cgroups to save memory with this little extra code. >> +static struct cg_resource_pool* >> + find_cg_rpool(struct rdma_cgroup *cg, >> + struct rdmacg_device *device, >> + enum rdmacg_resource_pool_type type) >> + >> +{ >> + struct cg_resource_pool *pool; >> + > > lockdep_assert_held(...) > >> + list_for_each_entry(pool, &cg->rpool_head, cg_list) >> + if (pool->device == device && pool->type == type) >> + return pool; >> + >> + return NULL; >> +} >> + >> +static struct cg_resource_pool* >> + _get_cg_rpool(struct rdma_cgroup *cg, >> + struct rdmacg_device *device, >> + enum rdmacg_resource_pool_type type) >> +{ >> + struct cg_resource_pool *rpool; >> + >> + spin_lock(&cg->cg_list_lock); >> + rpool = find_cg_rpool(cg, device, type); >> + if (rpool) >> + goto found; >> +found: > > That's one extremely silly way to write noop. > >> + spin_unlock(&cg->cg_list_lock); >> + return rpool; >> +} > > This function doesn't make any sense. Push locking to the caller and > use find_cg_rpool(). This is nice wrapper around find_cg_rpool to write clean code. I would like to keep it for code readability. if(rpool) check can be removed, because for this function its going to be true always. > >> +static struct cg_resource_pool* >> + get_cg_rpool(struct rdma_cgroup *cg, >> + struct rdmacg_device *device, >> + enum rdmacg_resource_pool_type type) >> +{ >> + struct cg_resource_pool *rpool, *other_rpool; >> + struct rdmacg_pool_info *pool_info; >> + struct rdmacg_resource_pool_ops *ops; >> + int ret = 0; >> + >> + spin_lock(&cg->cg_list_lock); >> + rpool = find_cg_rpool(cg, device, type); >> + if (rpool) { >> + atomic_inc(&rpool->refcnt); >> + spin_unlock(&cg->cg_list_lock); >> + return rpool; >> + } > > Why does it need refcnting? Things stay if the device is there. > Things go away if the device goes away. No? No. If there is one device and 100 cgroups, we create resource pools when there is actually any of the process wants to perform charging. (Instead of creating 100 resource pools just because cgroup exists). So reference count of the rpool checks that when last resource is freed, it frees the resource pool, if its allocated as default pool. If user has configured the pool, than it stays (until device goes away). > Can device go away while resources are allocated? No. Thats ensured by the IB stack as following correct sequence of destruction. > >> + spin_unlock(&cg->cg_list_lock); >> + >> + /* ops cannot be NULL at this stage, as caller made to charge/get >> + * the resource pool being aware of such need and invoking with >> + * because it has setup resource pool ops. >> + */ >> + ops = get_pool_ops(device, type); >> + pool_info = ops->get_resource_pool_tokens(device); >> + if (!pool_info) { >> + ret = -EINVAL; >> + goto err; >> + } > > Please just do > > enum { > RDMA_RES_VERB_A, > RDMA_RES_VERB_B, > ... > RDMA_RES_VERB_MAX, > > RDMA_RES_HW_BASE = RDMA_RES_VERB_MAX, > RDMA_RES_HW_A = RDMA_RES_HW_BASE, > RDMA_RES_HW_B, > ... > RDMA_RES_HW_MAX, > }; > > static const char rdma_res_name[] = { > [RDMA_RES_VERB_A] = "...", > ... > }; What you have described is done in little different way in the loadable kernel module as explained earlier to let it defined by the IB stack. Otherwise this needs to be defined in rdma cgroup header file like my v0 patch, which I certainly want to avoid. > > And then let each device specify bitmap of resources that it supports > on registration. > >> + if (pool_info->resource_count == 0 || >> + pool_info->resource_count > RDMACG_MAX_RESOURCE_INDEX) { >> + ret = -EINVAL; >> + goto err; >> + } >> + >> + /* allocate resource pool */ >> + rpool = alloc_cg_rpool(cg, device, pool_info->resource_count, type); >> + if (IS_ERR_OR_NULL(rpool)) >> + return rpool; >> + >> + /* cgroup lock is held to synchronize with multiple >> + * resource pool creation in parallel. >> + */ >> + spin_lock(&cg->cg_list_lock); >> + other_rpool = find_cg_rpool(cg, device, type); >> + /* if other task added resource pool for this device for this cgroup >> + * free up which was recently created and use the one we found. >> + */ >> + if (other_rpool) { >> + atomic_inc(&other_rpool->refcnt); >> + spin_unlock(&cg->cg_list_lock); >> + _free_cg_rpool(rpool); >> + return other_rpool; >> + } >> + >> + atomic_inc(&rpool->refcnt); >> + list_add_tail(&rpool->cg_list, &cg->rpool_head); >> + >> + spin_unlock(&cg->cg_list_lock); >> + return rpool; >> + >> +err: >> + spin_unlock(&cg->cg_list_lock); >> + return ERR_PTR(ret); >> +} > > You're grabbing the lock anyway. Why bother with atomics at all? > Just grab the lock on entry, look up the entry and then do normal > integer ops. While we free the entry, we don't grab the lock if the refcnt is not reached zero. Please refer to put_cg_rpool that does that. > > The whole thing is still way more complex than it needs to be. Please > slim it down. It really shouldn't take half as much code. Keep it > simple, please. I have tried to keep as simple as possible. If you have specific comments like above, I can certainly address them. > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Parav. On Sun, Jan 31, 2016 at 02:14:13AM +0530, Parav Pandit wrote: ... > V1 patch had IB resources defined in the header file of rdmacg, which > I believe is very restrictive model with evolving rdma stack and > features. Wasn't this the model that we agreed upon? Besides, even if the resources are to be supplied by the driver, a better way would be letting it specify the tables of resources. There's no reason for indirection through a callback. > Therefore it will be kept as defined in v3 patch in IB headers (non > compile time for rdma cgroup). So support infrastructure APIs will > continue. So, what we discussed before just went out the window? > > The only thing necessary is each device > > declaring which resources are to be used. > > > Thats what rpool_ops structure does, allowing to query name strings > and type of it by utilizing the match tokens. Keep the resource types in an array in minimal way and match with the information from core side. It doesn't make any sense to use match tokens in defining resources when the resource type is always fixed. > > Why is this necessary? > > > User can unset the configured limit by writing "remove" for a given > device, instead of writing max values for all the resources. > As I explained in cover note and other comment, when its marked for > remove, the resource pool is marked as of default type so that it can > be freed. When all resources are freed, we don't free the rpool > because it holds the configured limit. I still don't get it. Why isn't this tied to the lifetime of the device? > >> +enum rpool_creator { > >> + RDMACG_RPOOL_CREATOR_DEFAULT, > >> + RDMACG_RPOOL_CREATOR_USR, > >> +}; > > > > Why does this matter? > As you got in later comment and as I explained above, basically > resource marked as of user type is not freed, until either device goes > away or either user wants to clear the configuration. You're re-stating the same thing without explaining the reasoning behind it. Why is this different from other controllers? What's the justification? > >> +static void dealloc_cg_rpool(struct rdma_cgroup *cg, > >> + struct cg_resource_pool *rpool) > >> +{ > >> + /* Don't free the resource pool which is created by the > >> + * user, otherwise we lose the configured limits. We don't > >> + * gain much either by splitting storage of limit and usage. > >> + * So keep it around until user deletes the limits. > >> + */ > >> + if (atomic_read(&rpool->creator) == RDMACG_RPOOL_CREATOR_DEFAULT) > >> + _dealloc_cg_rpool(cg, rpool); > >> +} > > > > The config is tied to the device. If the device goes away, all its > > pools go away. If the device stays, all configs stay. > > > config stays, until the resources are allocated. If device is there, > we don't create resource pool for all the created cgroups to save > memory with this little extra code. Yeah, create on demand all you want but why is the end of lifetime tied to who created? > >> +static struct cg_resource_pool* > >> + _get_cg_rpool(struct rdma_cgroup *cg, > >> + struct rdmacg_device *device, > >> + enum rdmacg_resource_pool_type type) > >> +{ > >> + struct cg_resource_pool *rpool; > >> + > >> + spin_lock(&cg->cg_list_lock); > >> + rpool = find_cg_rpool(cg, device, type); > >> + if (rpool) > >> + goto found; > >> +found: > > > > That's one extremely silly way to write noop. > > > >> + spin_unlock(&cg->cg_list_lock); > >> + return rpool; > >> +} > > > > This function doesn't make any sense. Push locking to the caller and > > use find_cg_rpool(). > > This is nice wrapper around find_cg_rpool to write clean code. I would > like to keep it for code readability. > if(rpool) check can be removed, because for this function its going to > be true always. No, the locking scheme doesn't make any sense. Except for some special cases, sequence like the following indicates that the code is buggy or at least silly. lock; obj = find_refcnted_obj(); unlock; return obj; In this particular case, just push out locking to the users. > >> +static struct cg_resource_pool* > >> + get_cg_rpool(struct rdma_cgroup *cg, > >> + struct rdmacg_device *device, > >> + enum rdmacg_resource_pool_type type) > >> +{ > >> + struct cg_resource_pool *rpool, *other_rpool; > >> + struct rdmacg_pool_info *pool_info; > >> + struct rdmacg_resource_pool_ops *ops; > >> + int ret = 0; > >> + > >> + spin_lock(&cg->cg_list_lock); > >> + rpool = find_cg_rpool(cg, device, type); > >> + if (rpool) { > >> + atomic_inc(&rpool->refcnt); > >> + spin_unlock(&cg->cg_list_lock); > >> + return rpool; > >> + } > > > > Why does it need refcnting? Things stay if the device is there. > > Things go away if the device goes away. No? > > No. If there is one device and 100 cgroups, we create resource pools > when there is actually any of the process wants to perform charging. > (Instead of creating 100 resource pools just because cgroup exists). > So reference count of the rpool checks that when last resource is > freed, it frees the resource pool, if its allocated as default pool. > If user has configured the pool, than it stays (until device goes away). Just create on demand and keep it around till the device is unregistered. > What you have described is done in little different way in the > loadable kernel module as explained earlier to let it defined by the > IB stack. > Otherwise this needs to be defined in rdma cgroup header file like my > v0 patch, which I certainly want to avoid. IIRC, I clearly objected to delegating resource definition to individual drivers. As it currently stands, Nacked-by: Tejun Heo <tj@kernel.org> Thanks.
On Sun, Jan 31, 2016 at 3:32 PM, Tejun Heo <tj@kernel.org> wrote: > Hello, Parav. > > On Sun, Jan 31, 2016 at 02:14:13AM +0530, Parav Pandit wrote: > ... >> V1 patch had IB resources defined in the header file of rdmacg, which >> I believe is very restrictive model with evolving rdma stack and >> features. > > Wasn't this the model that we agreed upon? Besides, even if the > resources are to be supplied by the driver, a better way would be > letting it specify the tables of resources. There's no reason for > indirection through a callback. No. We agreed that let IB stack define in the header file that rdmacg can include. However when I started with that I realized that, such design has basic flaw that IB stack is compiled as loadable modules. cgroup which is compiled along with kernel, cannot rely on the header file of the loadable module, as it will lead to incompatibly and possible crash. Therefore its defined as indirect table. So instead of a callback, it can be further simplified to return as pointer to data structure stored in the rdmacg_device (similar to the name field). Would that be reasonable? > >> Therefore it will be kept as defined in v3 patch in IB headers (non >> compile time for rdma cgroup). So support infrastructure APIs will >> continue. > > So, what we discussed before just went out the window? No. As explained above, structure size and num fields are constant, so lets do above way, without a callback function. >> > The only thing necessary is each device >> > declaring which resources are to be used. >> > >> Thats what rpool_ops structure does, allowing to query name strings >> and type of it by utilizing the match tokens. > > Keep the resource types in an array in minimal way and match with the > information from core side. It doesn't make any sense to use match > tokens in defining resources when the resource type is always fixed. > Match token is being used in other places also typically where user configuration is involved. so Match token infrastructure APIs help to avoid rewriting parser. What exactly is the issue in using match token? Resource type is fixed - sure it is with given version of loadable module. But if new feature/resource is added in IB stack, at that point new token will be added in the array. >> > Why is this necessary? >> > >> User can unset the configured limit by writing "remove" for a given >> device, instead of writing max values for all the resources. >> As I explained in cover note and other comment, when its marked for >> remove, the resource pool is marked as of default type so that it can >> be freed. When all resources are freed, we don't free the rpool >> because it holds the configured limit. > > I still don't get it. Why isn't this tied to the lifetime of the > device? Let me try to take example. Say there is one device and 100 cgroups, with single hierarchy. Out of which 15 are active cgroups allocating rdma resources, rest 85 are not active in terms of rdma resource usage. So rpool object is created for those 15 cgroups (regardless of user has configured limits or not). In this design its not tied to the lifetime of the device. At the sametime if device goes away, those 15 will be freed anyway because device doesn't exist. Now coming to remove command's need. If user has previously configured limit of say mr=15. Now if wants to remove that configuration and don't want to bother for the limit. So the way, its done is by issuing "remove" command. Should I name is "reset". When user issues "remove" command it could still happen that there are active rdma resources. So we cannot really free the rpool object. That is freed when last resource is uncharged. Make sense? > >> >> +enum rpool_creator { >> >> + RDMACG_RPOOL_CREATOR_DEFAULT, >> >> + RDMACG_RPOOL_CREATOR_USR, >> >> +}; >> > >> > Why does this matter? >> As you got in later comment and as I explained above, basically >> resource marked as of user type is not freed, until either device goes >> away or either user wants to clear the configuration. > > You're re-stating the same thing without explaining the reasoning > behind it. Why is this different from other controllers? What's the > justification? As in above example of 15-85, if we keep it or create is we will end up allocating rpool objects for all the 100 cgroups, which might not be necessary. If it happens to be multi level hierarchy, it will end up having in each level. So therefore its allocated and freed dynamically. > >> >> +static void dealloc_cg_rpool(struct rdma_cgroup *cg, >> >> + struct cg_resource_pool *rpool) >> >> +{ >> >> + /* Don't free the resource pool which is created by the >> >> + * user, otherwise we lose the configured limits. We don't >> >> + * gain much either by splitting storage of limit and usage. >> >> + * So keep it around until user deletes the limits. >> >> + */ >> >> + if (atomic_read(&rpool->creator) == RDMACG_RPOOL_CREATOR_DEFAULT) >> >> + _dealloc_cg_rpool(cg, rpool); >> >> +} >> > >> > The config is tied to the device. If the device goes away, all its >> > pools go away. If the device stays, all configs stay. >> > >> config stays, until the resources are allocated. If device is there, >> we don't create resource pool for all the created cgroups to save >> memory with this little extra code. > > Yeah, create on demand all you want but why is the end of lifetime > tied to who created? > If its created by user configuration, and if we free the rpool object when last resource is freed which is holding the configuration, user configuration is lost. Also it doesn't make much sense to have two different allocation for limit and usage configuration. Pointer storing overhead is more than the actual content. >> >> +static struct cg_resource_pool* >> >> + _get_cg_rpool(struct rdma_cgroup *cg, >> >> + struct rdmacg_device *device, >> >> + enum rdmacg_resource_pool_type type) >> >> +{ >> >> + struct cg_resource_pool *rpool; >> >> + >> >> + spin_lock(&cg->cg_list_lock); >> >> + rpool = find_cg_rpool(cg, device, type); >> >> + if (rpool) >> >> + goto found; >> >> +found: >> > >> > That's one extremely silly way to write noop. >> > >> >> + spin_unlock(&cg->cg_list_lock); >> >> + return rpool; >> >> +} >> > >> > This function doesn't make any sense. Push locking to the caller and >> > use find_cg_rpool(). >> >> This is nice wrapper around find_cg_rpool to write clean code. I would >> like to keep it for code readability. >> if(rpool) check can be removed, because for this function its going to >> be true always. > > No, the locking scheme doesn't make any sense. Except for some > special cases, sequence like the following indicates that the code is > buggy or at least silly. > Help me to understand, silly means - unacceptable? I am still trying to understand why a wrapper function makes the code buggy. > lock; > obj = find_refcnted_obj(); > unlock; > return obj; > > In this particular case, just push out locking to the users. > >> >> +static struct cg_resource_pool* >> >> + get_cg_rpool(struct rdma_cgroup *cg, >> >> + struct rdmacg_device *device, >> >> + enum rdmacg_resource_pool_type type) >> >> +{ >> >> + struct cg_resource_pool *rpool, *other_rpool; >> >> + struct rdmacg_pool_info *pool_info; >> >> + struct rdmacg_resource_pool_ops *ops; >> >> + int ret = 0; >> >> + >> >> + spin_lock(&cg->cg_list_lock); >> >> + rpool = find_cg_rpool(cg, device, type); >> >> + if (rpool) { >> >> + atomic_inc(&rpool->refcnt); >> >> + spin_unlock(&cg->cg_list_lock); >> >> + return rpool; >> >> + } >> > >> > Why does it need refcnting? Things stay if the device is there. >> > Things go away if the device goes away. No? >> >> No. If there is one device and 100 cgroups, we create resource pools >> when there is actually any of the process wants to perform charging. >> (Instead of creating 100 resource pools just because cgroup exists). >> So reference count of the rpool checks that when last resource is >> freed, it frees the resource pool, if its allocated as default pool. >> If user has configured the pool, than it stays (until device goes away). > > Just create on demand and keep it around till the device is > unregistered. > Why you don't want them to be freed when there are no requester allocating the resource? Device usually stays for longer time, but applications go way and come back more often, so freeing them makes more sense when not in use. What exactly is the problem in freeing when uncharing occurs, due to which I should defer it to device unregistration stage? >> What you have described is done in little different way in the >> loadable kernel module as explained earlier to let it defined by the >> IB stack. >> Otherwise this needs to be defined in rdma cgroup header file like my >> v0 patch, which I certainly want to avoid. > > IIRC, I clearly objected to delegating resource definition to > individual drivers. > I understand that. Instead of individual driver, it can be well abstracted out at IB stack level that drivers will use. So that it will be in cgroup.c or other file, but with that I don't anticipate a design change. I have not received review comments from Sean Hefty or any of the Intel folks who requested this feature. So I will keep this feature around for a while. I will ping him as well to finish his reviews and if there is any resource definitions that they can spell out. In absence of those inputs, other possibility is to just define verb level resource. I am keeping doors open for more review comments from IB folks on how do they see this. > As it currently stands, > > Nacked-by: Tejun Heo <tj@kernel.org> > No problem. Lets resolve these review comments for v6. > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Parav. On Sun, Jan 31, 2016 at 04:11:54PM +0530, Parav Pandit wrote: > No. We agreed that let IB stack define in the header file that rdmacg > can include. > However when I started with that I realized that, such design has > basic flaw that IB stack is compiled as loadable modules. > cgroup which is compiled along with kernel, cannot rely on the header > file of the loadable module, as it will lead to incompatibly and > possible crash. Yes, it can. It just becomes a part of kernel ABI that the IB stack module depends on. > > Keep the resource types in an array in minimal way and match with the > > information from core side. It doesn't make any sense to use match > > tokens in defining resources when the resource type is always fixed. > > > Match token is being used in other places also typically where user > configuration is involved. > so Match token infrastructure APIs help to avoid rewriting parser. > What exactly is the issue in using match token? > Resource type is fixed - sure it is with given version of loadable > module. But if new feature/resource is added in IB stack, at that > point new token will be added in the array. Sure, use parser for parsing but you end up stripping =%d in other places anyway. Do it the other way. Let the definitions contain what's essential and do the extra stuff in code handling it, not the other way around. > Now coming to remove command's need. > If user has previously configured limit of say mr=15. Now if wants to > remove that configuration and don't want to bother for the limit. > So the way, its done is by issuing "remove" command. > Should I name is "reset". How is this different from setting the limits to max? > When user issues "remove" command it could still happen that there are > active rdma resources. So we cannot really free the rpool object. > That is freed when last resource is uncharged. > Make sense? Not really. > > You're re-stating the same thing without explaining the reasoning > > behind it. Why is this different from other controllers? What's the > > justification? > > As in above example of 15-85, if we keep it or create is we will end > up allocating rpool objects for all the 100 cgroups, which might not > be necessary. If it happens to be multi level hierarchy, it will end > up having in each level. > So therefore its allocated and freed dynamically. Just keeping them around till device destruction would work just fine. If you wanna insist on freeing them dynamically, free them when both their usages and configs are nil. How the object is created shouldn't be a factor. > If its created by user configuration, and if we free the rpool object > when last resource is freed which is holding the configuration, > user configuration is lost. > Also it doesn't make much sense to have two different allocation for > limit and usage configuration. Pointer storing overhead is more than > the actual content. See above. If you want to free dynamically, free when the thing doesn't contain any information. Better, just don't worry about it. It's unlikely to matter. > > No, the locking scheme doesn't make any sense. Except for some > > special cases, sequence like the following indicates that the code is > > buggy or at least silly. > > > Help me to understand, silly means - unacceptable? Yes, it's a clear anti-pattern for a refcnted object. Again, just push the locking to the users and drop the atomics. > Why you don't want them to be freed when there are no requester > allocating the resource? > Device usually stays for longer time, but applications go way and come > back more often, so freeing them makes more sense when not in use. > What exactly is the problem in freeing when uncharing occurs, due to > which I should defer it to device unregistration stage? Two things. Freeing dynamically doesn't require creator type or refcnting here, so the code makes no sense to me. It's complicated without any purpose. Second, it's not that big a struct. Just leave them around till it's clear that this is problematic. For the Nth time in this whole thing, keep things simple. Stop overengineering.
Hi Doug, Liran, Jason, How would you like to see RDMA verb resources being defined - in RDMA cgroup or in IB stack? In current patch v5, its defined by the IB stack which is often shipped as different package due to high amount of changes, bug fixes, features. In v0 patch it was defined by the RDMA cgroup, which means any new resource addition/definition requires kernel upgrade. Which is hard to change often. If resources are defined by RDMA cgroup kernel than adding/removing resource means, someone have to do lot of engineering with different versions of kernel support and loadable module support using compat.h etc at driver level, which in my mind is some amount of engineering compare to what v5 has to offer and its already available. With one round of cleanup in resource definition, it should be usable. Its important to provide this feedback to Tejun and me, so that we take informed design decision. Hi Tejun, On Sun, Jan 31, 2016 at 4:34 PM, Tejun Heo <tj@kernel.org> wrote: > Hello, Parav. > > On Sun, Jan 31, 2016 at 04:11:54PM +0530, Parav Pandit wrote: >> No. We agreed that let IB stack define in the header file that rdmacg >> can include. >> However when I started with that I realized that, such design has >> basic flaw that IB stack is compiled as loadable modules. >> cgroup which is compiled along with kernel, cannot rely on the header >> file of the loadable module, as it will lead to incompatibly and >> possible crash. > > Yes, it can. It just becomes a part of kernel ABI that the IB stack > module depends on. > o.k. Its doable, but I believe its simple but its restrictive. If rest of the RDMA experts are ok with it, I will change it to do it that way. We get to hear their feedback before we put this as ABI. >> > Keep the resource types in an array in minimal way and match with the >> > information from core side. It doesn't make any sense to use match >> > tokens in defining resources when the resource type is always fixed. >> > >> Match token is being used in other places also typically where user >> configuration is involved. >> so Match token infrastructure APIs help to avoid rewriting parser. >> What exactly is the issue in using match token? >> Resource type is fixed - sure it is with given version of loadable >> module. But if new feature/resource is added in IB stack, at that >> point new token will be added in the array. > > Sure, use parser for parsing but you end up stripping =%d in other > places anyway. Do it the other way. Let the definitions contain > what's essential and do the extra stuff in code handling it, not the > other way around. > >> Now coming to remove command's need. >> If user has previously configured limit of say mr=15. Now if wants to >> remove that configuration and don't want to bother for the limit. >> So the way, its done is by issuing "remove" command. >> Should I name is "reset". > > How is this different from setting the limits to max? > We can set it to max. But during freeing time, checking array of 10+ elements whether its max or not, didn't find efficient either. >> When user issues "remove" command it could still happen that there are >> active rdma resources. So we cannot really free the rpool object. >> That is freed when last resource is uncharged. >> Make sense? > > Not really. > >> > You're re-stating the same thing without explaining the reasoning >> > behind it. Why is this different from other controllers? What's the >> > justification? >> >> As in above example of 15-85, if we keep it or create is we will end >> up allocating rpool objects for all the 100 cgroups, which might not >> be necessary. If it happens to be multi level hierarchy, it will end >> up having in each level. >> So therefore its allocated and freed dynamically. > > Just keeping them around till device destruction would work just fine. Most likely not. I tried to explained that in reply to Haggai's email for RFC, also in rdma.txt documentation patch 3/3. Let me try again. Device stays in a system for long time. Mostly for months until drivers are unloaded or firmware crashes or some other event. While on other hand, cgroup life cycle is in hours or more. So rpool that we allocated dynamically, it needs to be freed or atleast when set to max or when cgroup is deleted. But there is catch there, it cannot be freed because there are processed which has allocated the resource from this cgroup, but have been migrated to other cgroup which will uncharge sometime later. (This is not a usecase, but its the artifact of cgroup interface). So rpool needs to be freed at later when uncharge occurs. At that point we drop the reference to the css, so that css can be freed and same css->id can get circulated for new cgroup. If we don't do this, and wait until device destruction, rpool just keeps growing! I think above is more important design aspect than just saving memory to me. Let me know if you have different design thought here. (Like engineering can_attach() callback in rdma_cgroup, which I don't see necessary either). > If you wanna insist on freeing them dynamically, free them when both > their usages and configs are nil. Agree. Thats what the code is doing using marking object type being default. If I remove the object type, as alternative, It requires that when we uncharge resource, we should run through the max array to see if they are all set to max. And keep one aggregate counter for usage (apart from individual) counter for each resource. > How the object is created shouldn't be a factor. Loop for max and aggregate counter can avoid creator variable. > >> If its created by user configuration, and if we free the rpool object >> when last resource is freed which is holding the configuration, >> user configuration is lost. >> Also it doesn't make much sense to have two different allocation for >> limit and usage configuration. Pointer storing overhead is more than >> the actual content. > > See above. If you want to free dynamically, free when the thing > doesn't contain any information. o.k. > Better, just don't worry about it. It's unlikely to matter. I tried to explain above about the need to free dynamically, instead of device destruction time. > >> > No, the locking scheme doesn't make any sense. Except for some >> > special cases, sequence like the following indicates that the code is >> > buggy or at least silly. >> > >> Help me to understand, silly means - unacceptable? > > Yes, it's a clear anti-pattern for a refcnted object. Again, just > push the locking to the users and drop the atomics. > ok. Let me see how can I refactor this part of code. I will get back on this if I hit a block. >> Why you don't want them to be freed when there are no requester >> allocating the resource? >> Device usually stays for longer time, but applications go way and come >> back more often, so freeing them makes more sense when not in use. >> What exactly is the problem in freeing when uncharing occurs, due to >> which I should defer it to device unregistration stage? > > Two things. Freeing dynamically doesn't require creator type or > refcnting here, so the code makes no sense to me. It's complicated > without any purpose. Second, it's not that big a struct. Just leave > them around till it's clear that this is problematic. Above explanation should justify that its problematic to keep that around until device destruction. > For the Nth time in this whole thing, keep things simple. Stop overengineering. I have tried to keep in simple. I respect your comments and have improved upon most of the valuable comments that you gave in subsequent patches from v0 to v5. Few exceptions which I will work through now are: 1. resource definition place (cgroup vs IB stack) - avoid callback. 2. lock functions 3. dynamic freeing (which I think is justified now above). -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Parav. On Sun, Jan 31, 2016 at 11:20:45PM +0530, Parav Pandit wrote: > > Yes, it can. It just becomes a part of kernel ABI that the IB stack > > module depends on. > > > o.k. Its doable, but I believe its simple but its restrictive. The whole point is to make it somewhat restrictive so that low level drivers don't go crazy with it and the defined resources are clearly identified and documented. > If rest of the RDMA experts are ok with it, I will change it to do it that way. > We get to hear their feedback before we put this as ABI. So, I'm really not gonna go for individual drivers defining resources on their own. That's a trainwreck waiting to happen. There needs to be a lot more scrutiny than that. > >> Now coming to remove command's need. > >> If user has previously configured limit of say mr=15. Now if wants to > >> remove that configuration and don't want to bother for the limit. > >> So the way, its done is by issuing "remove" command. > >> Should I name is "reset". > > > > How is this different from setting the limits to max? > > We can set it to max. But during freeing time, checking array of 10+ > elements whether its max or not, didn't find efficient either. So, in terms of user interface, it doesn't buy anything, right? It's generally a bad idea to mix implementation details like "checking 10+ elems on free" with interface decisions. If the overhead of scanning the array matters, it can easily be resolved by keeping the count of non-max values. It could be that setting all attributes to "max" is inconvenient from interface POV (I don't think this would be a common use case tho). If so, please justify that, update the interface conventions along with other similar knobs. Don't introduce one-off custom thing. ... > If we don't do this, and wait until device destruction, rpool just > keeps growing! > > I think above is more important design aspect than just saving memory to me. > > Let me know if you have different design thought here. > (Like engineering can_attach() callback in rdma_cgroup, which I don't > see necessary either). Pin the css while things are in flight and free from css_free? > > If you wanna insist on freeing them dynamically, free them when both > > their usages and configs are nil. > Agree. Thats what the code is doing using marking object type being default. > If I remove the object type, as alternative, You don't need object type for that. Think about it again. All you need is a refcnt and cgroup already has a facility for that. Thanks.
On Tue, Feb 2, 2016 at 12:10 AM, Tejun Heo <tj@kernel.org> wrote: > Hello, Parav. > > On Sun, Jan 31, 2016 at 11:20:45PM +0530, Parav Pandit wrote: >> > Yes, it can. It just becomes a part of kernel ABI that the IB stack >> > module depends on. >> > >> o.k. Its doable, but I believe its simple but its restrictive. > > The whole point is to make it somewhat restrictive so that low level > drivers don't go crazy with it and the defined resources are clearly > identified and documented. > >> If rest of the RDMA experts are ok with it, I will change it to do it that way. >> We get to hear their feedback before we put this as ABI. > > So, I'm really not gonna go for individual drivers defining resources > on their own. That's a trainwreck waiting to happen. There needs to > be a lot more scrutiny than that. > Not every low level driver. I started with that infrastructure in v2,v3 but I got your inputs and I align with that. It could be just single IB stack in one header file in one enum list would be sufficient. You have already given that example. With that mostly two resource type that I have can also shrink to just single type. Will wait to hear from them, in case if they have any different thought. >> >> Now coming to remove command's need. >> >> If user has previously configured limit of say mr=15. Now if wants to >> >> remove that configuration and don't want to bother for the limit. >> >> So the way, its done is by issuing "remove" command. >> >> Should I name is "reset". >> > >> > How is this different from setting the limits to max? >> >> We can set it to max. But during freeing time, checking array of 10+ >> elements whether its max or not, didn't find efficient either. > > So, in terms of user interface, it doesn't buy anything, right? It's > generally a bad idea to mix implementation details like "checking 10+ > elems on free" with interface decisions. If the overhead of scanning > the array matters, it can easily be resolved by keeping the count of > non-max values. > > It could be that setting all attributes to "max" is inconvenient from > interface POV (I don't think this would be a common use case tho). If > so, please justify that, update the interface conventions along with > other similar knobs. Don't introduce one-off custom thing. > ok. setting all values to max is not really a common use case. So I believe its ok to do that and therefore remove "remove" option. > ... >> If we don't do this, and wait until device destruction, rpool just >> keeps growing! >> >> I think above is more important design aspect than just saving memory to me. >> >> Let me know if you have different design thought here. >> (Like engineering can_attach() callback in rdma_cgroup, which I don't >> see necessary either). > > Pin the css while things are in flight and free from css_free? Yes. Thats what is done in current patch. css_get on charge and css_put on uncharge, that takes care nicely to free the rpool. > >> > If you wanna insist on freeing them dynamically, free them when both >> > their usages and configs are nil. >> Agree. Thats what the code is doing using marking object type being default. >> If I remove the object type, as alternative, > > You don't need object type for that. Think about it again. All you > need is a refcnt and cgroup already has a facility for that. > ok. Let me attempt that. I will wait for day or two before I roll out v6 to get any feedback if Liran, Haggai or Doug has any comments on definition part for resource type. > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/02/2016 20:59, Parav Pandit wrote: > On Tue, Feb 2, 2016 at 12:10 AM, Tejun Heo <tj@kernel.org> wrote: >> So, I'm really not gonna go for individual drivers defining resources >> on their own. That's a trainwreck waiting to happen. There needs to >> be a lot more scrutiny than that. >> > Not every low level driver. I started with that infrastructure in > v2,v3 but I got your inputs and > I align with that. It could be just single IB stack in one header file > in one enum list would be sufficient. > You have already given that example. > With that mostly two resource type that I have can also shrink to just > single type. > Will wait to hear from them, in case if they have any different thought. Hi, Sorry for the late reply. I think that starting with the standard set of resources that uverbs provide is good, and if we need in the future new types of resources we can add them later. On 31/01/2016 19:50, Parav Pandit wrote: > How would you like to see RDMA verb resources being defined - in RDMA > cgroup or in IB stack? > In current patch v5, its defined by the IB stack which is often > shipped as different package due to high amount of changes, bug fixes, > features. > In v0 patch it was defined by the RDMA cgroup, which means any new > resource addition/definition requires kernel upgrade. Which is hard to > change often. There is indeed an effort to backport the latest RDMA subsystem modules to older kernels, and it would be preferable to be able to introduce new resources through these modules. However, I understand that there are no other cgroups that are modules or depend on modules this way, so I would understand if you decide against it. > If resources are defined by RDMA cgroup kernel than adding/removing > resource means, someone have to do lot of engineering with different > versions of kernel support and loadable module support using compat.h > etc at driver level, which in my mind is some amount of engineering > compare to what v5 has to offer and its already available. With one > round of cleanup in resource definition, it should be usable. If I understand correctly, if the resources are defined in the cgroup, you simply won't be able to add new resources with a module update, no matter how hard you work. I agree that if the cgroup code is changed for cleanup or whatever reason, the backporting may become difficult, but that's just life. > Its important to provide this feedback to Tejun and me, so that we > take informed design decision. Sure. I hope this patchset gets accepted eventually, as I believe it solves a real problem. Today RDMA application can easily hog these resources and the rdma cgroup allows users to prevent that. Regards, Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 10, 2016 at 9:57 PM, Haggai Eran <haggaie@mellanox.com> wrote: > > There is indeed an effort to backport the latest RDMA subsystem modules to > older kernels, and it would be preferable to be able to introduce new > resources through these modules. Right. There is hardly 10 to 20 lines of code that allows to support this functionality. So I think its good thing to have. > > If resources are defined by RDMA cgroup kernel than adding/removing > > resource means, someone have to do lot of engineering with different > > versions of kernel support and loadable module support using compat.h > > etc at driver level, which in my mind is some amount of engineering > > compare to what v5 has to offer and its already available. With one > > round of cleanup in resource definition, it should be usable. > If I understand correctly, if the resources are defined in the cgroup, > you simply won't be able to add new resources with a module update, > no matter how hard you work. > Yes. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tejun, (Sending again as by mistake previous mail was non plain text, sorry about that). So based on your comments, Haggai's comments below and past experience, I will spin v6.o I have made changes, and testing them. I am likely to post during coming long weekend. I have simplified many pieces. Can you please confirm below design/implementation looks ok to you? If you want to review v6 directly, I am fine with that too instead of below post. 1. Removed two type of resource pool, made is single type (as you described in past comment) 2. Removed match tokens and have array definition like "qp", "mr", "cq" etc. 3. Wrote small parser and avoided match_token API as that won't work due to different array definition 4. Removed one-off remove API to unconfigure cgroup, instead all resource should be set to max. 5. Removed resource pool type (user/default), instead having max_num_cnt, when ref_cnt drops to zero and max_num_cnt = total_rescource_cnt, pool is freed. 6. Resource definition ownership is now only with IB stack at single header file, no longer in each low level driver. This goes through IB maintainer and other reviewers eyes. This continue to give flexibility to not force kernel upgrade for few enums additions for new resource type. 7. Wherever possible pool lock is pushed out, except for hierarchical charging/unchanging points, as it not possible to do so, due to iterative process involves blocking allocations of rpool. Coming up more levels up to release locks doesn't make any sense either. This is anyway slow path where rpool is not allocated. Except for typical first resource allocation, this is less traveled path. 8.Other minor cleanups. 9. Avoided %d manipulation due to removal of match_token and replaced with seq_putc etc friend functions. Parav On Wed, Feb 10, 2016 at 9:57 PM, Haggai Eran <haggaie@mellanox.com> wrote: > On 01/02/2016 20:59, Parav Pandit wrote: >> On Tue, Feb 2, 2016 at 12:10 AM, Tejun Heo <tj@kernel.org> wrote: >>> So, I'm really not gonna go for individual drivers defining resources >>> on their own. That's a trainwreck waiting to happen. There needs to >>> be a lot more scrutiny than that. >>> >> Not every low level driver. I started with that infrastructure in >> v2,v3 but I got your inputs and >> I align with that. It could be just single IB stack in one header file >> in one enum list would be sufficient. >> You have already given that example. >> With that mostly two resource type that I have can also shrink to just >> single type. >> Will wait to hear from them, in case if they have any different thought. > > Hi, > > Sorry for the late reply. > > I think that starting with the standard set of resources that uverbs > provide is good, and if we need in the future new types of resources > we can add them later. > > On 31/01/2016 19:50, Parav Pandit wrote: >> How would you like to see RDMA verb resources being defined - in RDMA >> cgroup or in IB stack? >> In current patch v5, its defined by the IB stack which is often >> shipped as different package due to high amount of changes, bug fixes, >> features. >> In v0 patch it was defined by the RDMA cgroup, which means any new >> resource addition/definition requires kernel upgrade. Which is hard to >> change often. > > There is indeed an effort to backport the latest RDMA subsystem modules to > older kernels, and it would be preferable to be able to introduce new > resources through these modules. However, I understand that there are no > other cgroups that are modules or depend on modules this way, so I would > understand if you decide against it. > >> If resources are defined by RDMA cgroup kernel than adding/removing >> resource means, someone have to do lot of engineering with different >> versions of kernel support and loadable module support using compat.h >> etc at driver level, which in my mind is some amount of engineering >> compare to what v5 has to offer and its already available. With one >> round of cleanup in resource definition, it should be usable. > If I understand correctly, if the resources are defined in the cgroup, > you simply won't be able to add new resources with a module update, > no matter how hard you work. > > I agree that if the cgroup code is changed for cleanup or whatever > reason, the backporting may become difficult, but that's just life. > >> Its important to provide this feedback to Tejun and me, so that we >> take informed design decision. > > Sure. I hope this patchset gets accepted eventually, as I believe it > solves a real problem. Today RDMA application can easily hog these > resources and the rdma cgroup allows users to prevent that. > > Regards, > Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Parav. On Fri, Feb 12, 2016 at 02:49:38AM +0530, Parav Pandit wrote: > 1. Removed two type of resource pool, made is single type (as you > described in past comment) > 2. Removed match tokens and have array definition like "qp", "mr", "cq" etc. > 3. Wrote small parser and avoided match_token API as that won't work > due to different array definition > 4. Removed one-off remove API to unconfigure cgroup, instead all > resource should be set to max. > 5. Removed resource pool type (user/default), instead having max_num_cnt, > when ref_cnt drops to zero and max_num_cnt = total_rescource_cnt, pool is freed. > 6. Resource definition ownership is now only with IB stack at single > header file, no longer in each low level driver. > This goes through IB maintainer and other reviewers eyes. > This continue to give flexibility to not force kernel upgrade for few > enums additions for new resource type. > 7. Wherever possible pool lock is pushed out, except for hierarchical > charging/unchanging points, as it not possible to do so, due to > iterative process involves blocking allocations of rpool. Coming up > more levels up to release locks doesn't make any sense either. > This is anyway slow path where rpool is not allocated. Except for > typical first resource allocation, this is less traveled path. > 8.Other minor cleanups. > 9. Avoided %d manipulation due to removal of match_token and replaced > with seq_putc etc friend functions. Sounds great. Can't tell too much without looking at the code tho. Thanks.
diff --git a/include/linux/cgroup_rdma.h b/include/linux/cgroup_rdma.h new file mode 100644 index 0000000..5e6d9d8 --- /dev/null +++ b/include/linux/cgroup_rdma.h @@ -0,0 +1,80 @@ +#ifndef _CGROUP_RDMA_H +#define _CGROUP_RDMA_H + +#include <linux/cgroup.h> + +/* + * This file is subject to the terms and conditions of version 2 of the GNU + * General Public License. See the file COPYING in the main directory of the + * Linux distribution for more details. + */ + +enum rdmacg_resource_pool_type { + RDMACG_RESOURCE_POOL_VERB, + RDMACG_RESOURCE_POOL_HW, + RDMACG_RESOURCE_POOL_TYPE_MAX, +}; + +struct rdma_cgroup { +#ifdef CONFIG_CGROUP_RDMA + struct cgroup_subsys_state css; + + spinlock_t cg_list_lock; /* protects cgroup resource pool list */ + struct list_head rpool_head; /* head to keep track of all resource + * pools that belongs to this cgroup. + */ +#endif +}; + +#ifdef CONFIG_CGROUP_RDMA +#define RDMACG_MAX_RESOURCE_INDEX (64) + +struct match_token; +struct rdmacg_device; + +struct rdmacg_pool_info { + struct match_token *resource_table; + int resource_count; +}; + +struct rdmacg_resource_pool_ops { + struct rdmacg_pool_info* + (*get_resource_pool_tokens)(struct rdmacg_device *); +}; + +struct rdmacg_device { + struct rdmacg_resource_pool_ops + *rpool_ops[RDMACG_RESOURCE_POOL_TYPE_MAX]; + struct list_head rdmacg_list; + char *name; +}; + +/* APIs for RDMA/IB stack to publish when a device wants to + * participate in resource accounting + */ +void rdmacg_register_device(struct rdmacg_device *device, char *dev_name); +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, + enum rdmacg_resource_pool_type type, + int resource_index, + int num); +void rdmacg_uncharge(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int resource_index, + int num); + +void rdmacg_set_rpool_ops(struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type, + struct rdmacg_resource_pool_ops *ops); +void rdmacg_clear_rpool_ops(struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type); +int rdmacg_query_limit(struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int *limits, int max_count); + +#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 f8754f5..f8055f5 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1070,6 +1070,18 @@ 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 at RDMA/IB verb level and + enforcement of any RDMA/IB capable hardware advertized resources. + Its fairly easy for applications to exhaust RDMA resources, which + can result into kernel consumers or other application consumers of + RDMA resources left with no resources. RDMA controller is designed + to stop this from happening. + Attaching existing processes with active RDMA resources to the cgroup + hierarchy will be 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 mode 100644 index 0000000..305d791 --- /dev/null +++ b/kernel/cgroup_rdma.c @@ -0,0 +1,1021 @@ +/* + * This file is subject to the terms and conditions of version 2 of the GNU + * General Public License. See the file COPYING in the main directory of the + * Linux distribution for more details. + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/threads.h> +#include <linux/pid.h> +#include <linux/spinlock.h> +#include <linux/parser.h> +#include <linux/atomic.h> +#include <linux/seq_file.h> +#include <linux/hashtable.h> +#include <linux/cgroup.h> +#include <linux/cgroup_rdma.h> + +static DEFINE_MUTEX(dev_mutex); +static LIST_HEAD(dev_list); + +enum rdmacg_file_type { + RDMACG_VERB_RESOURCE_MAX, + RDMACG_VERB_RESOURCE_STAT, + RDMACG_HW_RESOURCE_MAX, + RDMACG_HW_RESOURCE_STAT, +}; + +#define RDMACG_USR_CMD_REMOVE "remove" + +/* resource tracker per resource for rdma cgroup */ +struct cg_resource { + int max; + atomic_t usage; +}; + +/** + * pool type indicating either it got created as part of default + * operation or user has configured the group. + * Depends on the creator of the pool, its decided to free up + * later or not. + */ +enum rpool_creator { + RDMACG_RPOOL_CREATOR_DEFAULT, + RDMACG_RPOOL_CREATOR_USR, +}; + +/** + * resource pool object which represents, per cgroup, per device, + * per resource pool_type resources. There are multiple instance + * of this object per cgroup, therefore it cannot be embedded within + * rdma_cgroup structure. Its maintained as list. + */ +struct cg_resource_pool { + struct list_head cg_list; + struct rdmacg_device *device; + enum rdmacg_resource_pool_type type; + + struct cg_resource *resources; + + atomic_t refcnt; /* count active user tasks of this pool */ + atomic_t creator; /* user created or default type */ +}; + +static struct rdma_cgroup *css_rdmacg(struct cgroup_subsys_state *css) +{ + return container_of(css, struct rdma_cgroup, css); +} + +static struct rdma_cgroup *parent_rdmacg(struct rdma_cgroup *cg) +{ + return css_rdmacg(cg->css.parent); +} + +static inline struct rdma_cgroup *task_rdmacg(struct task_struct *task) +{ + return css_rdmacg(task_css(task, rdma_cgrp_id)); +} + +static struct rdmacg_resource_pool_ops* + get_pool_ops(struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type) +{ + return device->rpool_ops[pool_type]; +} + +static inline void set_resource_limit(struct cg_resource_pool *rpool, + int index, int new_max) +{ + rpool->resources[index].max = new_max; +} + +static void _free_cg_rpool(struct cg_resource_pool *rpool) +{ + kfree(rpool->resources); + kfree(rpool); +} + +static void _dealloc_cg_rpool(struct rdma_cgroup *cg, + struct cg_resource_pool *rpool) +{ + spin_lock(&cg->cg_list_lock); + + /* if its started getting used by other task, before we take the + * spin lock, then skip freeing it. + */ + if (atomic_read(&rpool->refcnt) == 0) { + list_del_init(&rpool->cg_list); + spin_unlock(&cg->cg_list_lock); + + _free_cg_rpool(rpool); + return; + } + spin_unlock(&cg->cg_list_lock); +} + +static void dealloc_cg_rpool(struct rdma_cgroup *cg, + struct cg_resource_pool *rpool) +{ + /* Don't free the resource pool which is created by the + * user, otherwise we lose the configured limits. We don't + * gain much either by splitting storage of limit and usage. + * So keep it around until user deletes the limits. + */ + if (atomic_read(&rpool->creator) == RDMACG_RPOOL_CREATOR_DEFAULT) + _dealloc_cg_rpool(cg, rpool); +} + +static void put_cg_rpool(struct rdma_cgroup *cg, + struct cg_resource_pool *rpool) +{ + if (atomic_dec_and_test(&rpool->refcnt)) + dealloc_cg_rpool(cg, rpool); +} + +static struct cg_resource_pool* + alloc_cg_rpool(struct rdma_cgroup *cg, + struct rdmacg_device *device, + int count, + enum rdmacg_resource_pool_type type) +{ + struct cg_resource_pool *rpool; + int i, ret; + + rpool = kzalloc(sizeof(*rpool), GFP_KERNEL); + if (!rpool) { + ret = -ENOMEM; + goto err; + } + rpool->resources = kcalloc(count, sizeof(*rpool->resources), + GFP_KERNEL); + if (!rpool->resources) { + ret = -ENOMEM; + goto alloc_err; + } + + /* set pool ownership and type, so that it can be freed correctly */ + rpool->device = device; + rpool->type = type; + INIT_LIST_HEAD(&rpool->cg_list); + atomic_set(&rpool->creator, RDMACG_RPOOL_CREATOR_DEFAULT); + + for (i = 0; i < count; i++) + set_resource_limit(rpool, i, S32_MAX); + + return rpool; + +alloc_err: + kfree(rpool); +err: + return ERR_PTR(ret); +} + +static struct cg_resource_pool* + find_cg_rpool(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type) + +{ + struct cg_resource_pool *pool; + + list_for_each_entry(pool, &cg->rpool_head, cg_list) + if (pool->device == device && pool->type == type) + return pool; + + return NULL; +} + +static struct cg_resource_pool* + _get_cg_rpool(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type) +{ + struct cg_resource_pool *rpool; + + spin_lock(&cg->cg_list_lock); + rpool = find_cg_rpool(cg, device, type); + if (rpool) + goto found; +found: + spin_unlock(&cg->cg_list_lock); + return rpool; +} + +/** + * get_cg_rpool - get pid_cg map reference. + * @cg: cgroup for which resouce pool to be allocated. + * @device: device for which to allocate resource pool. + * @type: type of the resource pool. + * + * It searches a cgroup resource pool object for a given device and resource + * type, if it finds it would return it. If none is present, it would allocate + * new resource pool entry of default type. + * Returns resource pool on success, else return error pointer or null. + */ +static struct cg_resource_pool* + get_cg_rpool(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type) +{ + struct cg_resource_pool *rpool, *other_rpool; + struct rdmacg_pool_info *pool_info; + struct rdmacg_resource_pool_ops *ops; + int ret = 0; + + spin_lock(&cg->cg_list_lock); + rpool = find_cg_rpool(cg, device, type); + if (rpool) { + atomic_inc(&rpool->refcnt); + spin_unlock(&cg->cg_list_lock); + return rpool; + } + spin_unlock(&cg->cg_list_lock); + + /* ops cannot be NULL at this stage, as caller made to charge/get + * the resource pool being aware of such need and invoking with + * because it has setup resource pool ops. + */ + ops = get_pool_ops(device, type); + pool_info = ops->get_resource_pool_tokens(device); + if (!pool_info) { + ret = -EINVAL; + goto err; + } + if (pool_info->resource_count == 0 || + pool_info->resource_count > RDMACG_MAX_RESOURCE_INDEX) { + ret = -EINVAL; + goto err; + } + + /* allocate resource pool */ + rpool = alloc_cg_rpool(cg, device, pool_info->resource_count, type); + if (IS_ERR_OR_NULL(rpool)) + return rpool; + + /* cgroup lock is held to synchronize with multiple + * resource pool creation in parallel. + */ + spin_lock(&cg->cg_list_lock); + other_rpool = find_cg_rpool(cg, device, type); + /* if other task added resource pool for this device for this cgroup + * free up which was recently created and use the one we found. + */ + if (other_rpool) { + atomic_inc(&other_rpool->refcnt); + spin_unlock(&cg->cg_list_lock); + _free_cg_rpool(rpool); + return other_rpool; + } + + atomic_inc(&rpool->refcnt); + list_add_tail(&rpool->cg_list, &cg->rpool_head); + + spin_unlock(&cg->cg_list_lock); + return rpool; + +err: + spin_unlock(&cg->cg_list_lock); + return ERR_PTR(ret); +} + +/** + * uncharge_cg_resource - hierarchically uncharge resource for rdma cgroup + * @cg: pointer to cg to uncharge and all parents in hierarchy + * @device: pointer to ib device + * @type: the type of resource pool to uncharge + * @index: index of the resource to uncharge in cg (resource pool) + * @num: the number of rdma resource to uncharge + * + * It also frees the resource pool in the hierarchy for the resource pool + * of default type which was created as part of charing operation. + */ +static void uncharge_cg_resource(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int index, int num) +{ + struct cg_resource_pool *rpool; + + rpool = _get_cg_rpool(cg, device, type); + /* + * A negative count (or overflow) is invalid, + * it indicates a bug in the rdma controller. + */ + WARN_ON_ONCE(atomic_add_negative(-num, + &rpool->resources[index].usage)); + put_cg_rpool(cg, rpool); +} + +/** + * rdmacg_uncharge_resource - hierarchically uncharge rdma resource count + * @device: pointer to rdmacg device + * @type: the type of resource pool to charge + * @index: index of the resource to uncharge in cg in given resource pool type + * @num: the number of rdma resource to uncharge + * + */ +void rdmacg_uncharge(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int index, int num) +{ + struct rdma_cgroup *p; + + for (p = cg; p; p = parent_rdmacg(p)) + uncharge_cg_resource(p, device, type, index, num); + + css_put(&cg->css); +} +EXPORT_SYMBOL(rdmacg_uncharge); + +/** + * try_charge_resource - hierarchically try to charge given resource + * @cg: pointer to cg to charge and all parents in hierarchy + * @device: pointer to rdmacg device + * @type: the type of resource pool to charge + * @index: index of the resource to charge in cg (resource pool) + * @num: the number of rdma resource to charge + * + * This function follows the set limit. It will fail if the charge would cause + * the new value to exceed the hierarchical limit. + * Returns 0 if the charge succeded, otherwise -EAGAIN. + * + * It allocates resource pool in the hierarchy for each parent it come + * across for first resource. Later on resource pool will be available. + * Therefore it will be much faster thereon to charge/uncharge. + */ +static int try_charge_resource(struct rdma_cgroup **rdmacg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int index, int num) +{ + struct cg_resource_pool *rpool; + struct rdma_cgroup *cg, *p, *q; + int ret; + + cg = task_rdmacg(current); + + for (p = cg; p; p = parent_rdmacg(p)) { + s64 new; + + rpool = get_cg_rpool(p, device, type); + if (IS_ERR_OR_NULL(rpool)) { + ret = PTR_ERR(rpool); + goto err; + } + + new = atomic_add_return(num, &rpool->resources[index].usage); + if (new > rpool->resources[index].max) { + ret = -EAGAIN; + goto revert; + } + } + /* hold on to the css, as cgroup can be removed but resource + * accounting happens on css. + */ + css_get(&cg->css); + *rdmacg = cg; + return 0; + +revert: + uncharge_cg_resource(p, device, type, index, num); +err: + for (q = cg; q != p; q = parent_rdmacg(q)) + uncharge_cg_resource(q, device, type, index, num); + return ret; +} + +/** + * rdmacg_try_charge_resource - hierarchically try to charge + * the rdma resource count + * @device: pointer to rdmacg device + * @rdmacg: pointer to rdma cgroup which will own this resource. + * @type: the type of resource pool to charge + * @index: index of the resource to charge in cg (resource pool) + * @num: the number of rdma resource to charge + * + * This function follows charing resource in hierarchical way. + * It will fail if the charge would cause the new value to exceed the + * hierarchical limit. + * Returns 0 if the charge succeded, otherwise -EAGAIN, -ENOMEM or -EINVAL. + * Returns pointer to rdmacg for this resource. + */ +int rdmacg_try_charge(struct rdma_cgroup **rdmacg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int index, int num) +{ + /* Charger needs to account resources on three criteria. + * (a) per cgroup (b) per device & (c) per resource type resource usage. + * Per cgroup resource usage ensures that tasks of cgroup doesn't cross + * the configured limits. + * Per device provides granular configuration in multi device usage. + * Per resource type allows resource charing for multiple + * category of resources - currently (a) verb level & (b) hw driver + * defined. + */ + + /* charge the cgroup resource pool */ + return try_charge_resource(rdmacg, device, type, index, num); +} +EXPORT_SYMBOL(rdmacg_try_charge); + +/** + * rdmacg_register_rdmacg_device - register rdmacg device to rdma controller. + * @device: pointer to rdmacg device whose resources need to be accounted. + * + * If IB stack wish a device to participate in rdma cgroup resource + * tracking, it must invoke this API to register with rdma cgroup before + * any user space application can start using the RDMA resources. IB stack + * and/or HCA driver must invoke rdmacg_set_rpool_ops() either for verb or + * for hw or for both the types as they are mandetory operations to have + * to register with rdma cgroup. + * + */ +void rdmacg_register_device(struct rdmacg_device *device, char *dev_name) +{ + INIT_LIST_HEAD(&device->rdmacg_list); + device->name = dev_name; + + mutex_lock(&dev_mutex); + list_add_tail(&device->rdmacg_list, &dev_list); + mutex_unlock(&dev_mutex); +} +EXPORT_SYMBOL(rdmacg_register_device); + +/** + * rdmacg_unregister_rdmacg_device - unregister the rdmacg device + * from rdma controller. + * @device: pointer to rdmacg device which was previously registered with rdma + * controller using rdmacg_register_device(). + * + * IB stack must invoke this after all the resources of the IB device + * are destroyed and after ensuring that no more resources will be created + * when this API is invoked. + */ +void rdmacg_unregister_device(struct rdmacg_device *device) +{ + /* Synchronize with any active resource settings, + * usage query happening via configfs. + * At this stage, there should not be any active resource pools + * for this device, as RDMA/IB stack is expected to shutdown, + * tear down all the applications and free up resources. + */ + mutex_lock(&dev_mutex); + list_del_init(&device->rdmacg_list); + mutex_unlock(&dev_mutex); +} +EXPORT_SYMBOL(rdmacg_unregister_device); + +/** + * rdmacg_set_rpool_ops - helper function to set the resource pool + * call back function to provide matching + * string tokens to for defining names of the + * resources. + * @device: pointer to rdmacg device for which to set the resource pool + * operations. + * @pool_type: resource pool type, either VERBS type or HW type. + * @ops: pointer to function pointers to return (a) matching token + * which will be invoked to find out string tokens and + * (b) to find out maximum resource limits that is supported + * by each device of given resource pool type. + * + * This helper function allows setting resouce pool specific operation + * callback functions. + * It must be called one by respective subsystem that implements resource + * definition of rdma and owns the task of charging/uncharing the resource. + * This must be called before ib device is registered with the rdma cgroup + * using rdmacg_register_rdmacg_device(). + */ +void rdmacg_set_rpool_ops(struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type, + struct rdmacg_resource_pool_ops *ops) +{ + device->rpool_ops[pool_type] = ops; +} +EXPORT_SYMBOL(rdmacg_set_rpool_ops); + +/** + * rdmacg_clear_rpool_ops - + * helper function to clear the resource pool ops which was setup + * before using rdmacg_set_rpool_ops. + * @device: pointer to ib device for which to clear the resource pool + * operations. + * @pool_type: resource pool type, either VERBS type or HW type. + * + * This must be called after deregistering ib device using rdma cgroup + * using rdmacg_unregister_rdmacg_device(). + */ +void rdmacg_clear_rpool_ops(struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type) +{ + device->rpool_ops[pool_type] = NULL; +} +EXPORT_SYMBOL(rdmacg_clear_rpool_ops); + +/** + * rdmacg_query_limit - query the resource limits that + * might have been configured by the user. + * @device: pointer to ib device + * @pid: thread group pid that wants to know the resource limits + * of the cgroup. + * @type: the type of resource pool to know the limits of. + * @limits: pointer to an array of limits where rdma cg will provide + * the configured limits of the cgroup. + * @limits_array_len: number of array elements to be filled up at limits. + * + * This function follows charing resource in hierarchical way. + * It will fail if the charge would cause the new value to exceed the + * hierarchical limit. + * Returns 0 if the charge succeded, otherwise appropriate error code. + */ +int rdmacg_query_limit(struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int *limits, int max_count) +{ + struct rdma_cgroup *cg, *p, *q; + struct cg_resource_pool *rpool; + struct rdmacg_pool_info *pool_info; + struct rdmacg_resource_pool_ops *ops; + int i, status = 0; + + cg = task_rdmacg(current); + + ops = get_pool_ops(device, type); + if (!ops) { + status = -EINVAL; + goto err; + } + pool_info = ops->get_resource_pool_tokens(device); + if (!pool_info) { + status = -EINVAL; + goto err; + } + if (pool_info->resource_count == 0 || + pool_info->resource_count > RDMACG_MAX_RESOURCE_INDEX || + max_count > pool_info->resource_count) { + status = -EINVAL; + goto err; + } + + /* initialize to max */ + for (i = 0; i < max_count; i++) + limits[i] = S32_MAX; + + /* check in hirerchy which pool get the least amount of + * resource limits. + */ + for (p = cg; p; p = parent_rdmacg(p)) { + /* get handle to cgroups rpool */ + rpool = get_cg_rpool(cg, device, type); + if (IS_ERR_OR_NULL(rpool)) + goto rpool_err; + + for (i = 0; i < max_count; i++) + limits[i] = min_t(int, limits[i], + rpool->resources[i].max); + + put_cg_rpool(cg, rpool); + } + return 0; + +rpool_err: + for (q = cg; q != p; q = parent_rdmacg(q)) + put_cg_rpool(cg, _get_cg_rpool(cg, device, type)); +err: + return status; +} +EXPORT_SYMBOL(rdmacg_query_limit); + +static int rdmacg_parse_limits(char *options, struct match_token *opt_tbl, + int *new_limits, u64 *enables) +{ + substring_t argstr[MAX_OPT_ARGS]; + const char *c; + int err = -ENOMEM; + + /* parse resource options */ + while ((c = strsep(&options, " ")) != NULL) { + int token, intval, ret; + + err = -EINVAL; + token = match_token((char *)c, opt_tbl, argstr); + if (token < 0) + goto err; + + ret = match_int(&argstr[0], &intval); + if (ret < 0) { + pr_err("bad value (not int) at '%s'\n", c); + goto err; + } + new_limits[token] = intval; + *enables |= BIT(token); + } + return 0; + +err: + return err; +} + +static enum rdmacg_resource_pool_type of_to_pool_type(int of_type) +{ + enum rdmacg_resource_pool_type pool_type; + + switch (of_type) { + case RDMACG_VERB_RESOURCE_MAX: + case RDMACG_VERB_RESOURCE_STAT: + pool_type = RDMACG_RESOURCE_POOL_VERB; + break; + case RDMACG_HW_RESOURCE_MAX: + case RDMACG_HW_RESOURCE_STAT: + default: + pool_type = RDMACG_RESOURCE_POOL_HW; + break; + }; + return pool_type; +} + +static struct rdmacg_device *_rdmacg_get_device(const char *name) +{ + struct rdmacg_device *device; + + list_for_each_entry(device, &dev_list, rdmacg_list) + if (!strcmp(name, device->name)) + return device; + + return NULL; +} + +static void remove_unused_cg_rpool(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type type, + int count) +{ + struct cg_resource_pool *rpool = NULL; + int i; + + spin_lock(&cg->cg_list_lock); + rpool = find_cg_rpool(cg, device, type); + if (!rpool) { + spin_unlock(&cg->cg_list_lock); + return; + } + /* found the resource pool, check now what to do + * based on its reference count. + */ + if (atomic_read(&rpool->refcnt) == 0) { + /* if there is no active user of the rpool, + * free the memory, default group will get + * allocated automatically when new resource + * is created. + */ + list_del_init(&rpool->cg_list); + + spin_unlock(&cg->cg_list_lock); + + _free_cg_rpool(rpool); + } else { + /* if there are active processes and thereby + * active resources, set limits to max. + * Resource pool will get freed later on, + * when last resource will get deallocated. + */ + for (i = 0; i < count; i++) + set_resource_limit(rpool, i, S32_MAX); + atomic_set(&rpool->creator, RDMACG_RPOOL_CREATOR_DEFAULT); + + spin_unlock(&cg->cg_list_lock); + } +} + +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 cg_resource_pool *rpool; + struct rdmacg_resource_pool_ops *ops; + struct rdmacg_device *device; + char *options = strstrip(buf); + enum rdmacg_resource_pool_type pool_type; + struct rdmacg_pool_info *resource_tokens; + u64 enables = 0; + int *new_limits; + int i = 0, ret = 0; + bool remove = false; + + /* extract the device name first */ + dev_name = strsep(&options, " "); + if (!dev_name) { + ret = -EINVAL; + goto err; + } + + /* check if user asked to remove the cgroup limits */ + if (strstr(options, RDMACG_USR_CMD_REMOVE)) + remove = true; + + new_limits = kcalloc(RDMACG_MAX_RESOURCE_INDEX, sizeof(int), + GFP_KERNEL); + if (!new_limits) { + ret = -ENOMEM; + goto opt_err; + } + + /* acquire lock to synchronize with hot plug devices */ + mutex_lock(&dev_mutex); + + device = _rdmacg_get_device(dev_name); + if (!device) { + ret = -ENODEV; + goto parse_err; + } + pool_type = of_to_pool_type(of_cft(of)->private); + ops = get_pool_ops(device, pool_type); + if (!ops) { + ret = -EINVAL; + goto parse_err; + } + + resource_tokens = ops->get_resource_pool_tokens(device); + if (IS_ERR_OR_NULL(resource_tokens)) { + ret = -EINVAL; + goto parse_err; + } + + if (remove) { + remove_unused_cg_rpool(cg, device, pool_type, + resource_tokens->resource_count); + /* user asked to clear the limits; ignore rest of the options */ + goto parse_err; + } + + /* user didn't ask to remove, act on the options */ + ret = rdmacg_parse_limits(options, + resource_tokens->resource_table, + new_limits, &enables); + if (ret) + goto parse_err; + + rpool = get_cg_rpool(cg, device, pool_type); + if (IS_ERR_OR_NULL(rpool)) { + if (IS_ERR(rpool)) + ret = PTR_ERR(rpool); + else + ret = -ENOMEM; + goto parse_err; + } + /* set pool type as user regardless of previous type as + * user is configuring the limit now. + */ + atomic_set(&rpool->creator, RDMACG_RPOOL_CREATOR_USR); + + /* now set the new limits on the existing or newly created rool */ + 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++; + } + atomic_dec(&rpool->refcnt); +parse_err: + mutex_unlock(&dev_mutex); +opt_err: + kfree(new_limits); +err: + return ret ?: nbytes; +} + +static int get_resource_val(struct cg_resource *resource, + enum rdmacg_file_type type) +{ + int val = 0; + + switch (type) { + case RDMACG_VERB_RESOURCE_MAX: + case RDMACG_HW_RESOURCE_MAX: + val = resource->max; + break; + case RDMACG_VERB_RESOURCE_STAT: + case RDMACG_HW_RESOURCE_STAT: + val = atomic_read(&resource->usage); + break; + default: + val = 0; + break; + }; + return val; +} + +static u32 *get_cg_rpool_values(struct rdma_cgroup *cg, + struct rdmacg_device *device, + enum rdmacg_resource_pool_type pool_type, + enum rdmacg_file_type resource_type, + int resource_count) +{ + struct cg_resource_pool *rpool; + u32 *value_tbl; + int i, ret; + + value_tbl = kcalloc(resource_count, sizeof(u32), GFP_KERNEL); + if (!value_tbl) { + ret = -ENOMEM; + goto err; + } + + rpool = get_cg_rpool(cg, device, pool_type); + if (IS_ERR_OR_NULL(rpool)) { + if (IS_ERR(rpool)) + ret = PTR_ERR(rpool); + else + ret = -ENOMEM; + goto err; + } + + for (i = 0; i < resource_count; i++) { + value_tbl[i] = get_resource_val(&rpool->resources[i], + resource_type); + } + put_cg_rpool(cg, rpool); + return value_tbl; + +err: + return ERR_PTR(ret); +} + +static int print_rpool_values(struct seq_file *sf, + struct rdmacg_pool_info *pool_info, + u32 *value_tbl) +{ + struct match_token *resource_table; + char *name; + int i, ret, name_len; + + resource_table = pool_info->resource_table; + + for (i = 0; i < pool_info->resource_count; i++) { + if (value_tbl[i] == S32_MAX) { + /* since we have to print max string, and token + * string cannot be changed, make a copy and print + * from there. + */ + name_len = strlen(resource_table[i].pattern); + name = kzalloc(name_len, GFP_KERNEL); + if (!name) { + ret = -ENOMEM; + goto err; + } + strcpy(name, resource_table[i].pattern); + + /* change to string instead of int from %d to %s */ + name[name_len - 1] = 's'; + seq_printf(sf, name, "max"); + kfree(name); + } else { + seq_printf(sf, resource_table[i].pattern, value_tbl[i]); + } + seq_putc(sf, ' '); + } + return 0; + +err: + return ret; +} + +static int rdmacg_resource_read(struct seq_file *sf, void *v) +{ + struct rdmacg_device *device; + struct rdma_cgroup *cg = css_rdmacg(seq_css(sf)); + struct rdmacg_pool_info *pool_info; + struct rdmacg_resource_pool_ops *ops; + u32 *value_tbl; + enum rdmacg_resource_pool_type pool_type; + int ret = 0; + + pool_type = of_to_pool_type(seq_cft(sf)->private); + + mutex_lock(&dev_mutex); + + list_for_each_entry(device, &dev_list, rdmacg_list) { + ops = get_pool_ops(device, pool_type); + if (!ops) + continue; + + pool_info = ops->get_resource_pool_tokens(device); + if (IS_ERR_OR_NULL(pool_info)) { + ret = -EINVAL; + goto err; + } + + /* get the value from resource pool */ + value_tbl = get_cg_rpool_values(cg, device, pool_type, + seq_cft(sf)->private, + pool_info->resource_count); + if (!value_tbl) { + ret = -ENOMEM; + goto err; + } + + seq_printf(sf, "%s ", device->name); + ret = print_rpool_values(sf, pool_info, value_tbl); + seq_putc(sf, '\n'); + + kfree(value_tbl); + if (ret) + break; + } + + mutex_unlock(&dev_mutex); + +err: + return ret; +} + +static struct cftype rdmacg_files[] = { + { + .name = "verb.max", + .write = rdmacg_resource_set_max, + .seq_show = rdmacg_resource_read, + .private = RDMACG_VERB_RESOURCE_MAX, + .flags = CFTYPE_NOT_ON_ROOT, + }, + { + .name = "verb.current", + .seq_show = rdmacg_resource_read, + .private = RDMACG_VERB_RESOURCE_STAT, + .flags = CFTYPE_NOT_ON_ROOT, + }, + { + .name = "hw.max", + .write = rdmacg_resource_set_max, + .seq_show = rdmacg_resource_read, + .private = RDMACG_HW_RESOURCE_MAX, + .flags = CFTYPE_NOT_ON_ROOT, + }, + { + .name = "hw.current", + .seq_show = rdmacg_resource_read, + .private = RDMACG_HW_RESOURCE_STAT, + .flags = CFTYPE_NOT_ON_ROOT, + }, + { } /* terminate */ +}; + +static struct cgroup_subsys_state * +rdmacg_css_alloc(struct cgroup_subsys_state *parent) +{ + struct rdma_cgroup *cg; + + cg = kzalloc(sizeof(*cg), GFP_KERNEL); + if (!cg) + return ERR_PTR(-ENOMEM); + + INIT_LIST_HEAD(&cg->rpool_head); + spin_lock_init(&cg->cg_list_lock); + + return &cg->css; +} + +static void rdmacg_css_free(struct cgroup_subsys_state *css) +{ + struct rdma_cgroup *cg = css_rdmacg(css); + + kfree(cg); +} + +/** + * rdmacg_css_offline - cgroup css_offline callback + * @css: css of interest + * + * This function is called when @css is about to go away and responsible + * for shooting down all rdmacg associated with @css. As part of that it + * marks all the resource pool as default type, so that when resources are + * freeded, associated resource pool can be freed as well. + * + */ +static void rdmacg_css_offline(struct cgroup_subsys_state *css) +{ + struct rdma_cgroup *cg = css_rdmacg(css); + struct cg_resource_pool *rpool; + + spin_lock(&cg->cg_list_lock); + + /* mark resource pool as of default type, so that they can be freed + * later on when actual resources are freed. + */ + list_for_each_entry(rpool, &cg->rpool_head, cg_list) + atomic_set(&rpool->creator, RDMACG_RPOOL_CREATOR_DEFAULT); + + spin_unlock(&cg->cg_list_lock); +} + +struct cgroup_subsys rdma_cgrp_subsys = { + .css_alloc = rdmacg_css_alloc, + .css_free = rdmacg_css_free, + .css_offline = rdmacg_css_offline, + .legacy_cftypes = rdmacg_files, + .dfl_cftypes = rdmacg_files, +};
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, per resource pool_type entity which allows setting up accounting limits on per device basis. RDMA cgroup returns error when user space applications try to allocate resources more than its configured limit. Rdma cgroup implements resource accounting for two types of resource pools. (a) RDMA IB specification level verb resources defined by IB stack (b) HCA vendor device specific resources defined by vendor device driver Resources are not defined by the RDMA cgroup, instead they are defined by the external module, typically IB stack and optionally by HCA drivers for those RDMA devices which doesn't have one to one mapping of IB verb resource with hardware resource. This allows extending IB stack without changing kernel, which is frequent as IB stack is going through changes and enhancements. Resource pool are 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 <pandit.parav@gmail.com> --- include/linux/cgroup_rdma.h | 80 ++++ include/linux/cgroup_subsys.h | 4 + init/Kconfig | 12 + kernel/Makefile | 1 + kernel/cgroup_rdma.c | 1021 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 1118 insertions(+) create mode 100644 include/linux/cgroup_rdma.h create mode 100644 kernel/cgroup_rdma.c