Subject: Re: [PATCH v3 7/8] cgroup: Assign subsystem IDs during compile time

From: Daniel Wagner <[email-masked]>
Date: Tue, 11 Sep 2012 23:15:02 +0200

Hi Tejun,

On 09/11/2012 11:01 PM, Tejun Heo wrote:
> Hello, Daniel.
>
> I generally like this but I still think it's too big a patch.

Yes, I agree it is a bit too big.

>> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
>> index c75e3f9..6bc460c 100644
>> --- a/net/core/netprio_cgroup.c
>> +++ b/net/core/netprio_cgroup.c
>> @@ -326,9 +326,7 @@ struct cgroup_subsys net_prio_subsys = {
>>   	.create		= cgrp_create,
>>   	.destroy	= cgrp_destroy,
>>   	.attach		= net_prio_attach,
>> -#ifdef CONFIG_NETPRIO_CGROUP
>>   	.subsys_id	= net_prio_subsys_id,
>> -#endif
>>   	.base_cftypes	= ss_files,
>>   	.module		= THIS_MODULE
>>   };
>> @@ -366,10 +364,6 @@ static int __init init_cgroup_netprio(void)
>>   	ret = cgroup_load_subsys(&net_prio_subsys);
>>   	if (ret)
>>   		goto out;
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> -	smp_wmb();
>> -	net_prio_subsys_id = net_prio_subsys.subsys_id;
>> -#endif
>>
>>   	register_netdevice_notifier(&netprio_device_notifier);
>>
>> @@ -386,11 +380,6 @@ static void __exit exit_cgroup_netprio(void)
>>
>>   	cgroup_unload_subsys(&net_prio_subsys);
>>
>> -#ifndef CONFIG_NETPRIO_CGROUP
>> -	net_prio_subsys_id = -1;
>> -	synchronize_rcu();
>
> For example, it's not evident the above is correct and it's burried
> with all other changes.  Can't we just assign the fixed subsys ID to
> net_prio_subsys_id in this patch?  This patch would be correct without
> any netprio changes, no?

If net_prio_subsys_id is changed to be an enum, then the compiler will 
report an error:

error: lvalue required as left operand of assignment

that was the reason why I kept this change here. I think I just don't 
get what you are trying to tell me.

> Please separate these changes and explain them.

I will do that as soon I figured out what you are telling me.

> BTW, people who use barriers of any kind without explicitly explaining
> what's going on need to be kicked hard in the ass.  :(

I looked up the commit message when the synchronze_rcu() was added. It 
was not really explaining it. I really spend a few hours starring at 
this code.

thanks for the review,
daniel