usbnet: Get rid of spammy usbnet "kevent X may have been dropped"

In general when you've got a flag communicating that "something needs to be done" you want to clear that flag _before_ doing the task. If you clear the flag _after_ doing the task you end up with the risk that this will happen:

1. Requester sets flag saying task A needs to be done.
2. Worker comes and stars doing task A.
3. Worker finishes task A but hasn't yet cleared the flag.
4. Requester wants to set flag saying task A needs to be done again.
5. Worker clears the flag without doing anything.

Let's make the usbnet codebase consistently clear the flag _before_ it does the requested work. That way if there's another request to do the work while the work is already in progress it won't be lost.

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:

Quoted 15 lines

In general when you've got a flag communicating that "something needs to be done" you want to clear that flag _before_ doing the task. If you clear the flag _after_ doing the task you end up with the risk that this will happen:

1. Requester sets flag saying task A needs to be done.
2. Worker comes and stars doing task A.
3. Worker finishes task A but hasn't yet cleared the flag.
4. Requester wants to set flag saying task A needs to be done again.
5. Worker clears the flag without doing anything.

Let's make the usbnet codebase consistently clear the flag _before_ it does the requested work. That way if there's another request to do the work while the work is already in progress it won't be lost.

Quoted 2 lines

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.

Hi,

unfortunately the patch is wrong. The flags must be cleared only in case the handler is successful. That is not guaranteed.

Quoted 3 lines

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.

Hi,

unfortunately the patch is wrong. The flags must be cleared only in case the handler is successful. That is not guaranteed.

Just out of curiosity, what is the retry mechanism ? Whenever a new, possibly unrelated, event is scheduled ?

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:

Quoted 11 lines

In general when you've got a flag communicating that "something needs to be done" you want to clear that flag _before_ doing the task. If you clear the flag _after_ doing the task you end up with the risk that this will happen:

1. Requester sets flag saying task A needs to be done.
2. Worker comes and stars doing task A.
3. Worker finishes task A but hasn't yet cleared the flag.
4. Requester wants to set flag saying task A needs to be done again.
5. Worker clears the flag without doing anything.

Quoted 6 lines

Let's make the usbnet codebase consistently clear the flag _before_ it does the requested work. That way if there's another request to do the work while the work is already in progress it won't be lost.

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.

Hi,

unfortunately the patch is wrong. The flags must be cleared only in case the handler is successful. That is not guaranteed.

Regards
Oliver

NACK

Hi,

Quoted 29 lines · Oliver Neukum

Am Dienstag, den 19.09.2017, 09:15 -0700 schrieb Douglas Anderson:

Quoted 12 lines

In general when you've got a flag communicating that "something needs to be done" you want to clear that flag _before_ doing the task. If you clear the flag _after_ doing the task you end up with the risk that this will happen:

1. Requester sets flag saying task A needs to be done.
2. Worker comes and stars doing task A.
3. Worker finishes task A but hasn't yet cleared the flag.
4. Requester wants to set flag saying task A needs to be done again.
5. Worker clears the flag without doing anything.

Quoted 6 lines

Let's make the usbnet codebase consistently clear the flag _before_ it does the requested work. That way if there's another request to do the work while the work is already in progress it won't be lost.

NOTES:
- No known bugs are fixed by this; it's just found by code inspection.

Hi,

unfortunately the patch is wrong. The flags must be cleared only in case the handler is successful. That is not guaranteed.

Regards
Oliver

Quoted 1 line

NACK

OK, thanks for reviewing! I definitely wasn't super confident about the patch (hence the RFC).

Do you think that the races I identified are possible to hit? In

other words: should I try to rework the patch somehow or just drop it?
Originally I had the patch setting the flags back to true in the
failure cases, but then I convinced myself that wasn't needed. I can certainly go back and try it that way...

If rx_submit() returns an error code then nobody calls usb_free_urb(). That means it's leaked.

Every once in a while when my system is under a bit of stress I see some spammy messages show up in my logs that say:

kevent X may have been dropped

As far as I can tell these messages aren't terribly useful. The comments around the messages make me think that either workqueues used to work differently or that the original author of the code missed a sublety related to them. The error message appears to predate the git conversion of the kernel so it's somewhat hard to tell.

Specifically, workqueues should work like this:

A) If a workqueue hasn't been scheduled then schedule_work() schedules it and returns true.

B) If a workqueue has been scheduled (but hasn't started) then schedule_work() will do nothing and return false.

C) If a workqueue has been scheduled (and has started) then schedule_work() will put it on the queue to run again and return true.

Said another way: if you call schedule_work() you can guarantee that at least one full runthrough of the work will happen again. That should mean that the work will get processed and I don't see any reason to think something should be dropped.

Reading the comments in in usbnet_defer_kevent() made me think that B) and C) would be treated the same. That is: even if we've started the work and are 99% of the way through then schedule_work() would return false and the work wouldn't be queued again. If schedule_work() really did behave that way then, truly, some amount of work would be lost. ...but it doesn't.

NOTE: if somehow these warnings are useful to mean something then perhaps we should change them to make it more obvious. If it's interesting to know when the work is backlogged then we should change the spam to say "warning: usbnet is backlogged".

ALSO NOTE: If somehow some of the types of work need to be repeated if usbnet_defer_kevent() is called multiple times then that should be quite easy to accomplish without dropping any work on the floor. We can just keep an atomic count for that type of work and add a loop into usbnet_deferred_kevent().

Signed-off-by Douglas Anderson <[email protected]>

---

 drivers/net/usb/usbnet.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

drivers/net/usb/usbnet.c +7 -10

index 6510e5cc1817..a3e8dbaadcf9 100644
@@ -450,19 +450,17 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb,
 }
 
 /* some work can't be done in tasklets, so we use keventd
- *
- * NOTE:  annoying asymmetry:  if it's active, schedule_work() fails,
- * but tasklet_schedule() doesn't.  hope the failure is rare.
  */
 void usbnet_defer_kevent (struct usbnet *dev, int work)
 {
        set_bit (work, &dev->flags);
-       if (!schedule_work (&dev->kevent)) {
-               if (net_ratelimit())
-                       netdev_err(dev->net, "kevent %d may have been dropped\n", work);
-       } else {
-               netdev_dbg(dev->net, "kevent %d scheduled\n", work);
-       }
+
+       /* If work is already started this will mark it to run again when it
+        * finishes; if we already had work pending and it hadn't started
+        * yet then that's fine too.
+        */
+       schedule_work (&dev->kevent);
+       netdev_dbg(dev->net, "kevent %d scheduled\n", work);
 }
 EXPORT_SYMBOL_GPL(usbnet_defer_kevent);
 
--

2.14.1.690.gbb1197296e-goog

Every once in a while when my system is under a bit of stress I see some spammy messages show up in my logs that say:

kevent X may have been dropped

As far as I can tell these messages aren't terribly useful.

ALSO NOTE: If somehow some of the types of work need to be repeated if usbnet_defer_kevent() is called multiple times then that should be quite easy to accomplish without dropping any work on the floor.  We can just keep an atomic count for that type of work and add a loop into usbnet_deferred_kevent().

Signed-off-by Douglas Anderson <[email protected]>