netdev - Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50390743.2090203@monom.org>
Date:	Sat, 25 Aug 2012 19:11:31 +0200
From:	Daniel Wagner <wagi@...om.org>
To:	Tejun Heo <tj@...nel.org>
CC:	netdev@...r.kernel.org, cgroups@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Gao feng <gaofeng@...fujitsu.com>,
	Glauber Costa <glommer@...allels.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Li Zefan <lizefan@...wei.com>,
	Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH v2 06/10] cgroup: Assign subsystem IDs during compile
 time

On 25.08.2012 01:38, Tejun Heo wrote:
> Hello,
>
> On Fri, Aug 24, 2012 at 04:01:40PM +0200, Daniel Wagner wrote:
>> We are able to safe some space when we assign the subsystem
>                   ^               ^
> 		 save    if we assign both builtin and
> 		         module susbsystem IDs at compile time?
>
>> IDs at compile time. Instead of allocating per cgroup
>> cgroup->subsys[CGROUP_SUBSYS_COUNT] where CGROUP_SUBSYS_COUNT is
>> always 64, we allocate at max 12 (at this point there are 12
>> subsystem).
>
> Please note (in big fat ugly way) that this disallows support for
> modular controller which isn't known at kernel compile time.

yep, will do

>> task_cls_classid() and task_netprioidx() (when built as
>> module) are protected by a jump label and therefore we can
>> simply replace the subsystem index lookup with the enum.
>
> Can we put these in a separate patch?  ie. The first patch makes all
> subsys IDs constant and then patches to simplify users.

Wouldn't this break bisection? I merged this step so that all steps in 
this series are able to compile and run.

>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 3787872..ada517f 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -43,18 +43,27 @@ extern void cgroup_unload_subsys(struct cgroup_subsys *ss);
>>
>>   extern const struct file_operations proc_cgroup_operations;
>>
>> -/* Define the enumeration of all builtin cgroup subsystems */
>> -#define SUBSYS(_x) _x ## _subsys_id,
>> +/*
>> + * Define the enumeration of all builtin cgroup subsystems.
>> + * For the builtin subsystems the subsys_id needs to be indentical
>> + * with the index in css->subsys. Therefore, all the builtin
>> + * subsys are listed first and then the modules ids.
>> + */
>>   enum cgroup_subsys_id {
>> +#define SUBSYS(_x) _x ## _subsys_id,
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_BUILTIN(option)
>>   #include <linux/cgroup_subsys.h>
>> -};
>> +#undef IS_SUBSYS_ENABLED
>> +
>> +#define IS_SUBSYS_ENABLED(option) IS_MODULE(option)
>> +#include <linux/cgroup_subsys.h>
>> +#undef IS_SUBSYS_ENABLED
>> +
>
> Why do we need to segregate in-kernel and modular ones at all?  What's
> wrong with just defining them in one go?

I have done that but the result was a panic. There seems some code which 
expects this ordering. Let me dig into this and fix it.

>> diff --git a/include/net/cls_cgroup.h b/include/net/cls_cgroup.h
>> index 24443d2..43fae13 100644
>> --- a/include/net/cls_cgroup.h
>> +++ b/include/net/cls_cgroup.h
>> @@ -49,22 +49,16 @@ static inline u32 task_cls_classid(struct task_struct *p)
>>   extern struct static_key cgroup_cls_enabled;
>>   #define clscg_enabled static_key_false(&cgroup_cls_enabled)
>>
>> -extern int net_cls_subsys_id;
>> -
>>   static inline u32 task_cls_classid(struct task_struct *p)
>>   {
>> -	int id;
>> -	u32 classid = 0;
>> +	u32 classid;
>>
>>   	if (!clscg_enabled || in_interrupt())
>>   		return 0;
>>
>>   	rcu_read_lock();
>> -	id = rcu_dereference_index_check(net_cls_subsys_id,
>> -					 rcu_read_lock_held());
>> -	if (id >= 0)
>> -		classid = container_of(task_subsys_state(p, id),
>> -				       struct cgroup_cls_state, css)->classid;
>> +	classid = container_of(task_subsys_state(p, net_cls_subsys_id),
>> +			       struct cgroup_cls_state, css)->classid;
>>   	rcu_read_unlock();
>>
>>   	return classid;
>
> Hmm... patch sequence looks odd to me.  If you first make all IDs
> constant, you can first remove module specific ones and then later add
> jump labels as separate patches.  Wouldn't that be simpler?

As said above, I tried to keep all steps usable so bisection would work. 
I think your steps would lead to non working versions of the kernel.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html