sched, cgroups: fix MIN_SHARES on 64 bit boxen

PATCH In review Mike Galbraith 2 participants 4 messages Jun 4, 2011 – Jul 1, 2011

Share & tools

Mike Galbraith

author

2011-06-04 09:30 UTC #0 lore raw

PATCH 1/1 sched, cgroups: fix MIN_SHARES on 64 bit boxen

c8b28116 claimed to have no user-visible effect, but allows setting cpu.shares to < MIN_SHARES, which the user then indeed sees.

Signed-off-by Mike Galbraith <[email protected]>

---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched.c +1 -1

Index: linux-2.6/kernel/sched.c
===================================================================
@@ -292,7 +292,7 @@ static DEFINE_SPINLOCK(task_group_lock);
  * (The default weight is 1024 - so there's no practical
  *  limitation from this.)
  */
-#define MIN_SHARES	2
+#define MIN_SHARES	(scale_load(2))
 #define MAX_SHARES	(1UL << (18 + SCHED_LOAD_RESOLUTION))
 
 static int root_task_group_load = ROOT_TASK_GROUP_LOAD;

Peter Zijlstra

2011-06-04 11:25 UTC #1 lore raw

Quoted 13 lines · Mike Galbraith

c8b28116 claimed to have no user-visible effect, but allows setting cpu.shares to < MIN_SHARES, which the user then indeed sees.
Signed-off-by Mike Galbraith <[email protected]>
---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/kernel/sched.c
===================================================================

kernel/sched.c static DEFINE_SPINLOCK(task_group_lock); · line 298

Quoted 8 lines

kernel/sched.c +1 -1

@@ -292,7 +292,7 @@ static DEFINE_SPINLOCK(task_group_lock);
  * (The default weight is 1024 - so there's no practical
  *  limitation from this.)
  */
-#define MIN_SHARES	2
+#define MIN_SHARES	(scale_load(2))
 #define MAX_SHARES	(1UL << (18 + SCHED_LOAD_RESOLUTION))
 
 static int root_task_group_load = ROOT_TASK_GROUP_LOAD;

Hurm, but that destroys most of the gains from that patch, the whole point was being able to have finer graunlarities, but now calc_cfs_shares() and effective_load() are clipped the coarse granularity.

So maybe explicitly change the MIN_SHARES usage in sched_group_set_shares(). That wants to become a clamp user anyway, something like:

shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));

That way MAX_SHARES can also loose its SCHED_LOAD_RESOLUTION factor and bring is back in line with MIN_SHARES.

Mike Galbraith

author

2011-06-04 13:03 UTC #2 lore raw

Quoted 21 lines · Peter Zijlstra

Quoted 20 lines · Mike Galbraith
c8b28116 claimed to have no user-visible effect, but allows setting cpu.shares to < MIN_SHARES, which the user then indeed sees.
Signed-off-by Mike Galbraith <[email protected]>
---
 kernel/sched.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/kernel/sched.c
===================================================================
kernel/sched.c +1 -1
@@ -292,7 +292,7 @@ static DEFINE_SPINLOCK(task_group_lock);
  * (The default weight is 1024 - so there's no practical
  *  limitation from this.)
  */
-#define MIN_SHARES	2
+#define MIN_SHARES	(scale_load(2))
 #define MAX_SHARES	(1UL << (18 + SCHED_LOAD_RESOLUTION))
 

Quoted 6 lines

kernel/sched.c static DEFINE_SPINLOCK(task_group_lock); · line 298
Quoted 1 line
kernel/sched.c +0 -0
 static int root_task_group_load = ROOT_TASK_GROUP_LOAD;
Hurm, but that destroys most of the gains from that patch, the whole point was being able to have finer graunlarities, but now calc_cfs_shares() and effective_load() are clipped the coarse granularity.

Oh well, drink one, spill one, give one away :)

Quoted 8 lines

So maybe explicitly change the MIN_SHARES usage in sched_group_set_shares(). That wants to become a clamp user anyway, something like:
shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));
That way MAX_SHARES can also loose its SCHED_LOAD_RESOLUTION factor and bring is back in line with MIN_SHARES.

sched, cgroups: fix sched_group_set_shares() on 64 bit boxen

c8b28116 claimed to have no user-visible effect, but allows setting cpu.shares to < MIN_SHARES, which the user then sees.

Signed-off-by Mike Galbraith <[email protected]>

---
 kernel/sched.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

kernel/sched.c +3 -6

Index: linux-2.6/kernel/sched.c
===================================================================
@@ -292,8 +292,8 @@ static DEFINE_SPINLOCK(task_group_lock);
  * (The default weight is 1024 - so there's no practical
  *  limitation from this.)
  */
-#define MIN_SHARES	2
-#define MAX_SHARES	(1UL << (18 + SCHED_LOAD_RESOLUTION))
+#define MIN_SHARES	(1UL <<  1)
+#define MAX_SHARES	(1UL << 18)
 
 static int root_task_group_load = ROOT_TASK_GROUP_LOAD;
 #endif

@@ -8439,10 +8439,7 @@ int sched_group_set_shares(struct task_g
 	if (!tg->se[0])
 		return -EINVAL;
 
-	if (shares < MIN_SHARES)
-		shares = MIN_SHARES;
-	else if (shares > MAX_SHARES)
-		shares = MAX_SHARES;
+	shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));
 
 	mutex_lock(&shares_mutex);
 	if (tg->shares == shares)

tip-bot for Mike Galbraith

author

2011-07-01 15:15 UTC #3 lore raw

[tip:sched/urgent] sched, cgroups: Fix MIN_SHARES on 64-bit boxen

Commit-ID:  cd62287e364c0d15d517c6ced4e4808b54711475
Gitweb:     http://git.kernel.org/tip/cd62287e364c0d15d517c6ced4e4808b54711475
Author:     Mike Galbraith <[email protected]>
AuthorDate: Sat, 4 Jun 2011 15:03:20 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 1 Jul 2011 10:25:03 +0200

sched, cgroups: Fix MIN_SHARES on 64-bit boxen

Commit c8b28116 ("sched: Increase SCHED_LOAD_SCALE resolution") intended to have no user-visible effect, but allows setting cpu.shares to < MIN_SHARES, which the user then sees.

Signed-off-by Mike Galbraith <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Nikhil Rao <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

kernel/sched.c +3 -6

index 3f2e502..9769c75 100644
@@ -292,8 +292,8 @@ static DEFINE_SPINLOCK(task_group_lock);
  * (The default weight is 1024 - so there's no practical
  *  limitation from this.)
  */
-#define MIN_SHARES	2
-#define MAX_SHARES	(1UL << (18 + SCHED_LOAD_RESOLUTION))
+#define MIN_SHARES	(1UL <<  1)
+#define MAX_SHARES	(1UL << 18)
 
 static int root_task_group_load = ROOT_TASK_GROUP_LOAD;
 #endif

@@ -8450,10 +8450,7 @@ int sched_group_set_shares(struct task_group *tg, unsigned long shares)
 	if (!tg->se[0])
 		return -EINVAL;
 
-	if (shares < MIN_SHARES)
-		shares = MIN_SHARES;
-	else if (shares > MAX_SHARES)
-		shares = MAX_SHARES;
+	shares = clamp(shares, scale_load(MIN_SHARES), scale_load(MAX_SHARES));
 
 	mutex_lock(&shares_mutex);
 	if (tg->shares == shares)