From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Date: Sun, 03 Jun 2018 14:41:04 +0000 Subject: [PATCH 11/21] udlfb: fix semaphore value leak Message-Id: <20180603144222.989093650@twibright.com> List-Id: References: <20180603144053.875668929@twibright.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Mikulas Patocka , Bartlomiej Zolnierkiewicz , Dave Airlie , Bernie Thompson , Ladislav Michl Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org 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 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; }; From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mikulas Patocka Subject: [PATCH 11/21] udlfb: fix semaphore value leak Date: Sun, 03 Jun 2018 16:41:04 +0200 Message-ID: <20180603144222.989093650@twibright.com> References: <20180603144053.875668929@twibright.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from leontynka.twibright.com (109-183-129-149.tmcz.cz [109.183.129.149]) by gabe.freedesktop.org (Postfix) with ESMTPS id E955C6E176 for ; Sun, 3 Jun 2018 15:18:44 +0000 (UTC) Content-Disposition: inline; filename=udl-avoid-delayed-work.patch List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Mikulas Patocka , Bartlomiej Zolnierkiewicz , Dave Airlie , Bernie Thompson , Ladislav Michl Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org SSBvYnNlcnZlZCB0aGF0IHRoZSBwZXJmb3JtYW5jZSBvZiB0aGUgdWRsIGZiIGRyaXZlciBkZWdy YWRlcyBvdmVyIHRpbWUuCk9uIGEgZnJlc2hseSBib290ZWQgbWFjaGluZSwgaXQgdGFrZXMgNiBz ZWNvbmRzIHRvIGRvICJscyAtbGEgL3Vzci9iaW4iOwphZnRlciBzb21lIHRpbWUgb2YgdXNlLCB0 aGUgc2FtZSBvcGVyYXRpb24gdGFrZXMgMTQgc2Vjb25kcy4KClRoZSByZWFzb24gaXMgdGhhdCB0 aGUgdmFsdWUgb2YgImxpbWl0X3NlbSIgZGVjYXlzIG92ZXIgdGltZS4KClRoZSB1ZGwgZHJpdmVy IHVzZXMgYSBzZW1hcGhvcmUgImxpbWl0X3NldCIgdG8gc3BlY2lmeSBob3cgbWFueSBmcmVlIHVy YnMKYXJlIHRoZXJlIG9uIGRsZmItPnVyYnMubGlzdC4gSWYgdGhlIGNvdW50IGlzIHplcm8sIHRo ZSAiZG93biIgb3BlcmF0aW9uCndpbGwgc2xlZXAgdW50aWwgc29tZSB1cmJzIGFyZSBhZGRlZCB0 byB0aGUgZnJlZWxpc3QuCgpJbiBvcmRlciB0byBhdm9pZCBzb21lIGh5cG90aGV0aWNhbCBkZWFk bG9jaywgdGhlIGRyaXZlciB3aWxsIG5vdCBjYWxsCiJ1cCIgaW1tZWRpYXRlbGx5LCBidXQgaXQg d2lsbCBvZmZsb2FkIGl0IHRvIGEgd29ya3F1ZXVlLiBUaGUgcHJvYmxlbSBpcwp0aGF0IGlmIHdl IGNhbGwgInNjaGVkdWxlX2RlbGF5ZWRfd29yayIgb24gdGhlIHNhbWUgd29yayBpdGVtIG11bHRp cGxlCnRpbWVzLCB0aGUgd29yayBpdGVtIG1heSBvbmx5IGJlIGV4ZWN1dGVkIG9uY2UuCgpUaGlz IGlzIGhhcHBlbmluZzoKKiBzb21lIHVyYiBjb21wbGV0ZXMKKiBkbGZiX3VyYl9jb21wbGV0aW9u IGFkZHMgaXQgdG8gdGhlIGZyZWUgbGlzdAoqIGRsZmJfdXJiX2NvbXBsZXRpb24gY2FsbHMgc2No ZWR1bGVfZGVsYXllZF93b3JrIHRvIHNjaGVkdWxlIHRoZSBmdW5jdGlvbgogIGRsZmJfcmVsZWFz ZV91cmJfd29yayB0byBpbmNyZWFzZSB0aGUgc2VtYXBob3JlIGNvdW50CiogYXMgdGhlIHVyYiBp cyBvbiB0aGUgZnJlZSBsaXN0LCBzb21lIG90aGVyIHRhc2sgZ3JhYnMgaXQgYW5kIHN1Ym1pdHMg aXQKKiB0aGUgc3VibWl0dGVkIHVyYiBjb21wbGV0ZXMsIGRsZmJfdXJiX2NvbXBsZXRpb24gaXMg Y2FsbGVkIGFnYWluCiogZGxmYl91cmJfY29tcGxldGlvbiBjYWxscyBzY2hlZHVsZV9kZWxheWVk X3dvcmssIGJ1dCB0aGUgd29yayBpcyBhbHJlYWR5CiAgc2NoZWR1bGVkLCBzbyBpdCBkb2VzIG5v dGhpbmcKKiBmaW5hbGx5LCBkbGZiX3JlbGVhc2VfdXJiX3dvcmsgaXMgY2FsbGVkLCBpdCBpbmNy ZWFzZXMgdGhlIHNlbWFwaG9yZQogIGNvdW50IGJ5IDEsIGFsdGhvdWdoIGl0IHNob3VsZCBpbmNy ZWFzZSBpdCBieSAyCgpTbywgdGhlIHNlbWFwaG9yZSBjb3VudCBpcyBkZWNyZWFzaW5nIG92ZXIg dGltZSwgYW5kIHRoaXMgY2F1c2VzIGdyYWR1YWwKcGVyZm9ybWFuY2UgZGVncmFkYXRpb24uCgpO b3RlIHRoYXQgaW4gdGhlIGN1cnJlbnQga2VybmVsLCB0aGUgInVwIiBmdW5jdGlvbiBtYXkgYmUg Y2FsbGVkIGZyb20KaW50ZXJydXB0IGFuZCBpdCBtYXkgcmFjZSB3aXRoIHRoZSAiZG93biIgZnVu Y3Rpb24gY2FsbGVkIGJ5IGFub3RoZXIKdGhyZWFkLCBzbyB3ZSBkb24ndCBoYXZlIHRvIG9mZmxv YWQgdGhlIGNhbGwgb2YgInVwIiB0byBhIHdvcmtxdWV1ZSBhdAphbGwuIFRoaXMgcGF0Y2ggcmVt b3ZlcyB0aGUgd29ya3F1ZXVlIGNvZGUuIFRoZSBwYXRjaCBhbHNvIGNoYW5nZXMKImRvd25faW50 ZXJydXB0aWJsZSIgdG8gImRvd24iIGluIGRsZmJfZnJlZV91cmJfbGlzdCwgc28gdGhhdCB3ZSB3 aWxsCmNsZWFuIHVwIHRoZSBkcml2ZXIgcHJvcGVybHkgZXZlbiBpZiBhIHNpZ25hbCBhcnJpdmVz LgoKV2l0aCB0aGlzIHBhdGNoLCB0aGUgcGVyZm9ybWFuY2Ugb2YgdWRsZmIgbm8gbG9uZ2VyIGRl Z3JhZGVzLgoKU2lnbmVkLW9mZi1ieTogTWlrdWxhcyBQYXRvY2thIDxtcGF0b2NrYUByZWRoYXQu Y29tPgpDYzogc3RhYmxlQHZnZXIua2VybmVsLm9yZwoKLS0tCiBkcml2ZXJzL3ZpZGVvL2ZiZGV2 L3VkbGZiLmMgfCAgIDI3ICsrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQogaW5jbHVkZS92aWRl by91ZGxmYi5oICAgICAgIHwgICAgMSAtCiAyIGZpbGVzIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygr KSwgMjYgZGVsZXRpb25zKC0pCgpJbmRleDogbGludXgtNC4xNy1yYzcvZHJpdmVycy92aWRlby9m YmRldi91ZGxmYi5jCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGxpbnV4LTQuMTctcmM3Lm9yaWcvZHJpdmVycy92 aWRlby9mYmRldi91ZGxmYi5jCTIwMTgtMDUtMzEgMTI6MzE6MDQuMDAwMDAwMDAwICswMjAwCisr KyBsaW51eC00LjE3LXJjNy9kcml2ZXJzL3ZpZGVvL2ZiZGV2L3VkbGZiLmMJMjAxOC0wNS0zMSAx MjozMTowNC4wMDAwMDAwMDAgKzAyMDAKQEAgLTkyMiwxNCArOTIyLDYgQEAgc3RhdGljIHZvaWQg ZGxmYl9mcmVlKHN0cnVjdCBrcmVmICprcmVmKQogCWtmcmVlKGRsZmIpOwogfQogCi1zdGF0aWMg dm9pZCBkbGZiX3JlbGVhc2VfdXJiX3dvcmsoc3RydWN0IHdvcmtfc3RydWN0ICp3b3JrKQotewot CXN0cnVjdCB1cmJfbm9kZSAqdW5vZGUgPSBjb250YWluZXJfb2Yod29yaywgc3RydWN0IHVyYl9u b2RlLAotCQkJCQkgICAgICByZWxlYXNlX3VyYl93b3JrLndvcmspOwotCi0JdXAoJnVub2RlLT5k bGZiLT51cmJzLmxpbWl0X3NlbSk7Ci19Ci0KIHN0YXRpYyB2b2lkIGRsZmJfZnJlZV9mcmFtZWJ1 ZmZlcihzdHJ1Y3QgZGxmYl9kYXRhICpkbGZiKQogewogCXN0cnVjdCBmYl9pbmZvICppbmZvID0g ZGxmYi0+aW5mbzsKQEAgLTE3ODksMTQgKzE3ODEsNyBAQCBzdGF0aWMgdm9pZCBkbGZiX3VyYl9j b21wbGV0aW9uKHN0cnVjdCB1CiAJZGxmYi0+dXJicy5hdmFpbGFibGUrKzsKIAlzcGluX3VubG9j a19pcnFyZXN0b3JlKCZkbGZiLT51cmJzLmxvY2ssIGZsYWdzKTsKIAotCS8qCi0JICogV2hlbiB1 c2luZyBmYl9kZWZpbywgd2UgZGVhZGxvY2sgaWYgdXAoKSBpcyBjYWxsZWQKLQkgKiB3aGlsZSBh bm90aGVyIGlzIHdhaXRpbmcuIFNvIHF1ZXVlIHRvIGFub3RoZXIgcHJvY2Vzcy4KLQkgKi8KLQlp ZiAoZmJfZGVmaW8pCi0JCXNjaGVkdWxlX2RlbGF5ZWRfd29yaygmdW5vZGUtPnJlbGVhc2VfdXJi X3dvcmssIDApOwotCWVsc2UKLQkJdXAoJmRsZmItPnVyYnMubGltaXRfc2VtKTsKKwl1cCgmZGxm Yi0+dXJicy5saW1pdF9zZW0pOwogfQogCiBzdGF0aWMgdm9pZCBkbGZiX2ZyZWVfdXJiX2xpc3Qo c3RydWN0IGRsZmJfZGF0YSAqZGxmYikKQEAgLTE4MDUsMTYgKzE3OTAsMTEgQEAgc3RhdGljIHZv aWQgZGxmYl9mcmVlX3VyYl9saXN0KHN0cnVjdCBkbAogCXN0cnVjdCBsaXN0X2hlYWQgKm5vZGU7 CiAJc3RydWN0IHVyYl9ub2RlICp1bm9kZTsKIAlzdHJ1Y3QgdXJiICp1cmI7Ci0JaW50IHJldDsK IAl1bnNpZ25lZCBsb25nIGZsYWdzOwogCiAJLyoga2VlcCB3YWl0aW5nIGFuZCBmcmVlaW5nLCB1 bnRpbCB3ZSd2ZSBnb3QgJ2VtIGFsbCAqLwogCXdoaWxlIChjb3VudC0tKSB7Ci0KLQkJLyogR2V0 dGluZyBpbnRlcnJ1cHRlZCBtZWFucyBhIGxlYWssIGJ1dCBvayBhdCBkaXNjb25uZWN0ICovCi0J CXJldCA9IGRvd25faW50ZXJydXB0aWJsZSgmZGxmYi0+dXJicy5saW1pdF9zZW0pOwotCQlpZiAo cmV0KQotCQkJYnJlYWs7CisJCWRvd24oJmRsZmItPnVyYnMubGltaXRfc2VtKTsKIAogCQlzcGlu X2xvY2tfaXJxc2F2ZSgmZGxmYi0+dXJicy5sb2NrLCBmbGFncyk7CiAKQEAgLTE4NTQsOSArMTgz NCw2IEBAIHN0YXRpYyBpbnQgZGxmYl9hbGxvY191cmJfbGlzdChzdHJ1Y3QgZGwKIAkJCWJyZWFr OwogCQl1bm9kZS0+ZGxmYiA9IGRsZmI7CiAKLQkJSU5JVF9ERUxBWUVEX1dPUksoJnVub2RlLT5y ZWxlYXNlX3VyYl93b3JrLAotCQkJICBkbGZiX3JlbGVhc2VfdXJiX3dvcmspOwotCiAJCXVyYiA9 IHVzYl9hbGxvY191cmIoMCwgR0ZQX0tFUk5FTCk7CiAJCWlmICghdXJiKSB7CiAJCQlrZnJlZSh1 bm9kZSk7CkluZGV4OiBsaW51eC00LjE3LXJjNy9pbmNsdWRlL3ZpZGVvL3VkbGZiLmgKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gbGludXgtNC4xNy1yYzcub3JpZy9pbmNsdWRlL3ZpZGVvL3VkbGZiLmgJMjAxOC0w NS0zMSAxMjozMTowNC4wMDAwMDAwMDAgKzAyMDAKKysrIGxpbnV4LTQuMTctcmM3L2luY2x1ZGUv dmlkZW8vdWRsZmIuaAkyMDE4LTA1LTMxIDEyOjMxOjA0LjAwMDAwMDAwMCArMDIwMApAQCAtMjAs NyArMjAsNiBAQCBzdHJ1Y3QgZGxvYXJlYSB7CiBzdHJ1Y3QgdXJiX25vZGUgewogCXN0cnVjdCBs aXN0X2hlYWQgZW50cnk7CiAJc3RydWN0IGRsZmJfZGF0YSAqZGxmYjsKLQlzdHJ1Y3QgZGVsYXll ZF93b3JrIHJlbGVhc2VfdXJiX3dvcms7CiAJc3RydWN0IHVyYiAqdXJiOwogfTsKIAoKX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==