Re: [PATCH v2 1/2] sched: Nominate idle load balancer from a semi-idle package.

[Randy Dunlap <randy.dunlap@oracle.com>]

Gautham R Shenoy wrote:
> Hi Jaswinder,
> Thanks for the review. Comments interspersed.
> 
> 
> On Thu, Apr 02, 2009 at 09:22:11PM +0530, Jaswinder Singh Rajput wrote:
>> Here are some minor issues:
>>> diff --git a/kernel/sched.c b/kernel/sched.c
>>> index 706517c..4fc1ec0 100644
>>> --- a/kernel/sched.c
>>> +++ b/kernel/sched.c
> 
>>>  } nohz ____cacheline_aligned = {
>>>  	.load_balancer = ATOMIC_INIT(-1),
>>>  };
>>>  
>>> +#if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
>>> +/**
>> ^^^^^^
>> This comment is not valid and even Randy send patches to fix these
>> comments and also shared the error messages because of these comments by
>> your earlier patches. Replace it with /*
> 
> I think you're referring to this: http://lkml.org/lkml/2009/3/29/7
> I had missed this patch. Thanks for pointing it out.
> 
> I think Randy's patch was addressing the issue of structs
> not requiring a /** style comments. But in this case,
> it's a function, and hence needs kernel-doc style notation. Or am I
> missing something here ? Point about the single-line
> short function description is well taken.

If you want to use kernel-doc for functions or structs or unions
or enums, do so.  Just use the correct syntax, please.

If it's not kernel-doc, don't use /** to begin the comment block.


>>> + * lowest_flag_domain: Returns the lowest sched_domain
>>> + * that has the given flag set for a particular cpu.
>>> + * @cpu: The cpu whose lowest level of sched domain is to
>>> + * be returned.
>>> + *
>>> + * @flag: The flag to check for the lowest sched_domain
>>> + * for the given cpu
>>> + */
>>> +static inline struct sched_domain *lowest_flag_domain(int cpu, int flag)
>>> +{
>>> +	struct sched_domain *sd;
>>> +
>>> +	for_each_domain(cpu, sd)
>>> +		if (sd && (sd->flags & flag))
>>> +			break;
>>> +
>>> +	return sd;
>>> +}
>>> +
>>> +/**
>> Ditto.
>>
> 
> Ditto :-)
> 
>>> + * for_each_flag_domain: Iterates over all the scheduler domains
>>> + * for a given cpu that has the 'flag' set, starting from
>>> + * the lowest to the highest.
>>> + * @cpu: The cpu whose domains we're iterating over.
>>> + * @sd: variable holding the value of the power_savings_sd
>>> + * for cpu
>> This can be come in one line:
> Agreed. Will correct this.
> 
>> + * @sd: variable holding the value of the power_savings_sd for cpu
>>
>>> + */
>>> +#define for_each_flag_domain(cpu, sd, flag) \
>>> +	for (sd = lowest_flag_domain(cpu, flag); \
>>> +		(sd && (sd->flags & flag)); sd = sd->parent)
>>> +
>>> +static inline int is_semi_idle_group(struct sched_group *ilb_group)
>>> +{
>>> +	cpumask_and(nohz.tmpmask, nohz.cpu_mask, sched_group_cpus(ilb_group));
>>> +
>>> +	/*
>>> +	 * A sched_group is semi-idle when it has atleast one busy cpu
>>> +	 * and atleast one idle cpu.
>>> +	 */
>>> +	if (!(cpumask_empty(nohz.tmpmask) ||
>>> +		cpumask_equal(nohz.tmpmask, sched_group_cpus(ilb_group))))
>>> +		return 1;
>>> +
>>> +	return 0;
>>> +}
>>> +/**
>> Ditto.
>>
> 
> Ditto!
>>> + * find_new_ilb: Finds or nominates a new idle load balancer.
>>> + * @cpu: The cpu which is nominating a new idle_load_balancer.
>>> + *
>>> + * This algorithm picks the idle load balancer such that it belongs to a
>>> + * semi-idle powersavings sched_domain. The idea is to try and avoid
>>> + * completely idle packages/cores just for the purpose of idle load balancing
>>> + * when there are other idle cpu's which are better suited for that job.
>>> + */
>>> +static int find_new_ilb(int cpu)


-- 
~Randy