From patchwork Sun Jun 3 14:41:04 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikulas Patocka X-Patchwork-Id: 10445353 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 71552603B4 for ; Sun, 3 Jun 2018 15:18:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 610A7289ED for ; Sun, 3 Jun 2018 15:18:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 55AF9289FA; Sun, 3 Jun 2018 15:18:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id ED19928ACB for ; Sun, 3 Jun 2018 15:18:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id ECCB56E176; Sun, 3 Jun 2018 15:18:46 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org 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) Received: from root by leontynka.twibright.com with local (Exim 4.89) (envelope-from ) id 1fPUD9-0003t1-90; Sun, 03 Jun 2018 16:42:23 +0200 Message-Id: <20180603144222.989093650@twibright.com> User-Agent: quilt/0.63-1 Date: Sun, 03 Jun 2018 16:41:04 +0200 From: Mikulas Patocka To: Mikulas Patocka , Bartlomiej Zolnierkiewicz , Dave Airlie , Bernie Thompson , Ladislav Michl Subject: [PATCH 11/21] udlfb: fix semaphore value leak References: <20180603144053.875668929@twibright.com> MIME-Version: 1.0 Content-Disposition: inline; filename=udl-avoid-delayed-work.patch X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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; };