Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[Srivatsa Vaddagiri]

On Thu, Feb 09, 2006 at 03:11:47PM +0900, KUROSAWA Takahiro wrote:
> This patch adds CPU resource controller.  It enables us to control
> CPU time percentage of tasks grouped by the cpu_rc structure.
> It controls time_slice of tasks based on the feedback of difference
> between the target value and the current usage in order to control
> the percentage of the CPU usage to the target value.

I noticed some anomalies in guarantees that were provided to different classes.
Basically I had created two classes CA (10% guarantee) and CB (90% guarantee) 
and run few tasks on a 4-cpu system as below:

Case 1:

	CPU0	CPU1	CPU2	CPU3
	============================

	TA1	TA2	TA3	TA4
	TB1	TB2	TB3	TB4

Case 2:
	
	CPU0	CPU1	CPU2	CPU3
	============================

	TA1	TA2	TA3	TA4
	               		TB4

TA* tasks belong to CA and TB* belong to CB. All are CPU hungry tasks. Also 
each task is bound to the respective CPU indicated.

In both above cases, I found that CPU time was *equally* shared between the two 
classes (whereas I expected them to be shared in 1:9 ratio).

> +void cpu_rc_collect_hunger(task_t *tsk)
> +{

[snip]

> +	if (CPU_RC_GUAR_SCALE * tsk->last_slice	/ (wait + tsk->last_slice)
> +			< cr->guarantee / cr->rcd->numcpus)
					^^^^^^^^^^^^^^^^^^
					
Debugging it a bit indicated that the division of cr->guarantee by 
cr->rcd->numcpus in cpu_rc_collect_hunger doesn't seem to be required (since 
LHS is not on global scale and also the class's tasks may not be running
on other CPUs as in case 2). Removing the division rectified CPU sharing 
anomaly I had found.

Let me know what you think of this fix!


--- kernel/cpu_rc.c.org	2006-02-11 08:44:38.000000000 +0530
+++ kernel/cpu_rc.c	2006-02-13 18:34:30.000000000 +0530
@@ -204,7 +204,7 @@ void cpu_rc_collect_hunger(task_t *tsk)
 
 	wait = jiffies - tsk->last_activated;
 	if (CPU_RC_GUAR_SCALE * tsk->last_slice	/ (wait + tsk->last_slice)
-			< cr->guarantee / cr->rcd->numcpus)
+			< cr->guarantee)
 		cr->stat[cpu].maybe_hungry++;
 
 	tsk->last_activated = 0;



-- 
Regards,
vatsa

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[KUROSAWA Takahiro]

On Mon, 13 Feb 2006 20:03:45 +0530
Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:

> > +void cpu_rc_collect_hunger(task_t *tsk)
> > +{
> 
> [snip]
> 
> > +	if (CPU_RC_GUAR_SCALE * tsk->last_slice	/ (wait + tsk->last_slice)
> > +			< cr->guarantee / cr->rcd->numcpus)
> 					^^^^^^^^^^^^^^^^^^
> 					
> Debugging it a bit indicated that the division of cr->guarantee by 
> cr->rcd->numcpus in cpu_rc_collect_hunger doesn't seem to be required (since 
> LHS is not on global scale and also the class's tasks may not be running
> on other CPUs as in case 2). Removing the division rectified CPU sharing 
> anomaly I had found.
> 
> Let me know what you think of this fix!

Ah, you are right.  LHS is on per-cpu scale.
I'll apply your patch.

> --- kernel/cpu_rc.c.org	2006-02-11 08:44:38.000000000 +0530
> +++ kernel/cpu_rc.c	2006-02-13 18:34:30.000000000 +0530
> @@ -204,7 +204,7 @@ void cpu_rc_collect_hunger(task_t *tsk)
>  
>  	wait = jiffies - tsk->last_activated;
>  	if (CPU_RC_GUAR_SCALE * tsk->last_slice	/ (wait + tsk->last_slice)
> -			< cr->guarantee / cr->rcd->numcpus)
> +			< cr->guarantee)
>  		cr->stat[cpu].maybe_hungry++;
>  
>  	tsk->last_activated = 0;

Thanks,

-- 
KUROSAWA, Takahiro

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[Srivatsa Vaddagiri]

On Tue, Feb 14, 2006 at 08:55:29AM +0900, KUROSAWA Takahiro wrote:
> Ah, you are right.  LHS is on per-cpu scale.
> I'll apply your patch.

Great! I also feel that "guarantee" can be explained better (in your first 
documentation patch), especially in the context of multi-cpu systems. Initially 
I was confused what guarantee means in SMP.

-- 
Regards,
vatsa

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[KUROSAWA Takahiro]

Hi,

On Tue, 14 Feb 2006 10:30:13 +0530
"Suryanarayanan, Rajaram" <Rajaram.Suryanarayanan@in.unisys.com> wrote:

> Is this patch under discussion a new CPU resource controller for 2.6.15
> ?
> Or is it the modified version of the already existing resource
> controller for CKRM ?

It's not a forward port of the existing CPU resource controller 
but a new CPU resource controller for CKRM.  Its resource control
mechanism is different from that of the existing one.

