Adjust some codes to make them more reasonable.
Hayes Wang (3):
r8152: remove the duplicate the init for the list of rx_done
r8152: clear the flag of SCHEDULE_TASKLET in tasklet
r8152: check RTL8152_UNPLUG and netif_running before autoresume
drivers/net/usb/r8152.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
--
1.9.3
The INIT_LIST_HEAD(&tp->rx_done) would be done in rtl_start_rx(),
so remove the unnecessary one in alloc_all_mem().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f116335..ff54098 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1256,7 +1256,6 @@ static int alloc_all_mem(struct r8152 *tp)
spin_lock_init(&tp->rx_lock);
spin_lock_init(&tp->tx_lock);
- INIT_LIST_HEAD(&tp->rx_done);
INIT_LIST_HEAD(&tp->tx_free);
skb_queue_head_init(&tp->tx_queue);
--
1.9.3
If the device is unplugged or !netif_running(), the workqueue
doesn't neet to wake the device, and could return directly.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 670279a..e522abe 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2859,15 +2859,18 @@ static void rtl_work_func_t(struct work_struct *work)
{
struct r8152 *tp = container_of(work, struct r8152, schedule.work);
+ /* If the device is unplugged or !netif_running(), the workqueue
+ * doesn't neet to wake the device, and could return directly.
+ */
+ if (test_bit(RTL8152_UNPLUG, &tp->flags) || !netif_running(tp->netdev))
+ return;
+
if (usb_autopm_get_interface(tp->intf) < 0)
return;
if (!test_bit(WORK_ENABLE, &tp->flags))
goto out1;
- if (test_bit(RTL8152_UNPLUG, &tp->flags))
- goto out1;
-
if (!mutex_trylock(&tp->control)) {
schedule_delayed_work(&tp->schedule, 0);
goto out1;
--
1.9.3
Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ff54098..670279a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1798,6 +1798,9 @@ static void bottom_half(unsigned long data)
if (!netif_carrier_ok(tp->netdev))
return;
+ if (test_bit(SCHEDULE_TASKLET, &tp->flags))
+ clear_bit(SCHEDULE_TASKLET, &tp->flags);
+
rx_bottom(tp);
tx_bottom(tp);
}
--
1.9.3
v2:
Correct the spelling error for the comment of patch #3.
v1:
Adjust some codes to make them more reasonable.
Hayes Wang (3):
r8152: remove the duplicate init for the list of rx_done
r8152: clear the flag of SCHEDULE_TASKLET in tasklet
r8152: check RTL8152_UNPLUG and netif_running before autoresume
drivers/net/usb/r8152.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
--
1.9.3
Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ff54098..670279a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1798,6 +1798,9 @@ static void bottom_half(unsigned long data)
if (!netif_carrier_ok(tp->netdev))
return;
+ if (test_bit(SCHEDULE_TASKLET, &tp->flags))
+ clear_bit(SCHEDULE_TASKLET, &tp->flags);
+
rx_bottom(tp);
tx_bottom(tp);
}
--
1.9.3
If the device is unplugged or !netif_running(), the workqueue
doesn't need to wake the device, and could return directly.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 670279a..90cc89c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2859,15 +2859,18 @@ static void rtl_work_func_t(struct work_struct *work)
{
struct r8152 *tp = container_of(work, struct r8152, schedule.work);
+ /* If the device is unplugged or !netif_running(), the workqueue
+ * doesn't need to wake the device, and could return directly.
+ */
+ if (test_bit(RTL8152_UNPLUG, &tp->flags) || !netif_running(tp->netdev))
+ return;
+
if (usb_autopm_get_interface(tp->intf) < 0)
return;
if (!test_bit(WORK_ENABLE, &tp->flags))
goto out1;
- if (test_bit(RTL8152_UNPLUG, &tp->flags))
- goto out1;
-
if (!mutex_trylock(&tp->control)) {
schedule_delayed_work(&tp->schedule, 0);
goto out1;
--
1.9.3
The INIT_LIST_HEAD(&tp->rx_done) would be done in rtl_start_rx(),
so remove the unnecessary one in alloc_all_mem().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f116335..ff54098 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1256,7 +1256,6 @@ static int alloc_all_mem(struct r8152 *tp)
spin_lock_init(&tp->rx_lock);
spin_lock_init(&tp->tx_lock);
- INIT_LIST_HEAD(&tp->rx_done);
INIT_LIST_HEAD(&tp->tx_free);
skb_queue_head_init(&tp->tx_queue);
--
1.9.3
From: Hayes Wang <[email protected]>
Date: Fri, 31 Oct 2014 17:56:41 +0800
> Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
> re-schedule the tasklet again by workqueue.
>
> Signed-off-by: Hayes Wang <[email protected]>
> ---
> drivers/net/usb/r8152.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index ff54098..670279a 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1798,6 +1798,9 @@ static void bottom_half(unsigned long data)
> if (!netif_carrier_ok(tp->netdev))
> return;
>
> + if (test_bit(SCHEDULE_TASKLET, &tp->flags))
> + clear_bit(SCHEDULE_TASKLET, &tp->flags);
This is racey.
If another thread of control sets the bit between the test and the
clear, you will lose an event.
It really never makes sense to work with atomic bitops in a non-atomic
test-and-whatever manner like this, it's always a red flag and
indicates you're doing something very wrong.
David Miller [[email protected]]
> This is racey.
> If another thread of control sets the bit between the test and the
> clear, you will lose an event.
It is fine. The flag is used to schedule a tasklet, so if the tasklet is
starting running, all the other plans for scheduling a tasklet could
be cleared.
Besides, the flag is only set when a transmission occurs and the
device is in autosuspend mode. Then the workqueue could wake
up the device and schedule the tasklet to make sure the tasklet
wouldn't be run when the device is in suspend mode. Therefore,
if the tasklet is running, it means something happens and the
device is waked up. And the queue for scheduling the tasklet is
unnecessary. We don't need the tasklet runs again after current
one except that the relative tx/rx flows schedule it.
> It really never makes sense to work with atomic bitops in a non-atomic
> test-and-whatever manner like this, it's always a red flag and
> indicates you're doing something very wrong.
I use atomic because I don't wish the different threads which
set the different flags would chang the other bits which they
don't interesting in.
Best Regards,
Hayes
Hayes Wang <[email protected]> :
> David Miller [[email protected]]
[...]
> > If another thread of control sets the bit between the test and the
> > clear, you will lose an event.
>
> It is fine. The flag is used to schedule a tasklet, so if the tasklet is
> starting running, all the other plans for scheduling a tasklet could
> be cleared.
test_and_clear_bit (dense) or clear_bit would be more idiomatic.
--
Ueimor
Francois Romieu [mailto:[email protected]]
[...]
> test_and_clear_bit (dense) or clear_bit would be more idiomatic.
Excuse me. If I use clear_bit without test_bit or test_and_clear_bit,
they alwayes call the spin lock. However, for my original flow, the spin
lock is only called when the clear_bit is necessary. Is that better?
Best Regards,
Hayes
Francois Romieu [mailto:[email protected]]
> Sent: Monday, November 03, 2014 6:53 AM
[...]
> test_and_clear_bit (dense) or clear_bit would be more idiomatic.
Excuse me. Any suggestion?
Should I use clear_bit directly, or something else?
Or, do I have to remove this patch?
Best Regards,
Hayes
Hayes Wang <[email protected]> :
> Francois Romieu [mailto:[email protected]]
> > Sent: Monday, November 03, 2014 6:53 AM
> [...]
> > test_and_clear_bit (dense) or clear_bit would be more idiomatic.
>
> Excuse me. Any suggestion?
> Should I use clear_bit directly, or something else?
> Or, do I have to remove this patch?
The performance explanation leaves me a bit unconvinced. Without any
figure one could simply go for the always locked clear_bit because of:
1. the "I'm racy" message that the open-coded test + set sends
2. the extra work needed to avoid 1 (comment, explain, ...).
The extra time could thus be used to see what happens when napi is
shoehorned in this tasklet machinery. I'd naively expect it to be
relevant for efficiency.
I won't mind if your agenda is completely different. :o)
--
Ueimor
Francois Romieu [mailto:[email protected]]
> Sent: Sunday, November 09, 2014 6:12 AM
[...]
> The performance explanation leaves me a bit unconvinced. Without any
> figure one could simply go for the always locked clear_bit because of:
> 1. the "I'm racy" message that the open-coded test + set sends
> 2. the extra work needed to avoid 1 (comment, explain, ...).
Thanks. I would modify this patch with clear_bit only.
> The extra time could thus be used to see what happens when napi is
> shoehorned in this tasklet machinery. I'd naively expect it to be
> relevant for efficiency.
I thought about NAPI, but I gave up. The reasons are
1. I don't sure if it would run when autosuspending.
2. There is no hw interrupt for USB device. And I have
no idea about how to check if the USB transfer is
completed by polling.
3. I have to control the rx packets numbers in poll().
However, I couldn't control the packets number for
each bulk-in transfer. I have to do extra works to
deal with the rx flow.
4. I don't find much different between tasklet and NAPI.
Best Regards,
Hayes
The INIT_LIST_HEAD(&tp->rx_done) would be done in rtl_start_rx(),
so remove the unnecessary one in alloc_all_mem().
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 66b139a..a300467 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1255,7 +1255,6 @@ static int alloc_all_mem(struct r8152 *tp)
spin_lock_init(&tp->rx_lock);
spin_lock_init(&tp->tx_lock);
- INIT_LIST_HEAD(&tp->rx_done);
INIT_LIST_HEAD(&tp->tx_free);
skb_queue_head_init(&tp->tx_queue);
--
1.9.3
If the device is unplugged or !netif_running(), the workqueue
doesn't need to wake the device, and could return directly.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ad9dd7d..0a30fd3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -2857,15 +2857,18 @@ static void rtl_work_func_t(struct work_struct *work)
{
struct r8152 *tp = container_of(work, struct r8152, schedule.work);
+ /* If the device is unplugged or !netif_running(), the workqueue
+ * doesn't need to wake the device, and could return directly.
+ */
+ if (test_bit(RTL8152_UNPLUG, &tp->flags) || !netif_running(tp->netdev))
+ return;
+
if (usb_autopm_get_interface(tp->intf) < 0)
return;
if (!test_bit(WORK_ENABLE, &tp->flags))
goto out1;
- if (test_bit(RTL8152_UNPLUG, &tp->flags))
- goto out1;
-
if (!mutex_trylock(&tp->control)) {
schedule_delayed_work(&tp->schedule, 0);
goto out1;
--
1.9.3
v3:
Remove the test_bit for patch #2.
v2:
Correct the spelling error for the comment of patch #3.
v1:
Adjust some codes to make them more reasonable.
Hayes Wang (3):
r8152: remove the duplicate init for the list of rx_done
r8152: clear the flag of SCHEDULE_TASKLET in tasklet
r8152: check RTL8152_UNPLUG and netif_running before autoresume
drivers/net/usb/r8152.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
--
1.9.3
Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.
Signed-off-by: Hayes Wang <[email protected]>
---
drivers/net/usb/r8152.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index a300467..ad9dd7d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1797,6 +1797,8 @@ static void bottom_half(unsigned long data)
if (!netif_carrier_ok(tp->netdev))
return;
+ clear_bit(SCHEDULE_TASKLET, &tp->flags);
+
rx_bottom(tp);
tx_bottom(tp);
}
--
1.9.3
From: Hayes Wang <[email protected]>
Date: Wed, 12 Nov 2014 10:05:02 +0800
> v3:
> Remove the test_bit for patch #2.
>
> v2:
> Correct the spelling error for the comment of patch #3.
>
> v1:
> Adjust some codes to make them more reasonable.
Series applied, thanks.