[PATCH] IB/ehca: Reduce number of spin_lock/spin_unlocks in ehca_poll_eqs

[PATCH] IB/ehca: Reduce number of spin_lock/spin_unlocks in ehca_poll_eqs

[Anton Blanchard]

ftrace function_graph shows ehca_poll_eqs taking and dropping a spinlock
a lot:

 16)               |          .ehca_poll_eqs() {
 16)   0.492 us    |            ._spin_lock();
 16)   0.504 us    |            ._spin_lock_irqsave();
 16)   0.502 us    |            ._spin_unlock_irqrestore();
 16)   0.490 us    |            ._spin_lock_irqsave();
 16)   0.488 us    |            ._spin_unlock_irqrestore();
 16)   0.488 us    |            ._spin_lock_irqsave();
 16)   0.488 us    |            ._spin_unlock_irqrestore();
 16)   0.490 us    |            ._spin_lock_irqsave();
 16)   0.492 us    |            ._spin_unlock_irqrestore();

Since it's all the same spinlock, we can just take and drop it once.

Signed-off-by: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---

Index: linux.trees.git/drivers/infiniband/hw/ehca/ehca_main.c
===================================================================
--- linux.trees.git.orig/drivers/infiniband/hw/ehca/ehca_main.c	2009-11-27 10:17:29.000000000 +1100
+++ linux.trees.git/drivers/infiniband/hw/ehca/ehca_main.c	2009-11-27 10:19:08.000000000 +1100
@@ -957,15 +957,17 @@ void ehca_poll_eqs(unsigned long data)
 			int max = 3;
 			volatile u64 q_ofs, q_ofs2;
 			unsigned long flags;
+
 			spin_lock_irqsave(&eq->spinlock, flags);
+
 			q_ofs = eq->ipz_queue.current_q_offset;
-			spin_unlock_irqrestore(&eq->spinlock, flags);
 			do {
-				spin_lock_irqsave(&eq->spinlock, flags);
 				q_ofs2 = eq->ipz_queue.current_q_offset;
-				spin_unlock_irqrestore(&eq->spinlock, flags);
 				max--;
 			} while (q_ofs == q_ofs2 && max > 0);
+
+			spin_unlock_irqrestore(&eq->spinlock, flags);
+
 			if (q_ofs == q_ofs2)
 				ehca_process_eq(shca, 0);
 		}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/ehca: Reduce number of spin_lock/spin_unlocks in ehca_poll_eqs

[Roland Dreier]

 > +
 >  			spin_lock_irqsave(&eq->spinlock, flags);
 > +
 >  			q_ofs = eq->ipz_queue.current_q_offset;
 > -			spin_unlock_irqrestore(&eq->spinlock, flags);
 >  			do {
 > -				spin_lock_irqsave(&eq->spinlock, flags);
 >  				q_ofs2 = eq->ipz_queue.current_q_offset;
 > -				spin_unlock_irqrestore(&eq->spinlock, flags);
 >  				max--;
 >  			} while (q_ofs == q_ofs2 && max > 0);
 > +
 > +			spin_unlock_irqrestore(&eq->spinlock, flags);
 > +

Does this loop have to drop the lock to give the EQ code a chance to
update the queue pointer?  If I understand this correctly, the EQ
polling code is just trying to have a fallback if interrupts don't
work...

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/ehca: Reduce number of spin_lock/spin_unlocks in ehca_poll_eqs

[Hoang-Nam Nguyen]

Hi Roland,
Yes, you're right. The loop below is a fallback to detect possibly missing
irq events
by checking the eq pointer, which can be changed by irq handler.
@Anton, for above reason we can not remove the lock/unlock within the loop.
Thanks,
Nam

Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote on 30.11.2009 06:26:35:

> [image removed]
>
> Re: [PATCH] IB/ehca: Reduce number of spin_lock/spin_unlocks in
ehca_poll_eqs
>
> Roland Dreier
>
> to:
>
> Anton Blanchard
>
> 30.11.2009 06:26
>
> Cc:
>
> Hoang-Nam Nguyen, Christoph Raisch, Stefan Roscher, Alexander
> Schmidt, Joachim Fenkes, linux-rdma
>
>
>  > +
>  >           spin_lock_irqsave(&eq->spinlock, flags);
>  > +
>  >           q_ofs = eq->ipz_queue.current_q_offset;
>  > -         spin_unlock_irqrestore(&eq->spinlock, flags);
>  >           do {
>  > -            spin_lock_irqsave(&eq->spinlock, flags);
>  >              q_ofs2 = eq->ipz_queue.current_q_offset;
>  > -            spin_unlock_irqrestore(&eq->spinlock, flags);
>  >              max--;
>  >           } while (q_ofs == q_ofs2 && max > 0);
>  > +
>  > +         spin_unlock_irqrestore(&eq->spinlock, flags);
>  > +
>
> Does this loop have to drop the lock to give the EQ code a chance to
> update the queue pointer?  If I understand this correctly, the EQ
> polling code is just trying to have a fallback if interrupts don't
> work...
>
>  - R.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html