Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1329527 > unrolled thread
| Started by | Amit Pundir <amit.pundir@linaro.org> |
|---|---|
| First post | 2016-02-08 21:20 +0100 |
| Last post | 2016-02-09 19:30 +0100 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path Amit Pundir <amit.pundir@linaro.org> - 2016-02-08 21:20 +0100
Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path Amit Pundir <amit.pundir@linaro.org> - 2016-02-08 21:40 +0100
Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-09 00:30 +0100
Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path Amit Pundir <amit.pundir@linaro.org> - 2016-02-09 17:50 +0100
Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-09 19:30 +0100
| From | Amit Pundir <amit.pundir@linaro.org> |
|---|---|
| Date | 2016-02-08 21:20 +0100 |
| Subject | [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path |
| Message-ID | <r00rU-1Mq-19@gated-at.bofh.it> |
From: Badhri Jagan Sridharan <Badhri@google.com>
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@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mike Looijmans <mike.looijmans@topic.nl>
Cc: Robert Baldyga <r.baldyga@samsung.com>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
[pundir: cherry-picked this patch from AOSP experimental/android-4.4 tree.]
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
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
[toc] | [next] | [standalone]
| From | Amit Pundir <amit.pundir@linaro.org> |
|---|---|
| Date | 2016-02-08 21:40 +0100 |
| Subject | Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path |
| Message-ID | <r00Lg-1TO-7@gated-at.bofh.it> |
| In reply to | #1329527 |
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@linaro.org> wrote:
> From: Badhri Jagan Sridharan <Badhri@google.com>
>
> 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@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mike Looijmans <mike.looijmans@topic.nl>
> Cc: Robert Baldyga <r.baldyga@samsung.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
> [pundir: cherry-picked this patch from AOSP experimental/android-4.4 tree.]
> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> ---
> 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
>
[toc] | [prev] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2016-02-09 00:30 +0100 |
| Subject | Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path |
| Message-ID | <r03pM-3Ek-9@gated-at.bofh.it> |
| In reply to | #1329540 |
On Tue, Feb 09, 2016 at 02:07:02AM +0530, Amit Pundir wrote: > Please ignore this one too. I should have build tested these patches > individually and not in particular series. I'll resend this patch. Send them in a numbered series so we know what order they have to be applied in. And I always ignore RFC patches, if you can't be confident enough in submitting it for inclusion, why should we care? :) You have of course tested these, right? thanks, greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Amit Pundir <amit.pundir@linaro.org> |
|---|---|
| Date | 2016-02-09 17:50 +0100 |
| Subject | Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path |
| Message-ID | <r0jEe-6IG-25@gated-at.bofh.it> |
| In reply to | #1329683 |
On 9 February 2016 at 04:50, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Feb 09, 2016 at 02:07:02AM +0530, Amit Pundir wrote: >> Please ignore this one too. I should have build tested these patches >> individually and not in particular series. I'll resend this patch. > > Send them in a numbered series so we know what order they have to be > applied in. Thanks I'll send them in a numbered series again. Since the patch series didn't have much in common(feature wise), I changed my mind right at the last moment to send them individually but that didn't go well. > > And I always ignore RFC patches, if you can't be confident enough in > submitting it for inclusion, why should we care? :) Yes I got your point. I was not intending to submit it yet, but hoping to get any early feedback or objections from maintainers. > > You have of course tested these, right? I have tested some of it for regressions, though not on the latest kernel, but I will go through and re-test what I can. As I said I was mainly just hoping for early maintainer feedback here. Thank you for the suggestions and feedback Greg. Regards, Amit Pundir > > thanks, > > greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2016-02-09 19:30 +0100 |
| Subject | Re: [RFC][PATCH] usb: gadget: u_ether: Add workqueue as bottom half handler for rx data path |
| Message-ID | <r0ld0-7On-17@gated-at.bofh.it> |
| In reply to | #1330457 |
On Tue, Feb 09, 2016 at 10:17:46PM +0530, Amit Pundir wrote: > On 9 February 2016 at 04:50, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Tue, Feb 09, 2016 at 02:07:02AM +0530, Amit Pundir wrote: > >> Please ignore this one too. I should have build tested these patches > >> individually and not in particular series. I'll resend this patch. > > > > Send them in a numbered series so we know what order they have to be > > applied in. > > Thanks I'll send them in a numbered series again. Since the patch > series didn't have much in common(feature wise), I changed my mind > right at the last moment to send them individually but that didn't go > well. > > > > > And I always ignore RFC patches, if you can't be confident enough in > > submitting it for inclusion, why should we care? :) > > Yes I got your point. I was not intending to submit it yet, but hoping > to get any early feedback or objections from maintainers. Again, if you don't care enough to think it is submittable, why should we? :) Don't expect us to do your review and analysis work for you. thanks, greg k-h
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web