From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48558C07E96 for ; Thu, 8 Jul 2021 11:19:21 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 042746161F for ; Thu, 8 Jul 2021 11:19:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 042746161F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 872F46E1F1; Thu, 8 Jul 2021 11:19:20 +0000 (UTC) Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 69E756E1F1 for ; Thu, 8 Jul 2021 11:19:19 +0000 (UTC) Received: by mail-wr1-x42b.google.com with SMTP id i8so7010950wrp.12 for ; Thu, 08 Jul 2021 04:19:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=GBRiAHW1IiqIOt5/V0BSXc3KAEsqTESG/NcSN6alwWI=; b=atWo1G0qn2I4JBsi3YemImo6akK/Z24zauZSPL1v4HpH+8oZVEczBpiY1RxQbSowl0 JNXQdJcA9Vy+krFW5/c8uerzm9kiorwa9Efg7K1M7TdH69zKMX4kmVDYeCFO2yJjizHT z1rDNeKSMv4dXnyVDSzDPG/rCNv4cgjt4d+5SYmXZO/8WUswobLm9my8oSzDUNxNk5VW hLBAT8mMet/urj1v7vFvXnL6faUFTHKzH6dbasW0VNN1+LFzb3T1/4ABVIbCOeKo4pTi 22XVA4CCw/Pfn7JawfC3PZW7TtcbdM08XHPUkLH+MGNC0LgkrFGOt/ZukTYYxc6wNXe1 sMXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=GBRiAHW1IiqIOt5/V0BSXc3KAEsqTESG/NcSN6alwWI=; b=VdU41BW6+RmPkerVXiLsCVSFsus3BsB2upDNVf+GCobGviIQ6SNU03AKElyiZ8qLnf tG32J323X3t+86wnO7zp+1Amu/XGNZn6KVkwVQkzpLm30R8Kydp/zhfe9iZB7iYOOs+q JkazzseCKDnyVj9eUsMRtxG5ENnnD/+2Js8JLKBfct0xuq2msmSUb2AYMlXx78BzPTSP kMwskgZvFzdT/EWd7asEggKwLfEmAEbqwJz4eopSEmrkR3UgsIy7I+ZVsUUph5D4kbM/ h0ZsDN/IiNsR4x3E5EQJTNJECipFLm1QRPBdPjL4BwUW2CtTzbHoCvn2jCb9h6uwEedg S8Qg== X-Gm-Message-State: AOAM530SnNEyy3qHnCfOhVMnJcfo2cxCg18qNyglJlAUM0Ty/LNZs3QX gIoqZ0Sxanse7Of+hbmXbUBG/0Sa+HY= X-Google-Smtp-Source: ABdhPJwCyb/a1zKD1ZrJJen8WzeT7UrJ+c1MfDiEGo3x4V9yTV7M/dpDVwAo3ziFK/l4d+0xazCDVg== X-Received: by 2002:a5d:4d04:: with SMTP id z4mr33862384wrt.133.1625743158146; Thu, 08 Jul 2021 04:19:18 -0700 (PDT) Received: from abel.fritz.box ([2a02:908:1252:fb60:88a0:1bd:c5e1:444e]) by smtp.gmail.com with ESMTPSA id p3sm9123236wmq.17.2021.07.08.04.19.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jul 2021 04:19:17 -0700 (PDT) From: "=?UTF-8?q?Christian=20K=C3=B6nig?=" X-Google-Original-From: =?UTF-8?q?Christian=20K=C3=B6nig?= To: dri-devel@lists.freedesktop.org, roberto.sassu@huawei.com Subject: [PATCH] dma-buf: fix and rework dma_buf_poll v5 Date: Thu, 8 Jul 2021 13:19:16 +0200 Message-Id: <20210708111916.7291-1-christian.koenig@amd.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Daniel pointed me towards this function and there are multiple obvious problems in the implementation. First of all the retry loop is not working as intended. In general the retry makes only sense if you grab the reference first and then check the sequence values. Then we should always also wait for the exclusive fence. It's also good practice to keep the reference around when installing callbacks to fences you don't own. And last the whole implementation was unnecessary complex and rather hard to understand which could lead to probably unexpected behavior of the IOCTL. Fix all this by reworking the implementation from scratch. Dropping the whole RCU approach and taking the lock instead. Only mildly tested and needs a thoughtful review of the code. v2: fix the reference counting as well v3: keep the excl fence handling as is for stable v4: back to testing all fences, drop RCU v5: handle in and out separately Signed-off-by: Christian König CC: stable@vger.kernel.org --- drivers/dma-buf/dma-buf.c | 152 +++++++++++++++++--------------------- include/linux/dma-buf.h | 2 +- 2 files changed, 68 insertions(+), 86 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index eadd1eaa2fb5..439e2379e1cb 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry) * If you hit this BUG() it means someone dropped their ref to the * dma-buf while still having pending operation to the buffer. */ - BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); + BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); dmabuf->ops->release(dmabuf); @@ -202,16 +202,57 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) wake_up_locked_poll(dcb->poll, dcb->active); dcb->active = 0; spin_unlock_irqrestore(&dcb->poll->lock, flags); + dma_fence_put(fence); +} + +static bool dma_buf_poll_shared(struct dma_resv *resv, + struct dma_buf_poll_cb_t *dcb) +{ + struct dma_resv_list *fobj = dma_resv_get_list(resv); + struct dma_fence *fence; + int i, r; + + if (!fobj) + return false; + + for (i = 0; i < fobj->shared_count; ++i) { + fence = rcu_dereference_protected(fobj->shared[i], + dma_resv_held(resv)); + dma_fence_get(fence); + r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); + if (!r) + return true; + dma_fence_put(fence); + } + + return false; +} + +static bool dma_buf_poll_excl(struct dma_resv *resv, + struct dma_buf_poll_cb_t *dcb) +{ + struct dma_fence *fence = dma_resv_get_excl(resv); + int r; + + if (!fence) + return false; + + dma_fence_get(fence); + r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb); + if (!r) + return true; + dma_fence_put(fence); + + return false; } static __poll_t dma_buf_poll(struct file *file, poll_table *poll) { struct dma_buf *dmabuf; struct dma_resv *resv; - struct dma_resv_list *fobj; - struct dma_fence *fence_excl; + unsigned shared_count; __poll_t events; - unsigned shared_count, seq; + int r, i; dmabuf = file->private_data; if (!dmabuf || !dmabuf->resv) @@ -225,101 +266,42 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) if (!events) return 0; -retry: - seq = read_seqcount_begin(&resv->seq); - rcu_read_lock(); - - fobj = rcu_dereference(resv->fence); - if (fobj) - shared_count = fobj->shared_count; - else - shared_count = 0; - fence_excl = rcu_dereference(resv->fence_excl); - if (read_seqcount_retry(&resv->seq, seq)) { - rcu_read_unlock(); - goto retry; - } - - if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) { - struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl; - __poll_t pevents = EPOLLIN; + dma_resv_lock(resv, NULL); - if (shared_count == 0) - pevents |= EPOLLOUT; + if (events & EPOLLOUT) { + struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_out; + /* Check that callback isn't busy */ spin_lock_irq(&dmabuf->poll.lock); - if (dcb->active) { - dcb->active |= pevents; - events &= ~pevents; - } else - dcb->active = pevents; + if (dcb->active) + events &= ~EPOLLOUT; + else + dcb->active = EPOLLOUT; spin_unlock_irq(&dmabuf->poll.lock); - if (events & pevents) { - if (!dma_fence_get_rcu(fence_excl)) { - /* force a recheck */ - events &= ~pevents; - dma_buf_poll_cb(NULL, &dcb->cb); - } else if (!dma_fence_add_callback(fence_excl, &dcb->cb, - dma_buf_poll_cb)) { - events &= ~pevents; - dma_fence_put(fence_excl); - } else { - /* - * No callback queued, wake up any additional - * waiters. - */ - dma_fence_put(fence_excl); - dma_buf_poll_cb(NULL, &dcb->cb); - } - } + if (events & EPOLLOUT && !dma_buf_poll_shared(resv, dcb) && + !dma_buf_poll_excl(resv, dcb)) + /* No callback queued, wake up any other waiters */ + dma_buf_poll_cb(NULL, &dcb->cb); } - if ((events & EPOLLOUT) && shared_count > 0) { - struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared; - int i; + if (events & EPOLLIN) { + struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_in; - /* Only queue a new callback if no event has fired yet */ + /* Check that callback isn't busy */ spin_lock_irq(&dmabuf->poll.lock); if (dcb->active) - events &= ~EPOLLOUT; + events &= ~EPOLLIN; else - dcb->active = EPOLLOUT; + dcb->active = EPOLLIN; spin_unlock_irq(&dmabuf->poll.lock); - if (!(events & EPOLLOUT)) - goto out; - - for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence = rcu_dereference(fobj->shared[i]); - - if (!dma_fence_get_rcu(fence)) { - /* - * fence refcount dropped to zero, this means - * that fobj has been freed - * - * call dma_buf_poll_cb and force a recheck! - */ - events &= ~EPOLLOUT; - dma_buf_poll_cb(NULL, &dcb->cb); - break; - } - if (!dma_fence_add_callback(fence, &dcb->cb, - dma_buf_poll_cb)) { - dma_fence_put(fence); - events &= ~EPOLLOUT; - break; - } - dma_fence_put(fence); - } - - /* No callback queued, wake up any additional waiters. */ - if (i == shared_count) + if (events & EPOLLIN && !dma_buf_poll_excl(resv, dcb)) + /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); } -out: - rcu_read_unlock(); + dma_resv_unlock(resv); return events; } @@ -562,8 +544,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) dmabuf->owner = exp_info->owner; spin_lock_init(&dmabuf->name_lock); init_waitqueue_head(&dmabuf->poll); - dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll; - dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0; + dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll; + dmabuf->cb_in.active = dmabuf->cb_out.active = 0; if (!resv) { resv = (struct dma_resv *)&dmabuf[1]; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index efdc56b9d95f..7e747ad54c81 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -329,7 +329,7 @@ struct dma_buf { wait_queue_head_t *poll; __poll_t active; - } cb_excl, cb_shared; + } cb_in, cb_out; }; /** -- 2.25.1