linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mikulas Patocka <mpatocka@redhat.com>
To: Mikulas Patocka <mpatocka@redhat.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Dave Airlie <airlied@redhat.com>,
	Bernie Thompson <bernie@plugable.com>,
	Ladislav Michl <ladis@linux-mips.org>
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: [PATCH 11/21] udlfb: fix semaphore value leak
Date: Sun, 03 Jun 2018 14:41:04 +0000	[thread overview]
Message-ID: <20180603144222.989093650@twibright.com> (raw)
In-Reply-To: 20180603144053.875668929@twibright.com

I observed that the performance of the udl fb driver degrades over time.
On a freshly booted machine, it takes 6 seconds to do "ls -la /usr/bin";
after some time of use, the same operation takes 14 seconds.

The reason is that the value of "limit_sem" decays over time.

The udl driver uses a semaphore "limit_set" to specify how many free urbs
are there on dlfb->urbs.list. If the count is zero, the "down" operation
will sleep until some urbs are added to the freelist.

In order to avoid some hypothetical deadlock, the driver will not call
"up" immediatelly, but it will offload it to a workqueue. The problem is
that if we call "schedule_delayed_work" on the same work item multiple
times, the work item may only be executed once.

This is happening:
* some urb completes
* dlfb_urb_completion adds it to the free list
* dlfb_urb_completion calls schedule_delayed_work to schedule the function
  dlfb_release_urb_work to increase the semaphore count
* as the urb is on the free list, some other task grabs it and submits it
* the submitted urb completes, dlfb_urb_completion is called again
* dlfb_urb_completion calls schedule_delayed_work, but the work is already
  scheduled, so it does nothing
* finally, dlfb_release_urb_work is called, it increases the semaphore
  count by 1, although it should increase it by 2

So, the semaphore count is decreasing over time, and this causes gradual
performance degradation.

Note that in the current kernel, the "up" function may be called from
interrupt and it may race with the "down" function called by another
thread, so we don't have to offload the call of "up" to a workqueue at
all. This patch removes the workqueue code. The patch also changes
"down_interruptible" to "down" in dlfb_free_urb_list, so that we will
clean up the driver properly even if a signal arrives.

With this patch, the performance of udlfb no longer degrades.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/video/fbdev/udlfb.c |   27 ++-------------------------
 include/video/udlfb.h       |    1 -
 2 files changed, 2 insertions(+), 26 deletions(-)

Index: linux-4.17-rc7/drivers/video/fbdev/udlfb.c
=================================--- linux-4.17-rc7.orig/drivers/video/fbdev/udlfb.c	2018-05-31 12:31:04.000000000 +0200
+++ linux-4.17-rc7/drivers/video/fbdev/udlfb.c	2018-05-31 12:31:04.000000000 +0200
@@ -922,14 +922,6 @@ static void dlfb_free(struct kref *kref)
 	kfree(dlfb);
 }
 
