Re: Submission #3 for S2io 10GbE driver

[Jeff Garzik <jgarzik@pobox.com>]

Feldman, Scott wrote:
>>This is incorrect, and definitely an issue that needs to be addressed.
>>
>>As I said, the model is, the driver calls netif_stop_queue() after 
>>queueing a packet, when it knows there is no more room for a full 
>>packet.  The tg3 driver does it like this:
>>	
>>	...queue an skb to hardware...
>>         if (TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1))
>>                 netif_stop_queue(dev);
>>
>>Therefore you guarantee the queue is stopped until you are 
>>100% certain that another skb (up to MAX_SKB_FRAGS + "main frag"
>>fragments) may be queued to hardware.
>>
>>You do -not- want to figure out "after the fact" that you 
>>cannot queue the skb you were just passed.
> 
> 
> But tg3 checks this case also and returns 1:
> 
>         /* This is a hard error, log it. */
>         if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags +
> 1))) {
>                 netif_stop_queue(dev);
>                 ...
>                 return 1;
>         }
> 
> Does this code path happen?

You snipped the important

                 printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue 
awake!\n",
                        dev->name);

It's a BUG because it should never happen.

tg3 needs to kfree the skb too, leading to my comment "some existing 
drivers get this wrong too".  Requeueing the skb only occurs in -some- 
packet schedulers, not all.  So drivers cannot depend on the net stack 
doing what you want in all cases.  Conditional correct behavior or 
leak..  :/

	Jeff