kernel/softirq.c: delete 'while' looping to improve a little performance and beautify code

PATCH In review Chen Gang 3 participants 5 messages Jun 9, 2013 – Jun 13, 2013

Share & tools

Chen Gang

author

2013-06-09 12:31 UTC #0 lore raw

PATCH 1/1 kernel/softirq.c: delete 'while' looping to improve a little performance and beautify code

After finish the internal 'while', need not test TASKLET_STATE_SCHED again, so looping back to outside 'while' is only for set_bit().

When use 'if' and set_bit() instead of 'while', it will save at least one running conditional instruction, and also will be clearer for readers (although the binary size will be a little bigger).

The related patch is "1da177e Linux-2.6.12-rc2"

Signed-off-by Chen Gang <[email protected]>

---
 kernel/softirq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/softirq.c +2 -1

index a5f8836..52da25f 100644
@@ -540,10 +540,11 @@ void tasklet_kill(struct tasklet_struct *t)
 	if (in_interrupt())
 		printk("Attempt to kill tasklet from interrupt\n");
 
-	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
+	if (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
 		do {
 			yield();
 		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
+		set_bit(TASKLET_STATE_SCHED, &t->state);
 	}
 	tasklet_unlock_wait(t);
 	clear_bit(TASKLET_STATE_SCHED, &t->state);

Signature · 1 line

1.7.7.6

Paul E. McKenney

2013-06-10 12:25 UTC #1 lore raw

Quoted 21 lines · Chen Gang

After finish the internal 'while', need not test TASKLET_STATE_SCHED again, so looping back to outside 'while' is only for set_bit().
When use 'if' and set_bit() instead of 'while', it will save at least one running conditional instruction, and also will be clearer for readers (although the binary size will be a little bigger).
The related patch is "1da177e Linux-2.6.12-rc2"
Signed-off-by Chen Gang <[email protected]>
---
 kernel/softirq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
kernel/softirq.c +0 -0
index a5f8836..52da25f 100644

kernel/softirq.c void tasklet_kill(struct tasklet_struct *t) · line 547

Quoted 9 lines

kernel/softirq.c +2 -1

@@ -540,10 +540,11 @@ void tasklet_kill(struct tasklet_struct *t)
 	if (in_interrupt())
 		printk("Attempt to kill tasklet from interrupt\n");
 
-	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
+	if (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
 		do {
 			yield();
 		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
+		set_bit(TASKLET_STATE_SCHED, &t->state);

This replaces an atomic test-and-set with two operations, a test and a set. Is this safe?

Thanx, Paul

Quoted 6 lines

kernel/softirq.c +0 -0

 	}
 	tasklet_unlock_wait(t);
 	clear_bit(TASKLET_STATE_SCHED, &t->state);

Signature · 1 line

1.7.7.6

Thomas Gleixner

2013-06-10 14:16 UTC #3 lore raw

In reply to Chen Gang

Quoted 6 lines · Chen Gang

After finish the internal 'while', need not test TASKLET_STATE_SCHED again, so looping back to outside 'while' is only for set_bit().
When use 'if' and set_bit() instead of 'while', it will save at least one running conditional instruction, and also will be clearer for readers (although the binary size will be a little bigger).

And by doing that you break the atomicity of test_and_set_bit. There is a good reason why this is an atomic operation and why the code is written as is.

Quoted 1 line

The related patch is "1da177e Linux-2.6.12-rc2"

How is that patch related to the problem?

Thanks

tglx

Chen Gang

author

2013-06-13 02:09 UTC #2 lore raw

In reply to Paul E. McKenney

Quoted 25 lines · Paul E. McKenney

Quoted 24 lines · Chen Gang
Quoted 24 lines
After finish the internal 'while', need not test TASKLET_STATE_SCHED again, so looping back to outside 'while' is only for set_bit().
When use 'if' and set_bit() instead of 'while', it will save at least one running conditional instruction, and also will be clearer for readers (although the binary size will be a little bigger).
The related patch is "1da177e Linux-2.6.12-rc2"
Signed-off-by Chen Gang <[email protected]>
---
 kernel/softirq.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
kernel/softirq.c +0 -0
index a5f8836..52da25f 100644
@@ -540,10 +540,11 @@ void tasklet_kill(struct tasklet_struct *t)
 	if (in_interrupt())
 		printk("Attempt to kill tasklet from interrupt\n");
 

Quoted 8 lines

Quoted 6 lines
kernel/softirq.c void tasklet_kill(struct tasklet_struct *t) · line 547
Quoted 6 lines
kernel/softirq.c +2 -1
-	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
+	if (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
 		do {
 			yield();
 		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
+		set_bit(TASKLET_STATE_SCHED, &t->state);
This replaces an atomic test-and-set with two operations, a test and a set. Is this safe?

Oh, it seems not safe, at least it is not the original author's willing.

It is my fault, and also sorry for replying late.

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation

Chen Gang

author

2013-06-13 02:11 UTC #4 lore raw

In reply to Thomas Gleixner

Quoted 11 lines · Thomas Gleixner

Quoted 6 lines · Chen Gang
Quoted 6 lines
After finish the internal 'while', need not test TASKLET_STATE_SCHED again, so looping back to outside 'while' is only for set_bit().
When use 'if' and set_bit() instead of 'while', it will save at least one running conditional instruction, and also will be clearer for readers (although the binary size will be a little bigger).
And by doing that you break the atomicity of test_and_set_bit. There is a good reason why this is an atomic operation and why the code is written as is.

OK, thanks. it is my fault (and also sorry for replying late).

Quoted 2 lines

Quoted 1 line
Quoted 1 line
The related patch is "1da177e Linux-2.6.12-rc2"
How is that patch related to the problem?

Because the modification since Linux-2.6.12-rc2.

Thanks.

Signature · 3 lines

Chen Gang

Asianux Corporation