-static void dlfb_release_urb_work(struct work_struct *work)
-{
-	struct urb_node *unode = container_of(work, struct urb_node,
-					      release_urb_work.work);
-
-	up(&unode->dlfb->urbs.limit_sem);
-}
-
 static void dlfb_free_framebuffer(struct dlfb_data *dlfb)
 {
 	struct fb_info *info = dlfb->info;
@@ -1789,14 +1781,7 @@ static void dlfb_urb_completion(struct u
 	dlfb->urbs.available++;
 	spin_unlock_irqrestore(&dlfb->urbs.lock, flags);
 
-	/*
-	 * When using fb_defio, we deadlock if up() is called
-	 * while another is waiting. So queue to another process.
-	 */
-	if (fb_defio)
-		schedule_delayed_work(&unode->release_urb_work, 0);
-	else
-		up(&dlfb->urbs.limit_sem);
+	up(&dlfb->urbs.limit_sem);
 }
 
 static void dlfb_free_urb_list(struct dlfb_data *dlfb)
@@ -1805,16 +1790,11 @@ static void dlfb_free_urb_list(struct dl
 	struct list_head *node;
 	struct urb_node *unode;
 	struct urb *urb;
-	int ret;
 	unsigned long flags;
 
 	/* keep waiting and freeing, until we've got 'em all */
 	while (count--) {
-
-		/* Getting interrupted means a leak, but ok at disconnect */
-		ret = down_interruptible(&dlfb->urbs.limit_sem);
-		if (ret)
-			break;
+		down(&dlfb->urbs.limit_sem);
 
 		spin_lock_irqsave(&dlfb->urbs.lock, flags);
 
@@ -1854,9 +1834,6 @@ static int dlfb_alloc_urb_list(struct dl
 			break;
 		unode->dlfb = dlfb;
 
-		INIT_DELAYED_WORK(&unode->release_urb_work,
-			  dlfb_release_urb_work);
-
 		urb = usb_alloc_urb(0, GFP_KERNEL);
 		if (!urb) {
 			kfree(unode);
Index: linux-4.17-rc7/include/video/udlfb.h
=================================--- linux-4.17-rc7.orig/include/video/udlfb.h	2018-05-31 12:31:04.000000000 +0200
+++ linux-4.17-rc7/include/video/udlfb.h	2018-05-31 12:31:04.000000000 +0200
@@ -20,7 +20,6 @@ struct dloarea {
 struct urb_node {
 	struct list_head entry;
 	struct dlfb_data *dlfb;
-	struct delayed_work release_urb_work;
 	struct urb *urb;
 };
 


  parent reply	other threads:[~2018-06-03 14:41 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-03 14:40 [PATCH 00/21] USB DisplayLink patches Mikulas Patocka
2018-06-03 14:40 ` [PATCH 01/21] udl-kms: fix display corruption of the last line Mikulas Patocka
2018-06-03 14:40 ` [PATCH 02/21] udl-kms: change down_interruptible to down Mikulas Patocka
2018-06-03 14:40 ` [PATCH 03/21] udl-kms: handle allocation failure Mikulas Patocka
2018-06-03 14:40 ` [PATCH 04/21] udl-kms: fix crash due to uninitialized memory Mikulas Patocka
2018-06-03 14:40 ` [PATCH 05/21] udl-kms: fix a linked-list corruption when using fbdefio Mikulas Patocka
2018-06-03 14:40 ` [PATCH 06/21] udl-kms: make a local copy of fb_ops Mikulas Patocka
2018-06-03 14:41 ` [PATCH 07/21] udl-kms: avoid division Mikulas Patocka
2018-06-03 14:41 ` [PATCH 08/21] udl-kms: avoid prefetch Mikulas Patocka
2018-06-05 10:08   ` Alexey Brodkin
2018-06-05 10:48     ` Ladislav Michl
2018-06-05 15:30     ` Mikulas Patocka
2018-06-06 12:04       ` Alexey Brodkin
2018-06-06 15:46         ` Mikulas Patocka
2018-06-15 16:30           ` Alexey Brodkin
2018-06-03 14:41 ` [PATCH 09/21] udl-kms: use spin_lock_irq instead of spin_lock_irqsave Mikulas Patocka
2018-06-03 14:41 ` [PATCH 10/21] udl-kms: dont spam the syslog with debug messages Mikulas Patocka
2018-06-03 14:41 ` Mikulas Patocka [this message]
2018-06-03 14:41 ` [PATCH 12/21] udlfb: fix display corruption of the last line Mikulas Patocka
2018-06-03 14:41 ` [PATCH 13/21] udlfb: dont switch if we are switching to the same videomode Mikulas Patocka
2018-06-03 14:41 ` [PATCH 14/21] udlfb: make a local copy of fb_ops Mikulas Patocka
2018-06-03 14:41 ` [PATCH 15/21] udlfb: set optimal write delay Mikulas Patocka
2018-06-03 14:41 ` [PATCH 16/21] udlfb: handle allocation failure Mikulas Patocka
2018-06-03 14:41 ` [PATCH 17/21] udlfb: set line_length in dlfb_ops_set_par Mikulas Patocka
2018-06-03 14:41 ` [PATCH 18/21] udlfb: allow reallocating the framebuffer Mikulas Patocka
2018-06-03 19:24   ` kbuild test robot
2018-06-12 16:32     ` Mikulas Patocka
2018-07-03 14:58       ` Bartlomiej Zolnierkiewicz
2018-06-03 14:41 ` [PATCH 19/21] udlfb: optimization - test the backing buffer Mikulas Patocka
2018-06-03 14:41 ` [PATCH 20/21] udlfb: avoid prefetch Mikulas Patocka
2018-06-03 14:41 ` [PATCH 21/21] udlfb: use spin_lock_irq instead of spin_lock_irqsave Mikulas Patocka
2018-06-04  1:25 ` [PATCH 00/21] USB DisplayLink patches Dave Airlie
2018-06-04 14:14   ` Mikulas Patocka
2018-07-04  8:04     ` Daniel Vetter
2018-06-05  9:47 ` Alexey Brodkin
2018-06-05 15:34   ` Mikulas Patocka
2018-07-25 13:40     ` Bartlomiej Zolnierkiewicz

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=20180603144222.989093650@twibright.com \
    --to=mpatocka@redhat.com \
    --cc=airlied@redhat.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=bernie@plugable.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ladis@linux-mips.org \
    --cc=linux-fbdev@vger.kernel.org \
    /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).