sched: Accelerate "pick_next_entity" under special condition

From: Michael Wang <[email protected]>

We can avoid some useless operation in some special condition.

For example:
If we have "cfs_rq->next" and it can be use, we just return it directly.

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

kernel/sched/fair.c +0 -0

index 84adb2d..9fc2c3c 100644

kernel/sched/fair.c +16 -11

+
 /*
  * Pick the next process, keeping these things in mind, in this order:
  * 1) keep things fair between processes/task groups

@@ -1308,29 +1310,33 @@ static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
 	struct sched_entity *left = se;
 
 	/*
-	 * Avoid running the skip buddy, if running something else can
-	 * be done without getting too unfair.
+	 * Someone really wants this to run. If it's not unfair, run it.
 	 */
-	if (cfs_rq->skip == se) {
-		struct sched_entity *second = __pick_next_entity(se);
-		if (second && wakeup_preempt_entity(second, left) < 1)
-			se = second;
+	if (cfs_rq->next && ENTITY_PREEMPT_ALLOWED(cfs_rq->next, left)) {
+		se = cfs_rq->next;
+		goto out;
 	}
 
 	/*
 	 * Prefer last buddy, try to return the CPU to a preempted task.
 	 */
-	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
+	if (cfs_rq->last && ENTITY_PREEMPT_ALLOWED(cfs_rq->last, left)) {
 		se = cfs_rq->last;
+		goto out;
+	}
 
 	/*
-	 * Someone really wants this to run. If it's not unfair, run it.
+	 * Avoid running the skip buddy, if running something else can
+	 * be done without getting too unfair.
 	 */
-	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
-		se = cfs_rq->next;
+	if (cfs_rq->skip == se) {
+		struct sched_entity *second = __pick_next_entity(se);
+		if (second && ENTITY_PREEMPT_ALLOWED(second, left))
+			se = second;
+	}
 
+out:
 	clear_buddies(cfs_rq, se);
-
 	return se;
 }
 

From: Michael Wang <[email protected]>

We can avoid some useless operation in some special condition.

For example:
If we have "cfs_rq->next" and it can be use, we just return it directly.

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

kernel/sched/fair.c +0 -0

index 84adb2d..9fc2c3c 100644

Quoted 3 lines · Michael Wang

From: Michael Wang <[email protected]>

We can avoid some useless operation in some special condition.

This is a pretty empty statement.

Quoted 2 lines

For example:
If we have "cfs_rq->next" and it can be use, we just return it directly.

What it doesn't state is what it actually does, if it affects the common case and performance numbers (or a good reason for the lack thereof).

Quoted 13 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

kernel/sched/fair.c +0 -0

index 84adb2d..9fc2c3c 100644
@@ -1295,6 +1295,8 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
 static int
 wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se);
 

kernel/sched/fair.c set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se) · line 1298

Quoted 1 line

kernel/sched/fair.c +1 -0

+#define ENTITY_PREEMPT_ALLOWED(prev,next)	(wakeup_preempt_entity(prev, next) < 1)

This is just uglification imo, its shouting and it doesn't actually win you much space.

From: wangyun <[email protected]>

In original code, we get the next entity in this way:

	if(condition1)
		result=value1;
	if(condition2)
		result=value2;
	if(condition3)
		result=value3;
	return result;

So if condition3 is true, we will get value3, but still need to check condition1 and condition2, this will waste our time.

This patch will change the way like:

	if(condition3) {
		result=value3;
		goto out;
	}
	if(condition2) {
		result=value2;
		goto out;
	}
	if(condition1) {
		result=value1;
		goto out;
	}

	out:
	return result;

So we can avoid check condition2 and condition1 when condition3 is true now.

v2:
1. do not use ugly macro any more.
2. add more description.

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

kernel/sched/fair.c +15 -11

index 84adb2d..e8a72b2 100644
@@ -1308,29 +1308,33 @@ static struct sched_entity
 *pick_next_entity(struct cfs_rq *cfs_rq)
 	struct sched_entity *left = se;
 
 	/*
-	 * Avoid running the skip buddy, if running something else can
-	 * be done without getting too unfair.
+	 * Someone really wants this to run. If it's not unfair, run it.
 	 */
-	if (cfs_rq->skip == se) {
-		struct sched_entity *second = __pick_next_entity(se);
-		if (second && wakeup_preempt_entity(second, left) < 1)
-			se = second;
+	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1) {
+		se = cfs_rq->next;
+		goto out;
 	}
 
 	/*
 	 * Prefer last buddy, try to return the CPU to a preempted task.
 	 */
