[PATCH 0/3] micro-optimize irq-poll

[PATCH 0/3] micro-optimize irq-poll

[Sagi Grimberg]

Some useful patches I came up with when working
on nvme irq-poll conversion (which still needs some
work).

Sagi Grimberg (3):
  irq-poll: Remove redundant include
  irq-poll: micro optimize some conditions
  irq-poll: Reduce local_irq_save/restore operations in irq_poll_softirq

 lib/irq_poll.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 1/3] irq-poll: Remove redundant include

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 lib/irq_poll.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/irq_poll.c b/lib/irq_poll.c
index 1d6565e81030..22d033e6ded2 100644
--- a/lib/irq_poll.c
+++ b/lib/irq_poll.c
@@ -5,7 +5,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
-#include <linux/bio.h>
 #include <linux/interrupt.h>
 #include <linux/cpu.h>
 #include <linux/irq_poll.h>
-- 
2.7.4

[PATCH 2/3] irq-poll: micro optimize some branch predictions

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 lib/irq_poll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/irq_poll.c b/lib/irq_poll.c
index 22d033e6ded2..44a5d1da4260 100644
--- a/lib/irq_poll.c
+++ b/lib/irq_poll.c
@@ -26,9 +26,9 @@ void irq_poll_sched(struct irq_poll *iop)
 {
 	unsigned long flags;
 
-	if (test_bit(IRQ_POLL_F_DISABLE, &iop->state))
+	if (unlikely(test_bit(IRQ_POLL_F_DISABLE, &iop->state)))
 		return;
-	if (test_and_set_bit(IRQ_POLL_F_SCHED, &iop->state))
+	if (likely(test_and_set_bit(IRQ_POLL_F_SCHED, &iop->state)))
 		return;
 
 	local_irq_save(flags);
@@ -120,7 +120,7 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 		 * move the instance around on the list at-will.
 		 */
 		if (work >= weight) {
-			if (test_bit(IRQ_POLL_F_DISABLE, &iop->state))
+			if (unlikely(test_bit(IRQ_POLL_F_DISABLE, &iop->state)))
 				__irq_poll_complete(iop);
 			else
 				list_move_tail(&iop->list, list);
-- 
2.7.4

[PATCH 3/3] irq-poll: Reduce local_irq_save/restore operations in irq_poll_softirq

[Sagi Grimberg]

splice to a local list (and splice back when done) so we won't
need to enable/disable local_irq in each iteration.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 lib/irq_poll.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/lib/irq_poll.c b/lib/irq_poll.c
index 44a5d1da4260..dc4c7ace9b41 100644
--- a/lib/irq_poll.c
+++ b/lib/irq_poll.c
@@ -75,13 +75,16 @@ EXPORT_SYMBOL(irq_poll_complete);
 
 static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 {
-	struct list_head *list = this_cpu_ptr(&blk_cpu_iopoll);
+	struct list_head *iop_list = this_cpu_ptr(&blk_cpu_iopoll);
 	int rearm = 0, budget = irq_poll_budget;
 	unsigned long start_time = jiffies;
+	LIST_HEAD(list);
 
 	local_irq_disable();
+	list_splice_init(iop_list, &list);
+	local_irq_enable();
 
-	while (!list_empty(list)) {
+	while (!list_empty(&list)) {
 		struct irq_poll *iop;
 		int work, weight;
 
@@ -93,14 +96,7 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 			break;
 		}
 
-		local_irq_enable();
-
-		/* Even though interrupts have been re-enabled, this
-		 * access is safe because interrupts can only add new
-		 * entries to the tail of this list, and only ->poll()
-		 * calls can remove this head entry from the list.
-		 */
-		iop = list_entry(list->next, struct irq_poll, list);
+		iop = list_first_entry(&list, struct irq_poll, list);
 
 		weight = iop->weight;
 		work = 0;
@@ -109,8 +105,6 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 
 		budget -= work;
 
-		local_irq_disable();
-
 		/*
 		 * Drivers must not modify the iopoll state, if they
 		 * consume their assigned weight (or more, some drivers can't
@@ -120,13 +114,20 @@ static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
 		 * move the instance around on the list at-will.
 		 */
 		if (work >= weight) {
-			if (unlikely(test_bit(IRQ_POLL_F_DISABLE, &iop->state)))
+			if (unlikely(test_bit(IRQ_POLL_F_DISABLE, &iop->state))) {
+				local_irq_disable();
 				__irq_poll_complete(iop);
-			else
-				list_move_tail(&iop->list, list);
+				local_irq_enable();
+			} else {
+				list_move_tail(&iop->list, &list);
+			}
 		}
 	}
 
+	local_irq_disable();
+	list_splice_tail_init(iop_list, &list);
+	list_splice(&list, iop_list);
+
 	if (rearm)
 		__raise_softirq_irqoff(IRQ_POLL_SOFTIRQ);
 
-- 
2.7.4

Re: [PATCH 1/3] irq-poll: Remove redundant include

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] irq-poll: micro optimize some branch predictions

[Christoph Hellwig]

Are they really that unlikely?  I don't like these annotations unless
it's clearly an error path or they have a high, demonstrable benefit.

Re: [PATCH 3/3] irq-poll: Reduce local_irq_save/restore operations in irq_poll_softirq

[Christoph Hellwig]

> +	while (!list_empty(&list)) {

Maybe do a list_first_entry_or_null here if you're touching the list
iteration anyway?

> +	local_irq_disable();
> +	list_splice_tail_init(iop_list, &list);
> +	list_splice(&list, iop_list);
> +
>  	if (rearm)
>  		__raise_softirq_irqoff(IRQ_POLL_SOFTIRQ);

Maybe check if we have a non-empty list before disabling irqs?
Also that list_emtpy check can replace the rearm condition - we only
set the rearm flag if we break with a non-empty local list now.

Re: [PATCH 2/3] irq-poll: micro optimize some branch predictions

[Sagi Grimberg]

> Are they really that unlikely?  I don't like these annotations unless
> it's clearly an error path or they have a high, demonstrable benefit.

IRQ_POLL_F_DISABLE is set when disabling the iop (in the end of the
world). IRQ_POLL_F_SCHED is set on irq_poll_sched() itself so this cond
would match only if the user called irq_poll_sched() twice which it
shouldn't ever do.

So yes, I think these are really unlikely()

But now I noticed that I typo'ed the IRQ_POLL_F_SCHED from
unlikely to likely :(

need to fix that...

Re: [PATCH 3/3] irq-poll: Reduce local_irq_save/restore operations in irq_poll_softirq

[Sagi Grimberg]

>> +	while (!list_empty(&list)) {
>
> Maybe do a list_first_entry_or_null here if you're touching the list
> iteration anyway?

I can do that.

>> +	local_irq_disable();
>> +	list_splice_tail_init(iop_list, &list);
>> +	list_splice(&list, iop_list);
>> +
>>  	if (rearm)
>>  		__raise_softirq_irqoff(IRQ_POLL_SOFTIRQ);
>
> Maybe check if we have a non-empty list before disabling irqs?

Which list? the local list? do in case the local list is empty
but iop_list isn't another irq_poll_sched() already raised a
softirq?

> Also that list_emtpy check can replace the rearm condition - we only
> set the rearm flag if we break with a non-empty local list now.

You're right.