Re: [PATCH v2 3/4] sched/deadline: add the "set_flag" argument to cpudl_find()

[pang.xunlei]

On 20 November 2014 00:24, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Wed, 19 Nov 2014 23:46:21 +0800
> "pang.xunlei" <pang.xunlei@xxxxxxxxxx> wrote:
>
>> The call site of cpudl_find() in check_preempt_equal_dl() doesn't
>> use later_mask, so add this extra argument to distinquish the case.
>>
>> Signed-off-by: pang.xunlei <pang.xunlei@xxxxxxxxxx>
>> ---
>>  kernel/sched/cpudeadline.c |    6 ++++--
>>  kernel/sched/cpudeadline.h |    2 +-
>>  kernel/sched/deadline.c    |    6 +++---
>>  3 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c
>> index c01b3aa..3047846 100644
>> --- a/kernel/sched/cpudeadline.c
>> +++ b/kernel/sched/cpudeadline.c
>> @@ -98,11 +98,12 @@ static inline int cpudl_maximum(struct cpudl *cp)
>>   * @cp: the cpudl max-heap context
>>   * @p: the task
>>   * @later_mask: a mask to fill in with the selected CPUs (not NULL)
>> + * @set_flag: indicate if later_mask should be set
>>   *
>>   * Returns: int - best CPU (heap maximum if suitable)
>>   */
>>  int cpudl_find(struct cpudl *cp, struct task_struct *p,
>> -            struct cpumask *later_mask)
>> +            struct cpumask *later_mask, int set_flag)
>
> set_flag should be a bool type.
>
>>  {
>>       int best_cpu = -1;
>>       const struct sched_dl_entity *dl_se = &p->dl;
>> @@ -114,7 +115,8 @@ int cpudl_find(struct cpudl *cp, struct task_struct *p,
>>       } else if (cpumask_test_cpu(cpudl_maximum(cp), &p->cpus_allowed) &&
>>                       dl_time_before(dl_se->deadline, cp->elements[0].dl)) {
>>               best_cpu = cpudl_maximum(cp);
>> -             cpumask_set_cpu(best_cpu, later_mask);
>> +             if (set_flag)
>> +                     cpumask_set_cpu(best_cpu, later_mask);
>
> I'm not sure this is worth it. cpumask_set_cpu() is rather efficient.
HI Steve,

Thanks for your commenting, I've rethinked this a bit.
We can do a little trick with its return value, then could avoid this
extra cpumask_set_cpu() without this extra set_flag:
1) define macros for the return values of cpudl_find(), like:
#define    CPUDL_FIND_NONE          -2  /* no available cpus */
#define    CPUDL_FIND_CPUMASK   -1  /* available cpus in later_mask */

then, with the return value >=0, means it returns the only one available cpu.

2) In the leg of "if", it can just return CPUDL_FIND_CPUMASK, as we
want to select the best_cpu in find_later_rq().
In the leg of "else if", just returns cpudl_maximum(cp), apparently
there is no need to set the later_mask, since we will definitely
select this cpu as the best_cpu in find_later_rq() .

int cpudl_find(struct cpudl *cp, struct task_struct *p,
           struct cpumask *later_mask)
{
    const struct sched_dl_entity *dl_se = &p->dl;

    cpumask_and(later_mask, &p->cpus_allowed, &p->cpus_allowed);
    if (cpumask_and(later_mask, later_mask, cp->free_cpus)) {
        return CPUDL_FIND_CPUMASK;
    } else if (cpumask_test_cpu(cpudl_maximum(cp), &p->cpus_allowed) &&
                dl_time_before(dl_se->deadline, cp->elements[0].dl))
        int cpu;

        cpu = cpudl_maximum(cp);
        WARN_ON(!cpu_present(cpu));
        return cpu;
    }

out:

    return CPUDL_FIND_NONE;
}

Thus, in find_later_rq() we can change the call site code like:
    best_cpu = cpudl_find(&task_rq(task)->rd->cpudl, task,
                           later_mask);
    if (best_cpu == CPUDL_FIND_NONE)
        return -1;
    if (best_cpu != CPUDL_FIND_CPUMASK)
        return best_cpu;

    /* adjust the following code as that in RT find_lowest_rq(), omit here... */

What's your view about this?

Thanks,
Xunlei
>
>>       }
>>
>>  out:
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/