-	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
+	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1) {
 		se = cfs_rq->last;
+		goto out;
+	}
 
 	/*
-	 * Someone really wants this to run. If it's not unfair, run it.
+	 * Avoid running the skip buddy, if running something else can
+	 * be done without getting too unfair.
 	 */
-	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
-		se = cfs_rq->next;
+	if (cfs_rq->skip == se) {
+		struct sched_entity *second = __pick_next_entity(se);
+		if (second && wakeup_preempt_entity(second, left) < 1)
+			se = second;
+	}
 
+out:
 	clear_buddies(cfs_rq, se);
-
 	return se;
 }

From: wangyun <[email protected]>

In original code, we get the next entity in this way:

       if(condition1)
               result=value1;
       if(condition2)
               result=value2;
       if(condition3)
               result=value3;
       return result;

So if condition3 is true, we will get value3, but still need to check condition1 and condition2, this will waste our time.

This patch will change the way like:

       if(condition3) {
               result=value3;
               goto out;
       }
       if(condition2) {
               result=value2;
               goto out;
       }
       if(condition1) {
               result=value1;
               goto out;
       }

       out:
       return result;

So we can avoid check condition2 and condition1 when condition3 is true now.

v2:
       1. do not use ugly macro any more.
       2. add more description.

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   26 +++++++++++++++-----------

 1 files changed, 15 insertions(+), 11 deletions(-)

kernel/sched/fair.c +15 -12

index 84adb2d..e8a72b2 100644
@@ -1308,29 +1308,33 @@ static struct sched_entity
 *pick_next_entity(struct cfs_rq *cfs_rq)
        struct sched_entity *left = se;
 
        /*
-        * Avoid running the skip buddy, if running something else can
-        * be done without getting too unfair.
+        * Someone really wants this to run. If it's not unfair, run it.
         */
-       if (cfs_rq->skip == se) {
-               struct sched_entity *second = __pick_next_entity(se);
-               if (second && wakeup_preempt_entity(second, left) < 1)
-                       se = second;
+       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1) {
+               se = cfs_rq->next;
+               goto out;
        }
 
        /*
         * Prefer last buddy, try to return the CPU to a preempted task.
         */
-       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
+       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1) {
                se = cfs_rq->last;
+               goto out;
+       }
 
        /*
-        * Someone really wants this to run. If it's not unfair, run it.
+        * Avoid running the skip buddy, if running something else can
+        * be done without getting too unfair.
         */
-       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
-               se = cfs_rq->next;
+       if (cfs_rq->skip == se) {
+               struct sched_entity *second = __pick_next_entity(se);
+               if (second && wakeup_preempt_entity(second, left) < 1)
+                       se = second;
+       }
 
+out:
        clear_buddies(cfs_rq, se);
-
        return se;
  }
 
--

1.7.4.1

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

Quoted 31 lines · Michael Wang

From: wangyun <[email protected]>

In original code, we get the next entity in this way:

       if(condition1)
               result=value1;
       if(condition2)
               result=value2;
       if(condition3)
               result=value3;
       return result;

So if condition3 is true, we will get value3, but still need to check condition1 and condition2, this will waste our time.

This patch will change the way like:

       if(condition3) {
               result=value3;
               goto out;
       }
       if(condition2) {
               result=value2;
               goto out;
       }
       if(condition1) {
               result=value1;
               goto out;
       }

Quoted 5 lines

       out:
       return result;

So we can avoid check condition2 and condition1 when condition3 is true now.

Then what if condition 1 is true now?

Quoted 69 lines

v2:
1. do not use ugly macro any more.
2. add more description.

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/fair.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

kernel/sched/fair.c +15 -12

index 84adb2d..e8a72b2 100644
@@ -1308,29 +1308,33 @@ static struct sched_entity
 *pick_next_entity(struct cfs_rq *cfs_rq)
       struct sched_entity *left = se;
 
       /*
-        * Avoid running the skip buddy, if running something else can
-        * be done without getting too unfair.
+        * Someone really wants this to run. If it's not unfair, run it.
        */
-       if (cfs_rq->skip == se) {
-               struct sched_entity *second = __pick_next_entity(se);
-               if (second && wakeup_preempt_entity(second, left) < 1)
-                       se = second;
+       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1) {
+               se = cfs_rq->next;
+               goto out;
       }
 
       /*
        * Prefer last buddy, try to return the CPU to a preempted task.
        */
