Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path
From: Amit Pundir
Date: Mon Feb 08 2016 - 15:37:48 EST
Please ignore this one too. I should have build tested these patches
individually and not in particular series. I'll resend this patch.
Thanks,
Amit Pundir
On 9 February 2016 at 01:41, Amit Pundir <amit.pundir@xxxxxxxxxx> wrote:
> From: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
>
> u_ether driver passes rx data to network layer and resubmits the
> request back to usb hardware in interrupt context. Network layer
> processes rx data by scheduling tasklet. For high throughput
> scenarios on rx data path driver is spending lot of time in interrupt
> context due to rx data processing by tasklet and continuous completion
> and re-submission of the usb requests which results in watchdog bark.
> Hence move the rx data processing and usb request submission to a
> workqueue bottom half handler.
>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Mike Looijmans <mike.looijmans@xxxxxxxx>
> Cc: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> Cc: Android Kernel Team <kernel-team@xxxxxxxxxxx>
> Cc: John Stultz <john.stultz@xxxxxxxxxx>
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Signed-off-by: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
> [pundir: cherry-picked this patch from AOSP experimental/android-4.4 tree.]
> Signed-off-by: Amit Pundir <amit.pundir@xxxxxxxxxx>
> ---
> Cherry-picked this patch from AOSP common/experimental/android-4.4 tree.
> I could not find upstream submission history for this patch, so my
> apologies in advance if this has already been NACKed before.
>
> drivers/usb/gadget/function/u_ether.c | 119 +++++++++++++++++++++++-----------
> 1 file changed, 80 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 7f98a2d..4235c33 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -53,6 +53,8 @@
> * blocks and still have efficient handling. */
> #define GETHER_MAX_ETH_FRAME_LEN 15412
>
> +static struct workqueue_struct *uether_wq;
> +
> struct eth_dev {
> /* lock is held while accessing port_usb
> */
> @@ -77,6 +79,7 @@ struct eth_dev {
> struct sk_buff_head *list);
>
> struct work_struct work;
> + struct work_struct rx_work;
>
> unsigned long todo;
> #define WORK_RX_MEMORY 0
> @@ -248,18 +251,16 @@ enomem:
> DBG(dev, "rx submit --> %d\n", retval);
> if (skb)
> dev_kfree_skb_any(skb);
> - spin_lock_irqsave(&dev->req_lock, flags);
> - list_add(&req->list, &dev->rx_reqs);
> - spin_unlock_irqrestore(&dev->req_lock, flags);
> }
> return retval;
> }
>
> static void rx_complete(struct usb_ep *ep, struct usb_request *req)
> {
> - struct sk_buff *skb = req->context, *skb2;
> + struct sk_buff *skb = req->context;
> struct eth_dev *dev = ep->driver_data;
> int status = req->status;
> + bool queue = 0;
>
> switch (status) {
>
> @@ -283,30 +284,8 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req)
> } else {
> skb_queue_tail(&dev->rx_frames, skb);
> }
> - skb = NULL;
> -
> - skb2 = skb_dequeue(&dev->rx_frames);
> - while (skb2) {
> - if (status < 0
> - || ETH_HLEN > skb2->len
> - || skb2->len > GETHER_MAX_ETH_FRAME_LEN) {
> - dev->net->stats.rx_errors++;
> - dev->net->stats.rx_length_errors++;
> - DBG(dev, "rx length %d\n", skb2->len);
> - dev_kfree_skb_any(skb2);
> - goto next_frame;
> - }
> - skb2->protocol = eth_type_trans(skb2, dev->net);
> - dev->net->stats.rx_packets++;
> - dev->net->stats.rx_bytes += skb2->len;
> -
> - /* no buffer copies needed, unless hardware can't
> - * use skb buffers.
> - */
> - status = netif_rx(skb2);
> -next_frame:
> - skb2 = skb_dequeue(&dev->rx_frames);
> - }
> + if (!status)
> + queue = 1;
> break;
>
> /* software-driven interface shutdown */
> @@ -329,22 +308,20 @@ quiesce:
> /* FALLTHROUGH */
>
> default:
> + queue = 1;
> + dev_kfree_skb_any(skb);
> dev->net->stats.rx_errors++;
> DBG(dev, "rx status %d\n", status);
> break;
> }
>
> - if (skb)
> - dev_kfree_skb_any(skb);
> - if (!netif_running(dev->net)) {
> clean:
> - spin_lock(&dev->req_lock);
> - list_add(&req->list, &dev->rx_reqs);
> - spin_unlock(&dev->req_lock);
> - req = NULL;
> - }
> - if (req)
> - rx_submit(dev, req, GFP_ATOMIC);
> + spin_lock(&dev->req_lock);
> + list_add(&req->list, &dev->rx_reqs);
> + spin_unlock(&dev->req_lock);
> +
> + if (queue)
> + queue_work(uether_wq, &dev->rx_work);
> }
>
> static int prealloc(struct list_head *list, struct usb_ep *ep, unsigned n)
> @@ -409,16 +386,24 @@ static void rx_fill(struct eth_dev *dev, gfp_t gfp_flags)
> {
> struct usb_request *req;
> unsigned long flags;
> + int req_cnt = 0;
>
> /* fill unused rxq slots with some skb */
> spin_lock_irqsave(&dev->req_lock, flags);
> while (!list_empty(&dev->rx_reqs)) {
> + /* break the nexus of continuous completion and re-submission*/
> + if (++req_cnt > qlen(dev->gadget))
> + break;
> +
> req = container_of(dev->rx_reqs.next,
> struct usb_request, list);
> list_del_init(&req->list);
> spin_unlock_irqrestore(&dev->req_lock, flags);
>
> if (rx_submit(dev, req, gfp_flags) < 0) {
> + spin_lock_irqsave(&dev->req_lock, flags);
> + list_add(&req->list, &dev->rx_reqs);
> + spin_unlock_irqrestore(&dev->req_lock, flags);
> defer_kevent(dev, WORK_RX_MEMORY);
> return;
> }
> @@ -428,6 +413,36 @@ static void rx_fill(struct eth_dev *dev, gfp_t gfp_flags)
> spin_unlock_irqrestore(&dev->req_lock, flags);
> }
>
> +static void process_rx_w(struct work_struct *work)
> +{
> + struct eth_dev *dev = container_of(work, struct eth_dev, rx_work);
> + struct sk_buff *skb;
> + int status = 0;
> +
> + if (!dev->port_usb)
> + return;
> +
> + while ((skb = skb_dequeue(&dev->rx_frames))) {
> + if (status < 0
> + || ETH_HLEN > skb->len
> + || skb->len > ETH_FRAME_LEN) {
> + dev->net->stats.rx_errors++;
> + dev->net->stats.rx_length_errors++;
> + DBG(dev, "rx length %d\n", skb->len);
> + dev_kfree_skb_any(skb);
> + continue;
> + }
> + skb->protocol = eth_type_trans(skb, dev->net);
> + dev->net->stats.rx_packets++;
> + dev->net->stats.rx_bytes += skb->len;
> +
> + status = netif_rx_ni(skb);
> + }
> +
> + if (netif_running(dev->net))
> + rx_fill(dev, GFP_KERNEL);
> +}
> +
> static void eth_work(struct work_struct *work)
> {
> struct eth_dev *dev = container_of(work, struct eth_dev, work);
> @@ -789,6 +804,7 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
> spin_lock_init(&dev->lock);
> spin_lock_init(&dev->req_lock);
> INIT_WORK(&dev->work, eth_work);
> + INIT_WORK(&dev->rx_work, process_rx_w);
> INIT_LIST_HEAD(&dev->tx_reqs);
> INIT_LIST_HEAD(&dev->rx_reqs);
>
> @@ -1134,6 +1150,7 @@ void gether_disconnect(struct gether *link)
> {
> struct eth_dev *dev = link->ioport;
> struct usb_request *req;
> + struct sk_buff *skb;
>
> WARN_ON(!dev);
> if (!dev)
> @@ -1174,6 +1191,12 @@ void gether_disconnect(struct gether *link)
> spin_lock(&dev->req_lock);
> }
> spin_unlock(&dev->req_lock);
> +
> + spin_lock(&dev->rx_frames.lock);
> + while ((skb = __skb_dequeue(&dev->rx_frames)))
> + dev_kfree_skb_any(skb);
> + spin_unlock(&dev->rx_frames.lock);
> +
> link->out_ep->desc = NULL;
>
> /* finish forgetting about this USB link episode */
> @@ -1187,5 +1210,23 @@ void gether_disconnect(struct gether *link)
> }
> EXPORT_SYMBOL_GPL(gether_disconnect);
>
> -MODULE_LICENSE("GPL");
> +static int __init gether_init(void)
> +{
> + uether_wq = create_singlethread_workqueue("uether");
> + if (!uether_wq) {
> + pr_err("%s: Unable to create workqueue: uether\n", __func__);
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +module_init(gether_init);
> +
> +static void __exit gether_exit(void)
> +{
> + destroy_workqueue(uether_wq);
> +
> +}
> +module_exit(gether_exit);
> MODULE_AUTHOR("David Brownell");
> +MODULE_DESCRIPTION("ethernet over USB driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>