[Devel] [PATCH v2 vz9] ve: cgroup -- don't use atomic lock in the sleepable context

[Pavel Tikhomirov]

On 10.11.2021 13:25, Pavel Tikhomirov wrote:
> Except for commit message and comment fixups looks good.
> 
> Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> 
> On 09.11.2021 13:07, Cyrill Gorcunov wrote:
>> We should not keep @css_set_lock spinlock while removing group files
>> which leads to
>>
>>   | BUG: sleeping function called from invalid context at 
>> kernel/locking/mutex.c:938
>>
>> This is because cgroup_rm_file() function takes @kernfs_mutex itself.
>> Since we're keeping the reference to cset in VE itself we don't even
>> need to take spinlock at all, because cset won't disappear until last
>> put_ve() call.
> 
> Cset would

not (sorry for missprint)

  be destroyed because of the reference and it also would not
> change (cgroup_destroy_root:list_del(&link->cgrp_link) is done holding 
> cgroup_mutex) because we have cgroup_mutex. The last part is probably 
> worth noted.
> 
>>
>> https://jira.sw.ru/browse/PSBM-135460
>>
>> Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
>> ---
>>   kernel/cgroup/cgroup.c |   13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Index: vzkernel/kernel/cgroup/cgroup.c
>> ===================================================================
>> --- vzkernel.orig/kernel/cgroup/cgroup.c
>> +++ vzkernel/kernel/cgroup/cgroup.c
>> @@ -2152,9 +2152,9 @@ void cgroup_unmark_ve_roots(struct ve_st
>>       struct cftype *cft;
>>       cft = get_cftype_by_name(CGROUP_FILENAME_RELEASE_AGENT);
>> +    BUG_ON(!cft || cft->file_offset);
>>       mutex_lock(&cgroup_mutex);
>> -    spin_lock_irq(&css_set_lock);
>>       /*
>>        * We can safely use ve->ve_ns without rcu_read_lock here, as we 
>> are
>> @@ -2163,19 +2163,26 @@ void cgroup_unmark_ve_roots(struct ve_st
>>        */
>>       cset = rcu_dereference_protected(ve->ve_ns,
>>               lockdep_is_held(&ve->op_sem))->cgroup_ns->root_cset;
>> +    BUG_ON(!cset);
>> +    /*
>> +     * Traversing @cgrp_links without @css_set_lock is safe here because
>> +     * we ourself are keeping the reference to the @cset so it won't
>> +     * disappear until the last reference get dropped upon ve 
>> destruction
>> +     * via put_ve() call.
> 
> I believe that put_ve puts other reference (ve cgroup). The reference to 
> ve->ve_ns->cgroup_ns->root_cset is put via 
> ve_drop_context:put_nsproxy(ve_ns). Please fix the comment.
> 
>> +     */
>>       list_for_each_entry(link, &cset->cgrp_links, cgrp_link) {
>>           cgrp = link->cgrp;
>>           if (!is_virtualized_cgroup(cgrp))
>>               continue;
>> -        cgroup_rm_file(cgrp, cft);
>> +        if (!cgroup_is_dead(cgrp))
>> +            cgroup_rm_file(cgrp, cft);
>>           rcu_assign_pointer(cgrp->ve_owner, NULL);
>>           clear_bit(CGRP_VE_ROOT, &cgrp->flags);
>>       }
>> -    spin_unlock_irq(&css_set_lock);
>>       mutex_unlock(&cgroup_mutex);
>>       /* ve_owner == NULL will be visible */
>>       synchronize_rcu();
>>
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.