-       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
+       if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1) {
               se = cfs_rq->last;
+               goto out;
+       }
 
       /*
-        * Someone really wants this to run. If it's not unfair, run it.
+        * Avoid running the skip buddy, if running something else can
+        * be done without getting too unfair.
        */
-       if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
-               se = cfs_rq->next;
+       if (cfs_rq->skip == se) {
+               struct sched_entity *second = __pick_next_entity(se);
+               if (second && wakeup_preempt_entity(second, left) < 1)
+                       se = second;
+       }
 
+out:
       clear_buddies(cfs_rq, se);
-
       return se;
 }
 
--

1.7.4.1

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

Quoted 2 lines

Then what if condition 1 is true now?

We can see in original code, even condition 1 is true, we still will use value3 if condition3 is true, like this:

original:

condition1      condition3      result
true            true            value3
true            false           value1

That means if condition3 is true, we don't care whether condition1 is true or not because we will finally use value3.

Quoted 6 lines · Michael Wang

Quoted 2 lines

Then what if condition 1 is true now?

We can see in original code, even condition 1 is true, we still will use value3 if condition3 is true, like this:

Quoted 8 lines

original:

condition1      condition3      result
true            true            value3
true            false           value1

That means if condition3 is true, we don't care whether condition1 is true or not because we will finally use value3.

Right, so from the original 8 possible states we used to evaluate 3*8 = 24 conditionals. The new code will reduce this to 1*4 + 2*2 + 2*3 = 14.

Now I guess the question is if it matters for the modal or average state.

I've applied the patch since it can't be worse, but I've no idea if it matters or not in practice.

Quoted 16 lines · Peter Zijlstra

Quoted 6 lines · Michael Wang

Quoted 2 lines

Then what if condition 1 is true now?

We can see in original code, even condition 1 is true, we still will use value3 if condition3 is true, like this:

Quoted 8 lines

original:

condition1      condition3      result
true            true            value3
true            false           value1

That means if condition3 is true, we don't care whether condition1 is true or not because we will finally use value3.

Quoted 8 lines

Right, so from the original 8 possible states we used to evaluate 3*8 = 24 conditionals. The new code will reduce this to 1*4 + 2*2 + 2*3 = 14.

Now I guess the question is if it matters for the modal or average state.

I've applied the patch since it can't be worse, but I've no idea if it matters or not in practice.

A before/after kernel/sched.o size comparison on 64-bit defconfig typically gives a pretty good indication whether it's a step forward or not.

Thanks,

Ingo

Hi, Ingo

Thanks for your reply.

I have try use "ls -l" to see the size of sched.o, but after applied the patch, it is still 1636.

I have not use this method before, may be I use the wrong command...

But I think the new code should be similar to the old one after compile, because we still have 3 condition check here.

I suppose the new sched.o will be a little bigger, because one jump command and a label need to be added.

Quoted 9 lines · Michael Wang

Hi, Ingo

Thanks for your reply.

I have try use "ls -l" to see the size of sched.o, but after applied the patch, it is still 1636.

I have not use this method before, may be I use the wrong command...

Quoted 6 lines

But I think the new code should be similar to the old one after compile, because we still have 3 condition check here.

I suppose the new sched.o will be a little bigger, because one jump command and a label need to be added.

Try to see if size helps.

Quoted 17 lines · Cong Wang

Quoted 15 lines · Michael Wang

Hi, Ingo

Thanks for your reply.

I have try use "ls -l" to see the size of sched.o, but after applied the patch, it is still 1636.

I have not use this method before, may be I use the wrong command...

But I think the new code should be similar to the old one after compile, because we still have 3 condition check here.

I suppose the new sched.o will be a little bigger, because one jump command and a label need to be added.

Quoted 1 line

Try to see if size helps.

Hi, Cong

Thanks for your advise, but still, the size not changed.

And also I don't know whether the size can be some kind of proof to confirm the performance improvement in this case...

Quoted 19 lines · Michael Wang

Quoted 17 lines · Cong Wang

Quoted 15 lines · Michael Wang

Hi, Ingo

Thanks for your reply.

I have try use "ls -l" to see the size of sched.o, but after applied the patch, it is still 1636.

I have not use this method before, may be I use the wrong command...

But I think the new code should be similar to the old one after compile, because we still have 3 condition check here.

I suppose the new sched.o will be a little bigger, because one jump command and a label need to be added.

Quoted 8 lines

Quoted 1 line

Try to see if size helps.

Hi, Cong

Thanks for your advise, but still, the size not changed.

