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=-16.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HK_RANDOM_FROM,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 4A4F5C433F5 for ; Tue, 14 Sep 2021 10:53:43 +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 0DED4610FB for ; Tue, 14 Sep 2021 10:53:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0DED4610FB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 036406E48C; Tue, 14 Sep 2021 10:53:42 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 16B436E49C; Tue, 14 Sep 2021 10:53:41 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10106"; a="282959597" X-IronPort-AV: E=Sophos;i="5.85,292,1624345200"; d="scan'208";a="282959597" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2021 03:53:40 -0700 X-IronPort-AV: E=Sophos;i="5.85,292,1624345200"; d="scan'208";a="432952180" Received: from ocascan-mobl.ger.corp.intel.com (HELO [10.213.234.116]) ([10.213.234.116]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2021 03:53:38 -0700 To: =?UTF-8?Q?Christian_K=c3=b6nig?= , linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org Cc: daniel@ffwll.ch, intel-gfx@lists.freedesktop.org References: <20210913131707.45639-1-christian.koenig@amd.com> <20210913131707.45639-2-christian.koenig@amd.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: <1eee4105-e154-9d1d-b92b-d17c6f8f8432@linux.intel.com> Date: Tue, 14 Sep 2021 11:53:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210913131707.45639-2-christian.koenig@amd.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 13/09/2021 14:16, Christian König wrote: > Abstract the complexity of iterating over all the fences > in a dma_resv object. > > The new loop handles the whole RCU and retry dance and > returns only fences where we can be sure we grabbed the > right one. > > Signed-off-by: Christian König > --- > drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ > include/linux/dma-resv.h | 36 ++++++++++++++++++++++ > 2 files changed, 99 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 84fbe60629e3..213a9b7251ca 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) > } > EXPORT_SYMBOL(dma_resv_add_excl_fence); > > +/** > + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj > + * @obj: the dma_resv object > + * @cursor: cursor to record the current position > + * @all_fences: true returns also the shared fences > + * @first: if we should start over > + * > + * Return all the fences in the dma_resv object which are not yet signaled. > + * The returned fence has an extra local reference so will stay alive. > + * If a concurrent modify is detected the whole iterator is started over again. > + */ > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_cursor *cursor, > + bool all_fences, bool first) > +{ > + struct dma_fence *fence = NULL; > + > + do { > + /* Drop the reference from the previous round */ > + dma_fence_put(fence); > + > + cursor->is_first = first; > + if (first) { > + cursor->seq = read_seqcount_begin(&obj->seq); > + cursor->index = -1; > + cursor->fences = dma_resv_shared_list(obj); > + cursor->is_exclusive = true; > + > + fence = dma_resv_excl_fence(obj); > + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + fence = NULL; > + } else { > + fence = NULL; > + } > + > + if (fence) { > + fence = dma_fence_get_rcu(fence); > + } else if (all_fences && cursor->fences) { > + struct dma_resv_list *fences = cursor->fences; If rcu lock is allowed to be dropped while walking the list what guarantees list of fences hasn't been freed? Like: 1st call -> gets seqcount -> stores cursor->fences rcu lock dropped/re-acquired 2nd call -> dereferences into cursor->fences -> boom? > + > + cursor->is_exclusive = false; > + while (++cursor->index < fences->shared_count) { > + fence = rcu_dereference(fences->shared[ > + cursor->index]); > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + break; > + } > + if (cursor->index < fences->shared_count) > + fence = dma_fence_get_rcu(fence); > + else > + fence = NULL; > + } > + > + /* For the eventually next round */ > + first = true; > + } while (read_seqcount_retry(&obj->seq, cursor->seq)); > + > + return fence; > +} > +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); > + > /** > * dma_resv_copy_fences - Copy all fences from src to dst. > * @dst: the destination reservation object > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index 9100dd3dc21f..f5b91c292ee0 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -149,6 +149,39 @@ struct dma_resv { > struct dma_resv_list __rcu *fence; > }; > > +/** > + * struct dma_resv_cursor - current position into the dma_resv fences > + * @seq: sequence number to check > + * @index: index into the shared fences > + * @shared: the shared fences > + * @is_first: true if this is the first returned fence > + * @is_exclusive: if the current fence is the exclusive one > + */ > +struct dma_resv_cursor { > + unsigned int seq; > + unsigned int index; > + struct dma_resv_list *fences; > + bool is_first; Is_first is useful to callers - like they are legitimately allowed to look inside this, what could otherwise be private object? What is the intended use case, given when true the returned fence can be either exclusive or first from a shared list? > + bool is_exclusive; Is_exclusive could be written as index == -1 in the code, right? If so then an opportunity to remove some redundancy. > +}; > + > +/** > + * dma_resv_for_each_fence_unlocked - fence iterator > + * @obj: a dma_resv object pointer > + * @cursor: a struct dma_resv_cursor pointer > + * @all_fences: true if all fences should be returned > + * @fence: the current fence > + * > + * Iterate over the fences in a struct dma_resv object without holding the > + * dma_resv::lock. The RCU read side lock must be hold when using this, but can > + * be dropped and re-taken as necessary inside the loop. @all_fences controls > + * if the shared fences are returned as well. > + */ > +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ > + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ > + fence; dma_fence_put(fence), \ > + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) Has the fact RCU lock can be dropped so there is potential to walk over completely different snapshots been discussed? At least if I followed the code correctly - it appears there is potential the walk restarts from the start (exclusive slot) at any point during the walk. Because theoretically I think you could take an atomic snapshot of everything (given you have a cursor object) and then release it on the end condition. Regards, Tvrtko > + > #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) > #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) > > @@ -366,6 +399,9 @@ void dma_resv_fini(struct dma_resv *obj); > int dma_resv_reserve_shared(struct dma_resv *obj, unsigned int num_fences); > void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence); > void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_cursor *cursor, > + bool first, bool all_fences); > int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, > unsigned *pshared_count, struct dma_fence ***pshared); > int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); >