Subject: Re: [PATCH] sky2: safer transmit ring cleaning

From: Michael Breuer <[email-masked]>
Date: Tue, 12 Jan 2010 13:42:38 -0500

On 1/12/2010 1:35 PM, Michael Breuer wrote:
> On 1/12/2010 11:15 AM, Stephen Hemminger wrote:
>> This code makes transmit path and transmit reset safer by:
>>    * adding memory barrier before checking available ring slots
>>    * reseting state of tx ring elements after free
>>    * seperate cleanup function from ring done function
>>    * removing mostly unused tx_next element
>>
>> Signed-off-by: Stephen Hemminger<[email-masked]>
>>
>> ---
>> Please apply this instead of the various bits and pieces flying
>> around labeled as sky2 panic under load
>>
>>
>> --- a/drivers/net/sky2.c    2010-01-11 10:49:50.907113126 -0800
>> +++ b/drivers/net/sky2.c    2010-01-11 17:36:22.027429875 -0800
>> @@ -1596,6 +1596,9 @@ static inline int tx_inuse(const struct
>>   /* Number of list elements available for next tx */
>>   static inline int tx_avail(const struct sky2_port *sky2)
>>   {
>> +    /* Makes sure update of tx_prod from start_xmit and
>> +       tx_cons from tx_done are seen. */
>> +    smp_mb();
>>       return sky2->tx_pending - tx_inuse(sky2);
>>   }
>>
>> @@ -1618,8 +1621,7 @@ static unsigned tx_le_req(const struct s
>>       return count;
>>   }
>>
>> -static void sky2_tx_unmap(struct pci_dev *pdev,
>> -              const struct tx_ring_info *re)
>> +static void sky2_tx_unmap(struct pci_dev *pdev, struct tx_ring_info 
>> *re)
>>   {
>>       if (re->flags&  TX_MAP_SINGLE)
>>           pci_unmap_single(pdev, pci_unmap_addr(re, mapaddr),
>> @@ -1629,6 +1631,7 @@ static void sky2_tx_unmap(struct pci_dev
>>           pci_unmap_page(pdev, pci_unmap_addr(re, mapaddr),
>>                      pci_unmap_len(re, maplen),
>>                      PCI_DMA_TODEVICE);
>> +    re->flags = 0;
>>   }
>>
>>   /*
>> @@ -1804,7 +1807,8 @@ mapping_error:
>>   }
>>
>>   /*
>> - * Free ring elements from starting at tx_cons until "done"
>> + * Transmit complete processing
>> + * Free ring elements from starting at tx_cons until done index
>>    *
>>    * NB:
>>    *  1. The hardware will tell us about partial completion of 
>> multi-part
>> @@ -1813,9 +1817,9 @@ mapping_error:
>>    *     looks at the tail of the queue of FIFO (tx_cons), not
>>    *     the head (tx_prod)
>>    */
>> -static void sky2_tx_complete(struct sky2_port *sky2, u16 done)
>> +static void sky2_tx_done(struct net_device *dev, u16 done)
>>   {
>> -    struct net_device *dev = sky2->netdev;
>> +    struct sky2_port *sky2 = netdev_priv(dev);
>>       unsigned idx;
>>
>>       BUG_ON(done>= sky2->tx_ring_size);
>> @@ -1828,6 +1832,8 @@ static void sky2_tx_complete(struct sky2
>>           sky2_tx_unmap(sky2->hw->pdev, re);
>>
>>           if (skb) {
>> +            re->skb = NULL;
>> +
>>               if (unlikely(netif_msg_tx_done(sky2)))
>>                   printk(KERN_DEBUG "%s: tx done %u\n",
>>                          dev->name, idx);
>> @@ -1836,13 +1842,10 @@ static void sky2_tx_complete(struct sky2
>>               dev->stats.tx_bytes += skb->len;
>>
>>               dev_kfree_skb_any(skb);
>> -
>> -            sky2->tx_next = RING_NEXT(idx, sky2->tx_ring_size);
>>           }
>>       }
>>
>>       sky2->tx_cons = idx;
>> -    smp_mb();
>>
>>       if (tx_avail(sky2)>  MAX_SKB_TX_LE + 4)
>>           netif_wake_queue(dev);
>> @@ -1870,6 +1873,21 @@ static void sky2_tx_reset(struct sky2_hw
>>       sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
>>   }
>>
>> +static void sky2_tx_clean(struct sky2_port *sky2)
>> +{
>> +    u16 idx;
>> +
>> +    for (idx = 0; idx<  sky2->tx_ring_size; idx++) {
>> +        struct tx_ring_info *re = sky2->tx_ring + idx;
>> +
>> +        sky2_tx_unmap(sky2->hw->pdev, re);
>> +        if (re->skb) {
>> +            dev_kfree_skb_any(re->skb);
>> +            re->skb = NULL;
>> +        }
>> +    }
>> +}
>> +
>>   /* Network shutdown */
>>   static int sky2_down(struct net_device *dev)
>>   {
>> @@ -1933,8 +1951,7 @@ static int sky2_down(struct net_device *
>>       sky2_tx_reset(hw, port);
>>
>>       /* Free any pending frames stuck in HW queue */
>> -    sky2_tx_complete(sky2, sky2->tx_prod);
>> -
>> +    sky2_tx_clean(sky2);
>>       sky2_rx_clean(sky2);
>>
>>       sky2_free_buffers(sky2);
>> @@ -2411,15 +2428,6 @@ error:
>>       goto resubmit;
>>   }
>>
>> -/* Transmit complete */
>> -static inline void sky2_tx_done(struct net_device *dev, u16 last)
>> -{
>> -    struct sky2_port *sky2 = netdev_priv(dev);
>> -
>> -    if (netif_running(dev))
>> -        sky2_tx_complete(sky2, last);
>> -}
>> -
>>   static inline void sky2_skb_rx(const struct sky2_port *sky2,
>>                      u32 status, struct sk_buff *skb)
>>   {
>> @@ -4201,7 +4209,7 @@ static int sky2_debug_show(struct seq_fi
>>
>>       /* Dump contents of tx ring */
>>       sop = 1;
>> -    for (idx = sky2->tx_next; idx != sky2->tx_prod&&  idx<  
>> sky2->tx_ring_size;
>> +    for (idx = sky2->tx_cons; idx != sky2->tx_prod&&  idx<  
>> sky2->tx_ring_size;
>>            idx = RING_NEXT(idx, sky2->tx_ring_size)) {
>>           const struct sky2_tx_le *le = sky2->tx_le + idx;
>>           u32 a = le32_to_cpu(le->addr);
>> --- a/drivers/net/sky2.h    2010-01-11 17:29:22.817088617 -0800
>> +++ b/drivers/net/sky2.h    2010-01-11 17:29:28.197120484 -0800
>> @@ -2187,7 +2187,6 @@ struct sky2_port {
>>       u16             tx_ring_size;
>>       u16             tx_cons;        /* next le to check */
>>       u16             tx_prod;        /* next le to use */
>> -    u16             tx_next;        /* debug only */
>>
>>       u16             tx_pending;
>>       u16             tx_last_mss;
> Test observations:
>
> 1. DHCP request/response sequence having some issues... can't confirm 
> that it's a result of this patch, but I don't see this with the prior 
> patch. Prior to this patch, if I connect a new device (Blackberry in 
> this case) I see DHCPDISCOVER;DHCPOFFER;DHCPREQUEST;DHCPACK (just the 
> four messages). With this patch I'm seeing repeated attempts - i.e., 
> DISCOVER/OFFER 6 times before the REQUEST/ACK. This is repeatable and  
> happening whether or not under load. As my original problem started 
> with DHCP packets, this seems interesting. I don't see any errors 
> logged (dmesg, messages, etc.).
> 2. Probably not related to this patch, but perhaps to the driver - 
> I've finally tracked the source of my dropped RX packets. It's 
> happening when a sending data to a CIFS client on a MacOS system. The 
> RX drop rate seems proportional to the TX rate for SMB to that client 
> - at a tx rate of about 200Kb/s I see about 20 dropped RX packets/sec 
> - at 400 I see about 40.  I'm thinking therefore it's ACKs being 
> dropped on RX. Why? no idea (yet). Nothing reported by ethtool or 
> netstat -s remotely correlates to the number of dropped RX packets.
>
>
Let me add: the CIFS client from which the packets are dropped is 
connected via a dd-wrt router (on wifi) connected to the sky2 1G port. A 
Windows client connected directly to the 1G port does not exhibit the 
same symptoms (. I'll try later the Mac directly connected & a Wintel 
box over wifi if possible. The DD-WRT router (linksys wrt54g-tm) is 
bridged (wifi clients on same subnet as wired & serviced by DHCPD 
running on the linux box). This is also the source of the aforementioned 
perhaps-flaky DHCP connections.