And also I don't know whether the size can be some kind of proof to confirm the performance improvement in this case...

You could disassemble the .o file via objdump -d and run diff on it - is there any change in the code generated by GCC?

Thanks,

Ingo

But the sched.o under ./arch/x86/kernel/cpu/ still not change... I think I may checking the wrong file, because this patch is for fair.c.

And the fair.o changed after apply the patch, the size is a little bigger, and the gcc generated code changed.

But I still don't know what can we get from this result? Bigger size is caused by additional code, but these additional code will help to step over some unnecessary code under special condition, looks like some balance between size and performance...

Quoted 6 lines · Michael Wang

But the sched.o under ./arch/x86/kernel/cpu/ still not change... I think I may checking the wrong file, because this patch is for fair.c.

And the fair.o changed after apply the patch, the size is a little bigger, and the gcc generated code changed.

Quoted 4 lines

But I still don't know what can we get from this result? Bigger size is caused by additional code, but these additional code will help to step over some unnecessary code under special condition, looks like some balance between size and performance...

As your patch reduces the conditionals from 24 to 14, so it is possible that it also reduces the size of the code too. This is Ingo's point.

You need to check the diff to see why gcc actually generates bigger code.

Signature · 4 lines

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

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */

kernel/sched/core.c +0 -0

 out:
 

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -7

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);
-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

Quoted 5 lines · Michael Wang

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

It helps if you CC the guy who wrote the code.. I think you're right, although getting that skip_clock_update crap wrong is annoying.

Mike?

Quoted 13 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);

kernel/sched/core.c again: · line 4742

Quoted 10 lines

kernel/sched/core.c +0 -7

-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

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

Quoted 5 lines · Michael Wang

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

It helps if you CC the guy who wrote the code..

although getting that skip_clock_update crap wrong is annoying.

Mike?

Quoted 13 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);

kernel/sched/core.c again: · line 4742

Quoted 10 lines

kernel/sched/core.c +0 -7

-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

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

Quoted 5 lines · Michael Wang

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

It helps if you CC the guy who wrote the code.. I think you're right, although getting that skip_clock_update crap wrong is annoying.

Mike?

Quoted 15 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);

kernel/sched/core.c again: · line 4742

Quoted 10 lines

kernel/sched/core.c +0 -7

-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

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

I think Mike also agreed to remove that piece of code, should we action now?

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -7

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);
-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

Quoted 5 lines · Michael Wang

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

Could I get some conclusion on this patch? Should we clean up that peace of code or leave it there?

Regards,
Michael Wang

Quoted 15 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);

kernel/sched/core.c again: · line 4742

Quoted 10 lines

kernel/sched/core.c +0 -7

-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

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

Quoted 9 lines · Michael Wang

Quoted 5 lines · Michael Wang

From: Michael Wang <[email protected]>

it's impossible to enter else branch if we have set skip_clock_update in task_yield_fair(), as yield_to_task_fair() will directly return true after invoke task_yield_fair().

Could I get some conclusion on this patch? Should we clean up that peace of code or leave it there?

s /peace/piece and cc mike...

Quoted 36 lines

Regards,
Michael Wang

Quoted 15 lines

Signed-off-by Michael Wang <[email protected]>

---
 kernel/sched/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/sched/core.c +0 -0

index 9bb7d28..77c14aa 100644
@@ -4737,13 +4737,6 @@ again:
 		 */
 		if (preempt && rq != p_rq)
 			resched_task(p_rq->curr);

kernel/sched/core.c again: · line 4742

Quoted 10 lines

kernel/sched/core.c +0 -7

-	} else {
-		/*
-		 * We might have set it in task_yield_fair(), but are
-		 * not going to schedule(), so don't want to skip
-		 * the next update.
-		 */
-		rq->skip_clock_update = 0;
 	}
 
 out:

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

Quoted 6 lines · Michael Wang

Quoted 2 lines

Then what if condition 1 is true now?

We can see in original code, even condition 1 is true, we still will use value3 if condition3 is true, like this:

Quoted 8 lines

original:

condition1      condition3      result
true            true            value3
true            false           value1

That means if condition3 is true, we don't care whether condition1 is true or not because we will finally use value3.

Right, so from the original 8 possible states we used to evaluate 3*8 = 24 conditionals. The new code will reduce this to 1*4 + 2*2 + 2*3 = 14.

Now I guess the question is if it matters for the modal or average state.

I've applied the patch since it can't be worse, but I've no idea if it matters or not in practice.