linux-kernel - Re: Test for CFS Bandwidth Control V6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4DEF0EB5.1090703@jp.fujitsu.com>
Date:	Wed, 08 Jun 2011 14:55:01 +0900
From:	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
To:	Paul Turner <pjt@...gle.com>
CC:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Bharata B Rao <bharata@...ux.vnet.ibm.com>,
	Dhaval Giani <dhaval.giani@...il.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
	Srivatsa Vaddagiri <vatsa@...ibm.com>,
	Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...e.hu>, Pavel Emelyanov <xemul@...nvz.org>
Subject: Re: Test for CFS Bandwidth Control V6

(2011/06/08 11:54), Paul Turner wrote:
> On Mon, May 23, 2011 at 5:53 PM, Hidetoshi Seto
> <seto.hidetoshi@...fujitsu.com> wrote:
>
>> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
>> index 3936393..544072f 100644
>> --- a/kernel/sched_fair.c
>> +++ b/kernel/sched_fair.c
>> @@ -1537,7 +1537,7 @@ static void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
>>        walk_tg_tree_from(cfs_rq->tg, tg_unthrottle_down, tg_nop,
>>                          (void *)&udd);
>>
>> -       if (!cfs_rq->load.weight)
>> +       if (!cfs_rq->h_nr_running)
>>                return;
>>
> 
> Why change here?

I've confused a bit - just curious if by any chance there is throttled
cfs_rq that have (load.weight, h_nr_running) = (0, >0).


>>        task_delta = cfs_rq->h_nr_running;
>> @@ -1843,10 +1843,9 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>>                cfs_rq->h_nr_running++;
>>
>>                /* end evaluation on throttled cfs_rq */
>> -               if (cfs_rq_throttled(cfs_rq)) {
>> -                       se = NULL;
> 
> Hmm..  yeah this is a casualty of moving the h_nr_running computations
> in-line as a part of the previous refactoring within the last
> releases.  This optimization (setting se = NULL to skip the second
> half) obviously won't work properly with detecting whether we made it
> to the end of the tree.
> 
(snip)
> 
> How about instead something like the following.  We can actually take
> advantage of the second loop always executing by deferring the
> accounting update on a throttle entity.  This keeps the control flow
> within dequeue_task_fair linear.
> 
> What do you think of (untested):
> 
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -1744,13 +1744,12 @@ enqueue_task_fair(struct rq *rq, struct
> task_struct *p, int flags)
>                         break;
>                 cfs_rq = cfs_rq_of(se);
>                 enqueue_entity(cfs_rq, se, flags);
> -               cfs_rq->h_nr_running++;
> 
> -               /* end evaluation on throttled cfs_rq */
> -               if (cfs_rq_throttled(cfs_rq)) {
> -                       se = NULL;
> +               /* note: ordering with throttle check to perform
> h_nr_running accounting on throttled entity below */
> +               if (cfs_rq_throttled(cfs_rq))
>                         break;
> -               }
> +
> +               cfs_rq->h_nr_running++;
>                 flags = ENQUEUE_WAKEUP;
>         }
> 
> @@ -1786,13 +1785,12 @@ static void dequeue_task_fair(struct rq *rq,
> struct task_struct *p, int flags)
>         for_each_sched_entity(se) {
>                 cfs_rq = cfs_rq_of(se);
>                 dequeue_entity(cfs_rq, se, flags);
> -               cfs_rq->h_nr_running--;
> 
> -               /* end evaluation on throttled cfs_rq */
> -               if (cfs_rq_throttled(cfs_rq)) {
> -                       se = NULL;
> +               /* note: ordering with throttle check to perform
> h_nr_running accounting on throttled entity below */
> +               if (cfs_rq_throttled(cfs_rq))
>                         break;
> -               }
> +
> +               cfs_rq->h_nr_running--;
>                 /* Don't dequeue parent if it has other entities besides

Looks good if it abides by the nature of scheduler codes ;-)


Thanks,
H.Seto

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