[Devel] [PATCH rh7 2/3] ve: Wait till ve's attaching work finished

[Kirill Tkhai]

On 27.10.2015 19:20, Vladimir Davydov wrote:
> On Fri, Oct 16, 2015 at 07:25:00PM +0300, Kirill Tkhai wrote:
> ...
>> @@ -1565,6 +1565,10 @@ struct task_struct {
>>  #ifdef CONFIG_VE
>>  	struct ve_struct *task_ve;
>>  	struct callback_head task_ve_work;
>> +	union {
>> +		struct task_struct *task_ve_mover;
>> +		atomic_t task_ve_nr_works;
>> +	};
> 
> Isn't it possible to allocate it before attaching a task and freeing
> them once we are done? Cgroup attach doesn't worth blowing task_struct.

I don't think it's better. Please, see my reply for the patch 1/3.

>>  #endif
>>  #ifdef CONFIG_MEMCG /* memcg uses this to do batch job */
>>  	struct memcg_batch_info {
> ...
>> @@ -703,11 +703,25 @@ static void ve_destroy(struct cgroup *cg)
>>  
>>  static void ve_attach_work(struct callback_head *head)
>>  {
>> +	atomic_t *nr = &current->task_ve_mover->task_ve_nr_works;
>> +
>>  	rcu_read_lock();
>>  	current->task_ve = cgroup_ve(task_cgroup(current, ve_subsys_id));
>>  	rcu_read_unlock();
>>  	smp_mb(); /* Pairs with smp_mb() in ve_can_attach() */
>>  	init_task_work(&current->task_ve_work, NULL);
>> +
>> +	atomic_dec(nr);
>> +}
>> +
>> +static void ve_wait_work(struct callback_head *head)
>> +{
>> +	atomic_t *nr = &current->task_ve_nr_works;
>> +
>> +	while (atomic_read(nr))
>> +		schedule_timeout(1);
> 
> Let's not introduce such temporary solutions. They look misleading and
> don't ease review. I think it'd be OK to merge patches 2 and 3.

It's not a new interface, it's just a stub. Could you please explain,
how it mislead a reviewer?

The thing is I splited patches 2 and 3 after I saw the big one is difficult
to review.
 
>> +
>> +	init_task_work(&current->task_ve_work, NULL);
>>  }
>>  
>>  static int ve_can_attach(struct cgroup *cg, struct cgroup_taskset *tset)