On Tue, Sep 19, 2017 at 9:15 AM, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> 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 <dianders@xxxxxxxxxxxx>
Reviewed-by: Guenter Roeck <groeck@xxxxxxxxxxxx>
> ---
>
> drivers/net/usb/usbnet.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 6510e5cc1817..a3e8dbaadcf9 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
