Re: [PATCH] tg3_msi() and weakly ordered memory

Re: [PATCH] tg3_msi() and weakly ordered memory

[Grant Grundler]

On Mon, Jun 13, 2005 at 11:54:23PM -0700, Michael Chan wrote:
> > Once you write "0x1" to the mailbox register, the device stops
> > updating the status block and stops generating interrupts.
> > 
> > That is what makes a lot of things safe.
> 
> Only interrupts are stopped, status block will still be updated subject to
> during-ints coalescing.

Will setting during-ints to a very high threshhold essentially allow
us to "indefinitely" process stuff without taking any interrupts?
Would the threshhold counter get reset every time we write back
the status tag WITHOUT re-enableing interrupts?

If not, I suspect the CPU will circulate in tg3_poll until during-ints
is exhausted and DMA will stop until CPU reenables interrupts.
ie not until it's done processing outstanding packets.

thanks,
grant

Re: [PATCH] tg3_msi() and weakly ordered memory

[David S. Miller]

Ok, here is the patch I came up with as a result of this thread.

Michael stated he would investigate using a pure tag comparison in
place of tg3_has_work() when the chip is using tagged interrupts.

Thanks.

[TG3]: Fix missing memory barriers and SD_STATUS_UPDATED bit clearing.

There must be a rmb() between reading the status block tag
and calling tg3_has_work().  This was missing in tg3_mis()
and tg3_interrupt_tagged().  tg3_poll() got it right.

Also, SD_STATUS_UPDATED must be cleared in the status block
right before we call tg3_has_work().  Only tg3_poll() got this
wrong.

Based upon patches and commentary from Grant Grundler and
Michael Chan.

Signed-off-by: David S. Miller <davem@davemloft.net>

--- 1/drivers/net/tg3.c.~1~	2005-06-21 16:39:19.000000000 -0700
+++ 2/drivers/net/tg3.c	2005-06-21 16:47:55.000000000 -0700
@@ -2929,6 +2929,7 @@
 	if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)
 		tp->last_tag = sblk->status_tag;
 	rmb();
+	sblk->status &= ~SD_STATUS_UPDATED;
 
 	/* if no more work, tell net stack and NIC we're done */
 	done = !tg3_has_work(tp);
@@ -2964,6 +2965,7 @@
 	 */
 	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
 	tp->last_tag = sblk->status_tag;
+	rmb();
 	sblk->status &= ~SD_STATUS_UPDATED;
 	if (likely(tg3_has_work(tp)))
 		netif_rx_schedule(dev);		/* schedule NAPI poll */
@@ -3051,6 +3053,7 @@
 		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     0x00000001);
 		tp->last_tag = sblk->status_tag;
+		rmb();
 		sblk->status &= ~SD_STATUS_UPDATED;
 		if (likely(tg3_has_work(tp)))
 			netif_rx_schedule(dev);		/* schedule NAPI poll */

Re: [PATCH] tg3_msi() and weakly ordered memory

[Grant Grundler]

On Tue, Jun 21, 2005 at 04:56:34PM -0700, David S. Miller wrote:
> 
> Ok, here is the patch I came up with as a result of this thread.

looks good to me.

> Michael stated he would investigate using a pure tag comparison in
> place of tg3_has_work() when the chip is using tagged interrupts.

The more I think about it, the more I like the idea of each ISR calling
into a different tg3_poll routine. The specific _poll() routine could
do the "is there more work" checking instead the TX/RX ring
cleanup code. The main reason is the "more work" checks can
be better optimized for MSI (use tags) vs IRQ Line interrupt (use ring
indices) handlers. I also hope to reduce cacheline movement by touching
the status block fewer times.

This isn't a trivial patch and I'm short on time (preparing stuff
for OLS and HP World before my vacation). If there is still interest,
I can prototype a patch in late August or Sept (about 8 weeks from now).


> Thanks.

Welcome and thanks too.

BTW, I greatly appreciate Michael clarifying tg3 behavior.

thanks,
grant

[PATCH] dont use strlen() but the result from a prior sprintf()

[Eric Dumazet]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

Hi David

Small patch to save an unecessary call to strlen() : sprintf() gave us the length, just trust it.

Thank you

Eric Dumazet


diff -Nu linux-2.6.12-orig/net/socket.c linux-2.6.12/net/socket.c
--- linux-2.6.12-orig/net/socket.c      2005-06-22 14:47:56.000000000 +0200
+++ linux-2.6.12/net/socket.c   2005-06-22 14:49:22.000000000 +0200
@@ -382,9 +382,8 @@
                         goto out;
                 }

-               sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+               this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
                 this.name = name;
-               this.len = strlen(name);
                 this.hash = SOCK_INODE(sock)->i_ino;

                 file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);


[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 446 bytes --]

--- linux-2.6.12-orig/net/socket.c	2005-06-22 14:47:56.000000000 +0200
+++ linux-2.6.12/net/socket.c	2005-06-22 14:49:22.000000000 +0200
@@ -382,9 +382,8 @@
 			goto out;
 		}
 
-		sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+		this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
 		this.name = name;
-		this.len = strlen(name);
 		this.hash = SOCK_INODE(sock)->i_ino;
 
 		file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);