Skip to contentSkip to conversation

Do not include throttled time as steal time

PATCH In review Mike wolf 2 participants 3 messages Dec 5, 2011

lore
Share & tools
lore

Mike wolf

author
#0 lore raw

PATCH 1/1 Do not include throttled time as steal time

When the linux kernel is running as the guest OS and is configured for bandwidth control and steal time reporting, it can be confusing to users to see the throttled time show up in the steal time stats. The user will think they are not getting the cpu resources they have been configured.

---
 kernel/sched_fair.c  |    4 ++--
 kernel/sched_stats.h |    7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)
kernel/sched_fair.c +2 -2
index 5c9e679..a837e4e 100644
@@ -707,7 +707,7 @@ account_entity_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se)
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 /* we need this in update_cfs_load and load-balance functions below */
-static inline int throttled_hierarchy(struct cfs_rq *cfs_rq);
+inline int throttled_hierarchy(struct cfs_rq *cfs_rq);
 # ifdef CONFIG_SMP
 static void update_cfs_rq_load_contribution(struct cfs_rq *cfs_rq,
                         int global_update)
@@ -1420,7 +1420,7 @@ static inline int cfs_rq_throttled(struct cfs_rq *cfs_rq)
 }
 
 /* check whether cfs_rq, or any parent, is throttled */
-static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
+inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
 {
     return cfs_rq->throttle_count;
 }
kernel/sched_stats.h +6 -1
index 87f9e36..e30ff26 100644
@@ -213,14 +213,19 @@ static inline void sched_info_queued(struct task_struct *t)
  * sched_info_queued() to mark that it has now again started waiting on
  * the runqueue.
  */
+extern inline int throttled_hierarchy(struct cfs_rq *cfs_rq);
 static inline void sched_info_depart(struct task_struct *t)
 {
+    struct task_group *tg = task_group(t);
+    struct cfs_rq *cfs_rq;
     unsigned long long delta = task_rq(t)->clock -
                     t->sched_info.last_arrival;
 
+    cfs_rq = tg->cfs_rq[smp_processor_id()];
     rq_sched_info_depart(task_rq(t), delta);
 
-    if (t->state == TASK_RUNNING)
+
+    if (t->state == TASK_RUNNING && !throttled_hierarchy(cfs_rq))
         sched_info_queued(t);
 }
 
#1 lore raw

On Mon, 2011-12-05 at 09:08 -0600, Mike wolf wrote:

Hi Mike, couple of problems with this:

1) You failed to CC the appropriate maintainers for the piece of code you're trying to have changed. When in doubt see the MAINTAINERS file ;-)

2) You failed to CC the people who wrote the feature you're having a problem with.

Quoted 5 lines

When the linux kernel is running as the guest OS and is configured for bandwidth control and steal time reporting, it can be confusing to users to see the throttled time show up in the steal time stats. The user will think they are not getting the cpu resources they have been configured.

Supposedly this is a BAD (tm) thing :-)

Quoted 5 lines
---
  kernel/sched_fair.c  |    4 ++--
  kernel/sched_stats.h |    7 ++++++-
  2 files changed, 8 insertions(+), 3 deletions(-)

3) You blink you loose, those files don't exist anymore. Patches are best provided against the development tree of the particular subsystem you're working against.

In this particular case tip/master is your target.

Quoted 5 lines
  static inline void sched_info_depart(struct task_struct *t)
  {
kernel/sched_stats.h +2 -0
+    struct task_group *tg = task_group(t);
+    struct cfs_rq *cfs_rq;
      unsigned long long delta = task_rq(t)->clock -

kernel/sched_stats.h static inline void sched_info_queued(struct task_struct *t) · line 230

Quoted 10 lines
kernel/sched_stats.h +3 -1
                      t->sched_info.last_arrival;
 
+    cfs_rq = tg->cfs_rq[smp_processor_id()];
      rq_sched_info_depart(task_rq(t), delta);
 
-    if (t->state == TASK_RUNNING)
+
+    if (t->state == TASK_RUNNING && !throttled_hierarchy(cfs_rq))
          sched_info_queued(t);
  }

4) so there's a lot more steal time crap all over the scheduler, you failed to explain why only this particular bit is important enough to change.

author
#2 lore raw
Quoted 7 lines · Peter Zijlstra

On Mon, 2011-12-05 at 09:08 -0600, Mike wolf wrote:

Hi Mike, couple of problems with this:

1) You failed to CC the appropriate maintainers for the piece of code you're trying to have changed. When in doubt see the MAINTAINERS file ;-)

yes will do

Quoted 2 lines

2) You failed to CC the people who wrote the feature you're having a problem with.

ok, I will add Glauber and others when the patch is respun/resubmitted

Quoted 28 lines
Quoted 5 lines

When the linux kernel is running as the guest OS and is configured for bandwidth control and steal time reporting, it can be confusing to users to see the throttled time show up in the steal time stats. The user will think they are not getting the cpu resources they have been configured.

Supposedly this is a BAD (tm) thing :-)

Quoted 5 lines
---
   kernel/sched_fair.c  |    4 ++--
   kernel/sched_stats.h |    7 ++++++-
   2 files changed, 8 insertions(+), 3 deletions(-)

3) You blink you loose, those files don't exist anymore. Patches are best provided against the development tree of the particular subsystem you're working against.

In this particular case tip/master is your target.

Quoted 10 lines
   static inline void sched_info_depart(struct task_struct *t)
   {
kernel/sched_stats.h +3 -0
+    struct task_group *tg = task_group(t);
+    struct cfs_rq *cfs_rq;
       unsigned long long delta = task_rq(t)->clock -
                       t->sched_info.last_arrival;
 
+    cfs_rq = tg->cfs_rq[smp_processor_id()];
       rq_sched_info_depart(task_rq(t), delta);
Quoted 8 lines

kernel/sched_stats.h static inline void sched_info_queued(struct task_struct *t) · line 230

Quoted 5 lines
kernel/sched_stats.h +2 -1
-    if (t->state == TASK_RUNNING)
+
+    if (t->state == TASK_RUNNING&&  !throttled_hierarchy(cfs_rq))
           sched_info_queued(t);
   }

4) so there's a lot more steal time crap all over the scheduler, you failed to explain why only this particular bit is important enough to change.

I will make sure to explain why you would want the patch functionality better when I resubmit.

Quoted 2 lines