[PATCH net-next 0/3] Code adjustment

[PATCH net-next 0/3] Code adjustment

[Hayes Wang]

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

[PATCH net-next 1/3] r8152: remove the duplicate init for the list of rx_done

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next 2/3] r8152: clear the flag of SCHEDULE_TASKLET in tasklet

[Hayes Wang]

Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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

[PATCH net-next 3/3] r8152: check RTL8152_UNPLUG and netif_running before autoresume

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next v2 0/3] Code adjustment

[Hayes Wang]

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

[PATCH net-next v2 1/3] r8152: remove the duplicate init for the list of rx_done

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next v2 2/3] r8152: clear the flag of SCHEDULE_TASKLET in tasklet

[Hayes Wang]

Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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

Re: [PATCH net-next v2 2/3] r8152: clear the flag of SCHEDULE_TASKLET in tasklet

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
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 <hayeswang@realtek.com>
> ---
>  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.

RE: [PATCH net-next v2 2/3] r8152: clear the flagofSCHEDULE_TASKLET in tasklet

[Hayes Wang]

 David Miller [davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 2/3] r8152: clear the flagofSCHEDULE_TASKLET in tasklet

[Francois Romieu]

Hayes Wang <hayeswang@realtek.com> :
>  David Miller [davem@davemloft.net]
[...]
> > 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

RE: [PATCH net-next v2 2/3] r8152: clear theflagofSCHEDULE_TASKLETin tasklet

[Hayes Wang]

 Francois Romieu [mailto:romieu@fr.zoreil.com] 
[...]
> 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

RE: [PATCH net-next v2 2/3] r8152: clear theflagofSCHEDULE_TASKLETin tasklet

[Hayes Wang]

 Francois Romieu [mailto:romieu-W8zweXLXuWQS+FvcfC7Uqw@public.gmane.org] 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 2/3] r8152: clear theflagofSCHEDULE_TASKLETin tasklet

[Francois Romieu]

Hayes Wang <hayeswang@realtek.com> :
>  Francois Romieu [mailto:romieu@fr.zoreil.com] 
> > 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

RE: [PATCH net-next v2 2/3] r8152: cleartheflagofSCHEDULE_TASKLETintasklet

[Hayes Wang]

 Francois Romieu [mailto:romieu-W8zweXLXuWQS+FvcfC7Uqw@public.gmane.org] 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v2 3/3] r8152: check RTL8152_UNPLUG and netif_running before autoresume

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next v3 0/3] Code adjustment

[Hayes Wang]

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

[PATCH net-next v3 1/3] r8152: remove the duplicate init for the list of rx_done

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

[PATCH net-next v3 2/3] r8152: clear the flag of SCHEDULE_TASKLET in tasklet

[Hayes Wang]

Clear the flag of SCHEDULE_TASKLET in bottom_half() to avoid
re-schedule the tasklet again by workqueue.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 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

[PATCH net-next v3 3/3] r8152: check RTL8152_UNPLUG and netif_running before autoresume

[Hayes Wang]

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 <hayeswang@realtek.com>
---
 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

Re: [PATCH net-next v3 0/3] Code adjustment

[David Miller]

From: Hayes Wang <hayeswang@realtek.com>
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.