cgroup debug seems dead?

cgroup debug seems dead?

[Stephen Hemminger]

While running namespace checks to look for dead code,
I found that debug_subsys is declared global but never
used in current code.

This leads to the fact that since debug_subsys is never
used, the whole set of debugging functions is also never
used.  Is the whole CGROUP_DEBUG config option dead?

Re: cgroup debug seems dead?

[Li Zefan]

Stephen Hemminger wrote:
> While running namespace checks to look for dead code,
> I found that debug_subsys is declared global but never
> used in current code.
> 
> This leads to the fact that since debug_subsys is never
> used, the whole set of debugging functions is also never
> used.  Is the whole CGROUP_DEBUG config option dead?
> 

No, it's not.

The debug code has been moved from kernel/cgroup_debug.c to
kernel/cgroup.c, but the config and the code is not dead.

Is it a false positive of that check? The pointer to debug_subsys
is stored in an array, and the array will be iterated in some places,
so debug_subsys will be accessed.

Re: cgroup debug seems dead?

[Stephen Hemminger]

On Wed, 20 Oct 2010 09:11:10 +0800
Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:

> Stephen Hemminger wrote:
> > While running namespace checks to look for dead code,
> > I found that debug_subsys is declared global but never
> > used in current code.
> > 
> > This leads to the fact that since debug_subsys is never
> > used, the whole set of debugging functions is also never
> > used.  Is the whole CGROUP_DEBUG config option dead?
> > 
> 
> No, it's not.
> 
> The debug code has been moved from kernel/cgroup_debug.c to
> kernel/cgroup.c, but the config and the code is not dead.
> 
> Is it a false positive of that check? The pointer to debug_subsys
> is stored in an array, and the array will be iterated in some places,
> so debug_subsys will be accessed.

Where is it stored in an array?

~/kernel/linux-2.6$ git grep debug_subsys
kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
kernel/cgroup.c:        .subsys_id = debug_subsys_id,

There does not appear to be any place that structure is
referenced.
-- 

Re: cgroup debug seems dead?

[Paul Menage]

On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
<shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
>
> Where is it stored in an array?
>
> ~/kernel/linux-2.6$ git grep debug_subsys
> kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
> kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
> kernel/cgroup.c:        .subsys_id = debug_subsys_id,

It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c

Paul

Re: cgroup debug seems dead?

[Stephen Hemminger]

On Tue, 19 Oct 2010 18:14:10 -0700
Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
> <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
> >
> > Where is it stored in an array?
> >
> > ~/kernel/linux-2.6$ git grep debug_subsys
> > kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
> > kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
> > kernel/cgroup.c:        .subsys_id = debug_subsys_id,
> 
> It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c
> 
> Paul

That would work but doesn't because the following is missing
in cgroup_subsys.h!

#ifdef CONFIG_CGROUP_DEBUG
SUBSYS(debug)
#endif

/* */

Re: cgroup debug seems dead?

[Paul Menage]

On Tue, Oct 19, 2010 at 6:37 PM, Stephen Hemminger
<shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
>
> That would work but doesn't because the following is missing
> in cgroup_subsys.h!
>
> #ifdef CONFIG_CGROUP_DEBUG
> SUBSYS(debug)
> #endif
>
> /* */
>

I see that fragment at line 16 in cgroup_subsys.h (see e.g.
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/cgroup_subsys.h;h=ccefff02b6cb005123c4b904e06116cb3a850474;hb=HEAD)

Which kernel tree are you looking at?

Paul

Re: cgroup debug seems dead?

[Li Zefan]

Stephen Hemminger wrote:
> On Tue, 19 Oct 2010 18:14:10 -0700
> Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> 
>> On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
>> <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
>>> Where is it stored in an array?
>>>
>>> ~/kernel/linux-2.6$ git grep debug_subsys
>>> kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
>>> kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
>>> kernel/cgroup.c:        .subsys_id = debug_subsys_id,
>> It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c
>>
>> Paul
> 
> That would work but doesn't because the following is missing
> in cgroup_subsys.h!

