Re: [PATCH] [e1000]: Remove unnecessary tx_lock

[jamal <hadi@cyberus.ca>]

On Thu, 2006-03-08 at 17:09 -0700, Michael Chan wrote:
> On Thu, 2006-08-03 at 18:08 -0400, jamal wrote:
> 
> > 
> > Yes in your case you need to hold the lock but not in the e1000 case.
> > I dont understand though why you need to check for wake in the tx path.
> > 
> 
> tg3_start_xmit()
> 
> 	if (tx_ring_empty)
> 				<-  tg3_tx()
> 		netif_stop_queue()
> 
> 
> At the arrow, tg3_tx() can come in and clean up the entire ring but will
> not see that the queue is stopped and therefore will not call
> netif_wake_queue().  As a result, the tx_queue is stopped forever.
> 

Indeed - if you do it that way. 
What about (actually e1000 is not exactly like this but closer than tg3
is):

    tg3_start_xmit():
    
      prepare skb                       
      grab ring tx lock irq safe
      put skb on ring
      if (full-ring)
          stop
      release ring tx lock and restore flags blah 

and in tg3_tx():

     if likely(stopped) {  
       // essentially no lock needed here ....
       prune tx ring
       if prunned > threshold
           wake
     } else if not stopped { 
          // need a lock here ...
         grab ring tx lock irq safe
         prune tx ring
         release ring tx lock
     }

The above is safe to run tg3_start_xmit on one CPU and tg3_tx an
another.

And you can then take advantage of asynchronous wakes and remove the
paranoia checks you have on tx.

cheers,
jamal