| Message ID | 1394712342-15778-81-Taiwan-albertk@realtek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller [davem@davemloft.net] > 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
Francois Romieu [mailto:romieu@fr.zoreil.com] > 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(+)