Re: [PATCH nf-next v3] netfilter: xtables: lightweight process control group matching

[Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

On 2013/12/28 16:44, Daniel Borkmann wrote:
> On 12/28/2013 09:17 AM, Daniel Borkmann wrote:
>> On 12/28/2013 02:33 AM, Li Zefan wrote:
>>> On 2013/12/27 16:05, Daniel Borkmann wrote:
>>>> On 12/27/2013 04:13 AM, Li Zefan wrote:
>> ...
>>>>> I'd suggest splitting cls_cgroup code into 2 parts. The first part
>>>>> is to manage cgroupfs and classid, and should be put into net/core/
>>>>> and add a new config like NET_CGROUP_CLASSID for it. The second part
>>>>> is specific cls_cgroup code.
>>>>
>>>> Sure, if this is wished, I'd do this as a follow-up as it doesn't affect
>>>> any of this code in netfilter here.
>>>
>>> We should clean up the code before introducing a new feature, not the
>>> other way.
>>
>> Hehe, quite honestly, I think this is YOUR opinion of a "cleanup", which
>> makes the code actually more complicated, and it's not strictly needed.
>>
>> If you are so desperate about this separation, fine, I will do this, but
>> again, I don't think it's *strictly* required for this.
> 

Currently we have:

	cls_cgroup

With your patch, it's changed to:

                               __  cls_cgroup
                             /
	net_cgroup_classid -
                             \ __  netfilter_cgroup

The code should be re-organized.

> Thinking about this further a bit, with this separation into net/core/, you
> would need another Kconfig, where people choose between built-in or
> module (or none).

Just disable module support for net_cgroup_classid. I never like the cgroup
feature that allows modular subsystems. It will be just about 100 lines of
code, so we gain little by making it modular. Futhermore with this change,
we can simplify include/linux/cls_cgroup.h

Moreoever cls_cgroup is the only modular cgroup subsystem, so I may even remove
this feature from cgroup core after the change to cls_cgroup.

> With built-in you would disallow people to load/unload the
> cgroup part during runtime. With the module, you would introduce a module
> dependency _for each_, even if you only want to use net_cls, which sort of
> is not what we want from a simple "cleanup".
>