[Devel] [PATCH RHEL7 v17 07/13] ve/cgroup: Added ve_owner field to cgroup

[Kirill Tkhai]

On 23.04.2020 11:17, Kirill Tkhai wrote:
> On 22.04.2020 20:00, Valeriy Vdovin wrote:
>> Each cgroup representing a host or a container root of
>> cgroup subsystem hierarhy will have this field set to
>> a valid ve_struct, that owns this root. This way each
>> cgroup in a system will be able to know it's owning VE.
>> Non root cgroups will have this field set to NULL, this
>> is an optimization for cleanup code: at VE destruction
>> we only need to iterate over all root cgroups to clean
>> reference to former owning VE, rather than over all
>> cgroup hierarchy.
>> Still any cgroup that wants to know about it's owning
>> VE can find it's virtual root cgroup and read it's
>> ve_owner field.
>>
>> Signed-off-by: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
>> ---
>>  include/linux/cgroup.h |  4 ++++
>>  kernel/cgroup.c        | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 1bd0fe7..7ed718d 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -292,6 +292,9 @@ struct cgroup {
>>  	/* directory xattrs */
>>  	struct simple_xattrs xattrs;
>>  	u64 subgroups_limit;
>> +
>> +	/* ve_owner, responsible for running release agent. */
>> +	struct ve_struct *ve_owner;
> 
> In case of you decided to use rcu in below, __rcu prefix is required here.

I say this, because you have rcu_dereference() in cgroup_get_ve_owner().
I do not know you really need RCU. If you don't need, remove rcu_dereference()
from cgroup_get_ve_owner. If you need RCU, add a description in patch comment,
why so.

> 
>>  };
>>  
>>  #define MAX_CGROUP_ROOT_NAMELEN 64
>> @@ -637,6 +640,7 @@ int cgroup_task_count(const struct cgroup *cgrp);
>>  #ifdef CONFIG_VE
>>  void cgroup_mark_ve_roots(struct ve_struct *ve);
>>  void cgroup_unmark_ve_roots(struct ve_struct *ve);
>> +struct ve_struct *cgroup_get_ve_owner(struct cgroup *cgrp);
>>  #endif
>>  
>>  /*
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 010a23e..42f77c6 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -805,6 +805,42 @@ static struct cgroup_name *cgroup_alloc_name(struct dentry *dentry)
>>  	return name;
>>  }
>>  
>> +struct cgroup *cgroup_get_local_root(struct cgroup *cgrp)
>> +{
>> +	/*
>> +	 * Find nearest root cgroup, which might be host cgroup root
>> +	 * or ve cgroup root.
>> +	 *
>> +	 *    <host_root_cgroup> -> local_root
>> +	 *     \                    ^
>> +	 *      <cgroup>            |
>> +	 *       \                  |
>> +	 *        <cgroup>   --->   from here
>> +	 *        \
>> +	 *         <ve_root_cgroup> -> local_root
>> +	 *         \                   ^
>> +	 *          <cgroup>           |
>> +	 *          \                  |
>> +	 *           <cgroup>  --->    from here
>> +	 */
>> +	while (cgrp->parent && !test_bit(CGRP_VE_ROOT, &cgrp->flags))
>> +		cgrp = cgrp->parent;
>> +
>> +	return cgrp;
>> +}
>> +
>> +struct ve_struct *cgroup_get_ve_owner(struct cgroup *cgrp)
>> +{
>> +	struct ve_struct *ve;
>> +	/* Caller should hold RCU */
>> +
>> +	cgrp = cgroup_get_local_root(cgrp);
>> +	ve = rcu_dereference(cgrp->ve_owner);
>> +	if (!ve)
>> +		ve = get_ve0();
>> +	return ve;
>> +}
>> +
>>  static void cgroup_free_fn(struct work_struct *work)
>>  {
>>  	struct cgroup *cgrp = container_of(work, struct cgroup, destroy_work);
>> @@ -1647,6 +1683,7 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
>>  		const struct cred *cred;
>>  		int i;
>>  		struct css_set *cg;
>> +		root_cgrp->ve_owner = &ve0;
> 
> rcu_assign_pointer?
> 
>>  
>>  		BUG_ON(sb->s_root != NULL);
>>  
>> @@ -4254,12 +4291,14 @@ void cgroup_mark_ve_roots(struct ve_struct *ve)
>>  	mutex_lock(&cgroup_mutex);
>>  	for_each_active_root(root) {
>>  		cgrp = task_cgroup_from_root(ve->init_task, root);
>> +		rcu_assign_pointer(cgrp->ve_owner, ve);
>>  		set_bit(CGRP_VE_ROOT, &cgrp->flags);
>>  
>>  		if (test_bit(cpu_cgroup_subsys_id, &root->subsys_mask))
>>  			link_ve_root_cpu_cgroup(cgrp);
>>  	}
>>  	mutex_unlock(&cgroup_mutex);
>> +	synchronize_rcu();
>>  }
>>  
>>  void cgroup_unmark_ve_roots(struct ve_struct *ve)
>> @@ -4270,9 +4309,14 @@ void cgroup_unmark_ve_roots(struct ve_struct *ve)
>>  	mutex_lock(&cgroup_mutex);
>>  	for_each_active_root(root) {
>>  		cgrp = css_cgroup_from_root(ve->root_css_set, root);
>> +		BUG_ON(!rcu_dereference_protected(cgrp->ve_owner,
>> +				lockdep_is_held(&cgroup_mutex)));
>> +		rcu_assign_pointer(cgrp->ve_owner, NULL);
>>  		clear_bit(CGRP_VE_ROOT, &cgrp->flags);
>>  	}
>>  	mutex_unlock(&cgroup_mutex);
>> +	/* ve_owner == NULL will be visible */
>> +	synchronize_rcu();
>>  }
>>  
>>  struct cgroup *cgroup_get_ve_root(struct cgroup *cgrp)
>>
>