netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: Oliver Neukum <oneukum@suse.com>
Cc: groeck@chromium.org, grundler@chromium.org,
	Douglas Anderson <dianders@chromium.org>,
	netdev@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped"
Date: Tue, 19 Sep 2017 09:15:20 -0700	[thread overview]
Message-ID: <20170919161522.995-1-dianders@chromium.org> (raw)

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@chromium.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

             reply	other threads:[~2017-09-19 16:15 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-19 16:15 Douglas Anderson [this message]
2017-09-19 16:15 ` [RFC PATCH 2/3] usbnet: Avoid potential races in usbnet_deferred_kevent() Douglas Anderson
2017-09-19 20:37   ` Oliver Neukum
2017-09-19 20:51     ` Guenter Roeck
2017-09-20  8:23       ` Oliver Neukum
2017-09-19 20:53     ` Doug Anderson
2017-09-20  8:25       ` Oliver Neukum
2017-09-19 16:15 ` [RFC PATCH 3/3] usbnet: Fix memory leak when rx_submit() fails Douglas Anderson
2017-09-19 17:41   ` Bjørn Mork
2017-09-19 16:43 ` [RFC PATCH 1/3] usbnet: Get rid of spammy usbnet "kevent X may have been dropped" Guenter Roeck
2017-09-19 17:45 ` Bjørn Mork
2017-09-19 20:36 ` Oliver Neukum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170919161522.995-1-dianders@chromium.org \
    --to=dianders@chromium.org \
    --cc=groeck@chromium.org \
    --cc=grundler@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=oneukum@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).