It's there in cgroup_subsys.h (2.6.36-rc8). What kernel are you using?

#ifdef CONFIG_CPUSETS
SUBSYS(cpuset)
#endif

/* */

#ifdef CONFIG_CGROUP_DEBUG
SUBSYS(debug)
#endif

/* */

#ifdef CONFIG_CGROUP_NS
SUBSYS(ns)
#endif

> 
> #ifdef CONFIG_CGROUP_DEBUG
> SUBSYS(debug)
> #endif
> 

Re: cgroup debug seems dead?

[Stephen Hemminger]

On Wed, 20 Oct 2010 09:43:06 +0800
Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:

> Stephen Hemminger wrote:
> > On Tue, 19 Oct 2010 18:14:10 -0700
> > Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > 
> >> On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
> >> <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
> >>> Where is it stored in an array?
> >>>
> >>> ~/kernel/linux-2.6$ git grep debug_subsys
> >>> kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
> >>> kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
> >>> kernel/cgroup.c:        .subsys_id = debug_subsys_id,
> >> It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c
> >>
> >> Paul
> > 
> > That would work but doesn't because the following is missing
> > in cgroup_subsys.h!
> 
> It's there in cgroup_subsys.h (2.6.36-rc8). What kernel are you using?

Found it, thanks. Still not sure why the tools were confused.

Re: cgroup debug seems dead?

[Li Zefan]

Stephen Hemminger wrote:
> On Wed, 20 Oct 2010 09:43:06 +0800
> Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
> 
>> Stephen Hemminger wrote:
>>> On Tue, 19 Oct 2010 18:14:10 -0700
>>> Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>>> On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
>>>> <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
>>>>> Where is it stored in an array?
>>>>>
>>>>> ~/kernel/linux-2.6$ git grep debug_subsys
>>>>> kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
>>>>> kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
>>>>> kernel/cgroup.c:        .subsys_id = debug_subsys_id,
>>>> It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c
>>>>
>>>> Paul
>>> That would work but doesn't because the following is missing
>>> in cgroup_subsys.h!
>> It's there in cgroup_subsys.h (2.6.36-rc8). What kernel are you using?
> 
> Found it, thanks. Still not sure why the tools were confused.
> 

That's normal. Think about how many false positives we can get with
checkpatch.pl.

Your tools should be static code analyzers, and the macro we use in cgroup
is too complex for them.

Re: cgroup debug seems dead?

[Américo Wang]

On Wed, Oct 20, 2010 at 10:23:42AM +0800, Li Zefan wrote:
>Stephen Hemminger wrote:
>> On Wed, 20 Oct 2010 09:43:06 +0800
>> Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:
>> 
>>> Stephen Hemminger wrote:
>>>> On Tue, 19 Oct 2010 18:14:10 -0700
>>>> Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>
>>>>> On Tue, Oct 19, 2010 at 6:12 PM, Stephen Hemminger
>>>>> <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> Where is it stored in an array?
>>>>>>
>>>>>> ~/kernel/linux-2.6$ git grep debug_subsys
>>>>>> kernel/cgroup.c:        kfree(cont->subsys[debug_subsys_id]);
>>>>>> kernel/cgroup.c:struct cgroup_subsys debug_subsys = {
>>>>>> kernel/cgroup.c:        .subsys_id = debug_subsys_id,
>>>>> It's via the include of cgroup_subsys.h at the start of kernel/cgroup.c
>>>>>
>>>>> Paul
>>>> That would work but doesn't because the following is missing
>>>> in cgroup_subsys.h!
>>> It's there in cgroup_subsys.h (2.6.36-rc8). What kernel are you using?
>> 
>> Found it, thanks. Still not sure why the tools were confused.
>> 
>
>That's normal. Think about how many false positives we can get with
>checkpatch.pl.
>
>Your tools should be static code analyzers, and the macro we use in cgroup
>is too complex for them.

namespace.pl is not that static. Becuase it checks the binary not
the source, a little more dynamic that you think. ;)

Stephen, what message did you see?