Re: [PATCH 1/2] kernel/{sched,smp}.c: fix static decl prior to struct declaration

[Ingo Molnar <mingo@elte.hu>]

* Jeff Garzik <jeff@garzik.org> wrote:

> Ingo Molnar wrote:
>> * Jeff Garzik <jeff@garzik.org> wrote:
>>
>>> According to C99 6.9.2p3, any declaration "static struct foo my_foo;"
>>> must follow the definition of struct foo.
>>>
>>> Apparently, gcc's lack of warning is a bug.
>>>
>>> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
>>
>> good spotting, i'll apply this fix, but:
>>
>>>   */
>>>  struct task_group root_task_group;
>>>  -#ifdef CONFIG_FAIR_GROUP_SCHED
>>> -/* Default task group's sched entity on each cpu */
>>> -static DEFINE_PER_CPU(struct sched_entity, init_sched_entity);
>>> -/* Default task group's cfs_rq on each cpu */
>>> -static DEFINE_PER_CPU(struct cfs_rq, init_cfs_rq) ____cacheline_aligned_in_smp;
>>> -#endif /* CONFIG_FAIR_GROUP_SCHED */
>>> -
>>> -#ifdef CONFIG_RT_GROUP_SCHED
>>> -static DEFINE_PER_CPU(struct sched_rt_entity, init_sched_rt_entity);
>>> -static DEFINE_PER_CPU(struct rt_rq, init_rt_rq) ____cacheline_aligned_in_smp;
>>> -#endif /* CONFIG_RT_GROUP_SCHED */
>>> -#else /* !CONFIG_USER_SCHED */
>>> -#define root_task_group init_task_group
>>> -#endif /* CONFIG_USER_SCHED */
>>> -
>>>  /* task_group_lock serializes add/remove of task groups and also changes to
>>
>> This block has a visible imbalance of preprocessor directives -  
>> there's a #else you moved, without moving the outermost #ifdef. My  
>> guess is that this will go down in a fireball with CONFIG_GROUP_SCHED 
>> disabled?
>
> It seems you are correct, I missed it in my 'allmodconfig' and 
> 'jeffs hw config' builds.
>
> Are you fixing this up, or should I regen the patch?

Please resend it. That area of the code is a bit of an #ifdef maze, 
hard to move around. (And i'd rather let you mess it up than mess it 
up myself - then i can blame it on you and bounce it back to you for 
fixing ;-)

Also, a style cleanup there might help a bit: adding a bit more 
geometric structure to the #ifdefs there might make the hierarchy 
stand out a bit more:

# ifdef CONFIG_RT_GROUP_SCHED
static DEFINE_PER_CPU(struct sched_rt_entity, init_sched_rt_entity);
static DEFINE_PER_CPU(struct rt_rq, init_rt_rq) ____cacheline_aligned_in_smp;
# endif /* CONFIG_RT_GROUP_SCHED */
#else /* !CONFIG_USER_SCHED */
# define root_task_group init_task_group
#endif /* CONFIG_USER_SCHED */

in such a lineup it's more apparent at a glance when a piece of code 
gets snatched in an incomplete way.

Thanks,

	Ingo