Re: [PATCH net v2 2/3] r8152: modify the tx flow
From: hayeswang hayesw...@realtek.com Date: Thu, 31 Oct 2013 13:52:38 +0800 From: David Miller [mailto:da...@davemloft.net] Sent: Thursday, October 31, 2013 5:05 AM From: Hayes Wang hayesw...@realtek.com Date: Wed, 30 Oct 2013 15:13:39 +0800 [...] Basically, your driver will now queue up to 1,000 packets onto this tx_queue list, because that is what tx_queue_len will be for alloc_etherdev() allocated network devices. In my previous reply to you about this patch, I asked you to quantify and study the effects of using a limit of 60. I said that 60 might be too large. You've responded by removing the limit completely, which is exactly the opposite of what I've asked you to do. Why did you do this? Excuse me. My question is that the original code doesn't stop the tx queue either, so I don't understand why it is necessary for this patch. I don't say I wouldn't find the suitable value for the tx queue length. I feel I need some time to think how to find the reasonable value. And I don't hope it influences the submission of the other patches, so I remove it first. Or, may I submit the other two patches first? The more TX work you push into the workqueue handler, the longer the latency for releasing the SKB and releasing all the queues that are waiting for release of that packet. Do you know that sockets, queueing discplines, etc. all rely upon there being a timely release of SKBs once they are successfully transmitted? It must happen at the earliest moment possible that can be reasonable obtained. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net v2 2/3] r8152: modify the tx flow
Remove the code for sending the packet in the rtl8152_start_xmit().
Let rtl8152_start_xmit() to queue the packet only, and schedule a
tasklet to send the queued packets. This simplify the code and make
sure all the packet would be sent by the original order.
Signed-off-by: Hayes Wang hayesw...@realtek.com
---
drivers/net/usb/r8152.c | 46 +++---
1 file changed, 3 insertions(+), 43 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5dbfe50..763234d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1388,53 +1388,13 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff
*skb,
struct net_device *netdev)
{
struct r8152 *tp = netdev_priv(netdev);
- struct net_device_stats *stats = rtl8152_get_stats(netdev);
- unsigned long flags;
- struct tx_agg *agg = NULL;
- struct tx_desc *tx_desc;
- unsigned int len;
- u8 *tx_data;
- int res;
skb_tx_timestamp(skb);
- /* If tx_queue is not empty, it means at least one previous packt */
- /* is waiting for sending. Don't send current one before it. */
- if (skb_queue_empty(tp-tx_queue))
- agg = r8152_get_tx_agg(tp);
-
- if (!agg) {
- skb_queue_tail(tp-tx_queue, skb);
- return NETDEV_TX_OK;
- }
-
- tx_desc = (struct tx_desc *)agg-head;
- tx_data = agg-head + sizeof(*tx_desc);
- agg-skb_num = agg-skb_len = 0;
+ skb_queue_tail(tp-tx_queue, skb);
- len = skb-len;
- r8152_tx_csum(tp, tx_desc, skb);
- memcpy(tx_data, skb-data, len);
- dev_kfree_skb_any(skb);
- agg-skb_num++;
- agg-skb_len += len;
- usb_fill_bulk_urb(agg-urb, tp-udev, usb_sndbulkpipe(tp-udev, 2),
- agg-head, len + sizeof(*tx_desc),
- (usb_complete_t)write_bulk_callback, agg);
- res = usb_submit_urb(agg-urb, GFP_ATOMIC);
- if (res) {
- /* Can we get/handle EPIPE here? */
- if (res == -ENODEV) {
- netif_device_detach(tp-netdev);
- } else {
- netif_warn(tp, tx_err, netdev,
- failed tx_urb %d\n, res);
- stats-tx_dropped++;
- spin_lock_irqsave(tp-tx_lock, flags);
- list_add_tail(agg-list, tp-tx_free);
- spin_unlock_irqrestore(tp-tx_lock, flags);
- }
- }
+ if (!list_empty(tp-tx_free))
+ tasklet_schedule(tp-tl);
return NETDEV_TX_OK;
}
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net v2 2/3] r8152: modify the tx flow
From: Hayes Wang hayesw...@realtek.com Date: Wed, 30 Oct 2013 15:13:39 +0800 Remove the code for sending the packet in the rtl8152_start_xmit(). Let rtl8152_start_xmit() to queue the packet only, and schedule a tasklet to send the queued packets. This simplify the code and make sure all the packet would be sent by the original order. Signed-off-by: Hayes Wang hayesw...@realtek.com Basically, your driver will now queue up to 1,000 packets onto this tx_queue list, because that is what tx_queue_len will be for alloc_etherdev() allocated network devices. In my previous reply to you about this patch, I asked you to quantify and study the effects of using a limit of 60. I said that 60 might be too large. You've responded by removing the limit completely, which is exactly the opposite of what I've asked you to do. Why did you do this? This patch series is still not in a state where I can apply it, sorry. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net v2 2/3] r8152: modify the tx flow
From: David Miller [mailto:da...@davemloft.net] Sent: Thursday, October 31, 2013 5:05 AM From: Hayes Wang hayesw...@realtek.com Date: Wed, 30 Oct 2013 15:13:39 +0800 [...] Basically, your driver will now queue up to 1,000 packets onto this tx_queue list, because that is what tx_queue_len will be for alloc_etherdev() allocated network devices. In my previous reply to you about this patch, I asked you to quantify and study the effects of using a limit of 60. I said that 60 might be too large. You've responded by removing the limit completely, which is exactly the opposite of what I've asked you to do. Why did you do this? Excuse me. My question is that the original code doesn't stop the tx queue either, so I don't understand why it is necessary for this patch. I don't say I wouldn't find the suitable value for the tx queue length. I feel I need some time to think how to find the reasonable value. And I don't hope it influences the submission of the other patches, so I remove it first. Or, may I submit the other two patches first? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