> I just want to know if you are coming up with a different idea for CPU
> resource controller and whether we have choice between the existing
> resource controller and this new one ? 

If someone forward-ports the existing controller for CKRM f- version,
we will have a choice.

Regards,
-- 
KUROSAWA, Takahiro

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[Srivatsa Vaddagiri]

On Tue, Feb 14, 2006 at 10:30:13AM +0530, Suryanarayanan, Rajaram wrote:
> Is this patch under discussion a new CPU resource controller for 2.6.15
> ?

No, the patch does not talk of a new controller. The patch only fixes some 
problem with the existing controller. Hope this clarifies!

-- 
Regards,
vatsa

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[Srivatsa Vaddagiri]

On Tue, Feb 14, 2006 at 02:26:04PM +0900, KUROSAWA Takahiro wrote:
> It's not a forward port of the existing CPU resource controller 
> but a new CPU resource controller for CKRM.  Its resource control
> mechanism is different from that of the existing one.

Hmm ..I guess it depends on which version of CKRM you are referring when
you say "existing". When I replied earlier, I was referring to f-series.
f-series cpu controller is based on the patch you sent to lkml right?

-- 
Regards,
vatsa

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[KUROSAWA Takahiro]

On Tue, 14 Feb 2006 11:05:48 +0530
"Suryanarayanan, Rajaram" <Rajaram.Suryanarayanan@in.unisys.com> wrote:

> Thanks Kurosawa. I am not able to get your second point.
> I want to understand whether people will be able to choose and use any
> one resource controller among these two ? Will both exist as part of
> CKRM in future ? Also if anyone comes up with another new resource
> controller in future, will it also be added to the existing choices ?

Hmm...
I suppose that the project manager of CKRM (Chandra?) will decide which 
(or both) resource controller should be included in the CKRM.

-- 
KUROSAWA, Takahiro

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[KUROSAWA Takahiro]

On Tue, 14 Feb 2006 11:08:52 +0530
Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:

> > It's not a forward port of the existing CPU resource controller 
> > but a new CPU resource controller for CKRM.  Its resource control
> > mechanism is different from that of the existing one.
> 
> Hmm ..I guess it depends on which version of CKRM you are referring when
> you say "existing". When I replied earlier, I was referring to f-series.
> f-series cpu controller is based on the patch you sent to lkml right?

Ah, I referred to the CKRM e- series controller as "existing controller"
and referred to the controller that I had sent as "a new CPU resource 
controller."

So, the controller that I had sent is the existing controller of
f- series.

Sorry for confusion,

-- 
KUROSAWA, Takahiro

Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[KUROSAWA Takahiro]

On Tue, 14 Feb 2006 11:36:56 +0530
"Suryanarayanan, Rajaram" <Rajaram.Suryanarayanan@in.unisys.com> wrote:

> I have downloaded the CPU resource controller for 2.6.14 ( f-series
> cpurc_v0.3-2614 ). So is your resource controller built on top of this ?

The patch that I had sent is the forward port of cpurc_v0.3-2614.

> Has the logic of CPU resource controller been changed in 2.6.15 than
> what I have with me for 2.6.14 ?

No, the resource control logic is the same as cpurc_v0.3-2614.

-- 
KUROSAWA, Takahiro

RE: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

[Suryanarayanan, Rajaram]

  	
-----Original Message-----
From: KUROSAWA Takahiro [mailto:kurosawa@valinux.co.jp] 
Sent: Tuesday, February 14, 2006 11:28 AM
To: vatsa@in.ibm.com
Cc: Suryanarayanan, Rajaram; linux-kernel@vger.kernel.org;
ckrm-tech@lists.sourceforge.net; balbir.singh@in.ibm.com
Subject: Re: [ckrm-tech] [PATCH 1/2] add a CPU resource controller

On Tue, 14 Feb 2006 11:08:52 +0530
Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:

> > It's not a forward port of the existing CPU resource controller 
> > but a new CPU resource controller for CKRM.  Its resource control
> > mechanism is different from that of the existing one.
> 
> Hmm ..I guess it depends on which version of CKRM you are referring
when
> you say "existing". When I replied earlier, I was referring to
f-series.
> f-series cpu controller is based on the patch you sent to lkml right?

Ah, I referred to the CKRM e- series controller as "existing controller"
and referred to the controller that I had sent as "a new CPU resource 
controller."

So, the controller that I had sent is the existing controller of
f- series.

Sorry for confusion,

-- 
KUROSAWA, Takahiro


Thanks for the clarification.
I was actually confused by the words "This patchset adds a CPU resource
controller for CKRM" in the earlier mail. I thought this is some new
controller..
Currently I have downloaded cpurc_v0.3-2614 and plan to look in to it
for understanding CKRM. Later I guess I can catch up with the current
changes that you are making..

Regards,
Rajaram Suryanarayanan | Team Lead | ES7000 Linux Systems Group    
Unisys Global Services -India | 
'Purva Premier', 135/1, Residency Road, | Bangalore - 560 025 |
+91-80-51594560
Do your best. Leave the rest to Linux.