netdev - Re: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Sep 2017 09:43:37 -0700
From:   Guenter Roeck <groeck@...gle.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Oliver Neukum <oneukum@...e.com>,
        Guenter Roeck <groeck@...omium.org>,
        Grant Grundler <grundler@...omium.org>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may
 have been dropped"

On Tue, Sep 19, 2017 at 9:15 AM, Douglas Anderson <dianders@...omium.org> 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@...omium.org>

Reviewed-by: Guenter Roeck <groeck@...omium.org>

> ---
>
>  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
>