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

[Daniel Borkmann <dborkman@redhat.com>]

On 12/28/2013 07:04 PM, Daniel Borkmann wrote:
> On 12/28/2013 10:20 AM, Li Zefan wrote:
>> 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.

Btw, if you plan to remove modular support at some point in time, there's still
netprio_cgroup on todo.

> Ok, will send a v4 with this change included, thanks.
> 
>>> 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".
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>