From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Jakob Koschel <jakobkoschel@gmail.com>,
Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-wireless@vger.kernel.org, alsa-devel@alsa-project.org,
kvm@vger.kernel.org,
"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
linux-iio@vger.kernel.org, nouveau@lists.freedesktop.org,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
dri-devel@lists.freedesktop.org,
Cristiano Giuffrida <c.giuffrida@vu.nl>,
amd-gfx@lists.freedesktop.org,
linux1394-devel@lists.sourceforge.net, drbd-dev@lists.linbit.com,
linux-arch <linux-arch@vger.kernel.org>,
linux-cifs@vger.kernel.org, linux-aspeed@lists.ozlabs.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-staging@lists.linux.dev, "Bos, H.J." <h.j.bos@vu.nl>,
Jason Gunthorpe <jgg@ziepe.ca>,
intel-wired-lan@lists.osuosl.org,
kgdb-bugreport@lists.sourceforge.net,
bcm-kernel-feedback-list@broadcom.com,
Dan Carpenter <dan.carpenter@oracle.com>,
linux-media@vger.kernel.org, Kees Cook <keescook@chromium.org>,
Arnd Bergman <arnd@arndb.de>,
linux-pm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
Nathan Chancellor <nathan@kernel.org>,
dmaengine@vger.kernel.org,
Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
v9fs-developer@lists.sourceforge.net,
linux-tegra@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-arm-kernel@lists.infradead.org, linux-sgx@vger.kernel.org,
linux-block@vger.kernel.org, netdev@vger.kernel.org,
linux-usb@vger.kernel.org, samba-technical@lists.samba.org,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
tipc-discussion@lists.sourceforge.net,
linux-crypto@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-mediatek@lists.infradead.org,
Andrew Morton <akpm@linux-foundation.org>,
linuxppc-dev@lists.ozlabs.org, Mike Rapoport <rppt@kernel.org>
Subject: Re: [Intel-gfx] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
Date: Wed, 2 Mar 2022 17:14:50 +0000 [thread overview]
Message-ID: <ed52ce3c-0f4a-a1e8-4176-543657d6228d@linux.intel.com> (raw)
In-Reply-To: <20220228110822.491923-7-jakobkoschel@gmail.com>
On 28/02/2022 11:08, Jakob Koschel wrote:
> When list_for_each_entry() completes the iteration over the whole list
> without breaking the loop, the iterator value will be a bogus pointer
> computed based on the head element.
>
> While it is safe to use the pointer to determine if it was computed
> based on the head element, either with list_entry_is_head() or
> &pos->member == head, using the iterator variable after the loop should
> be avoided.
>
> In preparation to limiting the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element.
>
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
[snip until i915 parts]
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++---
> drivers/gpu/drm/i915/gt/intel_ring.c | 15 ++++---
[snip]
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 00327b750fbb..80c79028901a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -107,25 +107,27 @@ static void lut_close(struct i915_gem_context *ctx)
> radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
> struct i915_vma *vma = rcu_dereference_raw(*slot);
> struct drm_i915_gem_object *obj = vma->obj;
> - struct i915_lut_handle *lut;
> + struct i915_lut_handle *lut = NULL;
> + struct i915_lut_handle *tmp;
>
> if (!kref_get_unless_zero(&obj->base.refcount))
> continue;
>
> spin_lock(&obj->lut_lock);
> - list_for_each_entry(lut, &obj->lut_list, obj_link) {
> - if (lut->ctx != ctx)
> + list_for_each_entry(tmp, &obj->lut_list, obj_link) {
> + if (tmp->ctx != ctx)
> continue;
>
> - if (lut->handle != iter.index)
> + if (tmp->handle != iter.index)
> continue;
>
> - list_del(&lut->obj_link);
> + list_del(&tmp->obj_link);
> + lut = tmp;
> break;
> }
> spin_unlock(&obj->lut_lock);
>
> - if (&lut->obj_link != &obj->lut_list) {
> + if (lut) {
> i915_lut_handle_free(lut);
> radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
Looks okay although personally I would have left lut as is for a smaller
diff and introduced a new local like 'found' or 'unlinked'.
> i915_vma_close(vma);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 1736efa43339..fda9e3685ad2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2444,7 +2444,8 @@ static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct intel
> {
> struct intel_ring *ring = ce->ring;
> struct intel_timeline *tl = ce->timeline;
> - struct i915_request *rq;
> + struct i915_request *rq = NULL;
> + struct i915_request *tmp;
>
> /*
> * Completely unscientific finger-in-the-air estimates for suitable
> @@ -2460,15 +2461,17 @@ static struct i915_request *eb_throttle(struct i915_execbuffer *eb, struct intel
> * claiming our resources, but not so long that the ring completely
> * drains before we can submit our next request.
> */
> - list_for_each_entry(rq, &tl->requests, link) {
> - if (rq->ring != ring)
> + list_for_each_entry(tmp, &tl->requests, link) {
> + if (tmp->ring != ring)
> continue;
>
> - if (__intel_ring_space(rq->postfix,
> - ring->emit, ring->size) > ring->size / 2)
> + if (__intel_ring_space(tmp->postfix,
> + ring->emit, ring->size) > ring->size / 2) {
> + rq = tmp;
> break;
> + }
> }
> - if (&rq->link == &tl->requests)
> + if (!rq)
> return NULL; /* weird, we will check again later for real */
Alternatively, instead of break could simply do "return
i915_request_get(rq);" and replace the end of the function after the
loop with "return NULL;". A bit smaller diff, or at least less "spread
out" over the function, so might be easier to backport stuff touching
this area in the future. But looks correct as is.
>
> return i915_request_get(rq);
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c
> index 2fdd52b62092..4881c4e0c407 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c
> @@ -191,24 +191,27 @@ wait_for_space(struct intel_ring *ring,
> struct intel_timeline *tl,
> unsigned int bytes)
> {
> - struct i915_request *target;
> + struct i915_request *target = NULL;
> + struct i915_request *tmp;
> long timeout;
>
> if (intel_ring_update_space(ring) >= bytes)
> return 0;
>
> GEM_BUG_ON(list_empty(&tl->requests));
> - list_for_each_entry(target, &tl->requests, link) {
> - if (target->ring != ring)
> + list_for_each_entry(tmp, &tl->requests, link) {
> + if (tmp->ring != ring)
> continue;
>
> /* Would completion of this request free enough space? */
> - if (bytes <= __intel_ring_space(target->postfix,
> - ring->emit, ring->size))
> + if (bytes <= __intel_ring_space(tmp->postfix,
> + ring->emit, ring->size)) {
> + target = tmp;
> break;
> + }
> }
>
> - if (GEM_WARN_ON(&target->link == &tl->requests))
> + if (GEM_WARN_ON(!target))
> return -ENOSPC;
>
> timeout = i915_request_wait(target,
Looks okay as well. Less clear here if there is a clean solution to make
the diff smaller so no suggestions. I mean do I dare mention "goto
found;" from inside the loop, where the break is, instead of the
variable renames.. risky.. :) (And ofc "return -ENOSPC" immediately
after the loop.)
As a summary changes looks okay, up to you if you want to try to make
the diffs smaller or not. It doesn't matter hugely really, all I have is
a vague and uncertain "maybe it makes backporting of something, someday
easier". So for i915 it is good either way.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> # i915 bits only
Regards,
Tvrtko
next prev parent reply other threads:[~2022-03-02 22:59 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-28 11:08 [PATCH 0/6] Remove usage of list iterator past the loop body Jakob Koschel
2022-02-28 11:08 ` [PATCH 1/6] drivers: usb: remove " Jakob Koschel
2022-02-28 11:24 ` Dan Carpenter
2022-02-28 12:03 ` Jakob Koschel
2022-02-28 13:18 ` Dan Carpenter
2022-02-28 18:20 ` Joe Perches
2022-03-01 5:52 ` Dan Carpenter
2022-02-28 11:08 ` [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr Jakob Koschel
2022-02-28 11:20 ` Greg KH
2022-02-28 12:06 ` Jakob Koschel
2022-03-01 17:37 ` Greg KH
2022-02-28 12:19 ` Christian König
2022-02-28 19:56 ` Linus Torvalds
2022-02-28 20:03 ` Linus Torvalds
2022-02-28 20:10 ` Linus Torvalds
2022-02-28 20:14 ` Linus Torvalds
2022-02-28 20:53 ` Segher Boessenkool
2022-02-28 20:16 ` Matthew Wilcox
2022-02-28 20:27 ` Johannes Berg
2022-02-28 20:41 ` Linus Torvalds
2022-02-28 20:37 ` Linus Torvalds
2022-02-28 23:26 ` Matthew Wilcox
2022-03-01 0:45 ` Linus Torvalds
2022-03-01 0:57 ` Linus Torvalds
2022-03-01 18:14 ` Kees Cook
2022-03-01 18:47 ` Linus Torvalds
2022-03-01 19:01 ` Matthew Wilcox
2022-03-01 3:03 ` David Laight
2022-02-28 21:47 ` Jakob Koschel
2022-03-01 0:41 ` Linus Torvalds
2022-03-01 6:32 ` Jakub Kicinski
2022-03-01 11:28 ` Jakob Koschel
2022-03-01 17:36 ` Greg KH
2022-03-01 17:40 ` Jakob Koschel
2022-03-01 17:58 ` Greg KH
2022-03-01 18:21 ` Kees Cook
2022-03-02 9:31 ` Xiaomeng Tong
2022-03-02 14:04 ` David Laight
2022-03-03 2:27 ` Xiaomeng Tong
2022-03-03 4:58 ` David Laight
2022-03-03 7:26 ` Xiaomeng Tong
2022-03-03 9:30 ` David Laight
2022-03-03 12:37 ` Xiaomeng Tong
2022-03-03 12:18 ` [Kgdb-bugreport] " Daniel Thompson
2022-03-04 6:59 ` Xiaomeng Tong
2022-03-03 7:32 ` Jakob Koschel
2022-03-03 8:30 ` Xiaomeng Tong
2022-03-03 8:38 ` Xiaomeng Tong
2022-02-28 20:07 ` Christian König
2022-02-28 20:42 ` James Bottomley
2022-02-28 20:56 ` Christian König
2022-02-28 21:13 ` James Bottomley
2022-03-01 7:03 ` Christian König
2022-02-28 22:05 ` Jakob Koschel
2022-02-28 21:18 ` Jeffrey Walton
2022-02-28 21:59 ` Mike Rapoport
2022-02-28 22:28 ` James Bottomley
2022-02-28 22:50 ` Barnabás Pőcze
2022-03-01 0:30 ` Segher Boessenkool
2022-03-01 0:54 ` Linus Torvalds
2022-03-01 19:06 ` Linus Torvalds
2022-03-01 19:42 ` Linus Torvalds
2022-03-01 22:58 ` David Laight
2022-03-01 23:03 ` Linus Torvalds
2022-03-01 23:19 ` David Laight
2022-03-01 23:55 ` Linus Torvalds
2022-03-02 9:29 ` Rasmus Villemoes
2022-03-02 20:07 ` Kees Cook
2022-03-02 20:18 ` Linus Torvalds
2022-03-02 20:59 ` Kees Cook
2022-03-03 8:37 ` Dan Carpenter
2022-03-03 10:56 ` Dan Carpenter
2022-03-01 2:15 ` David Laight
2022-02-28 13:13 ` Dan Carpenter
2022-02-28 11:08 ` [PATCH 3/6] treewide: fix incorrect use to determine if list is empty Jakob Koschel
2022-02-28 11:38 ` Dan Carpenter
2022-02-28 11:08 ` [PATCH 4/6] drivers: remove unnecessary use of list iterator variable Jakob Koschel
2022-02-28 11:08 ` [PATCH 5/6] treewide: remove dereference of list iterator after loop body Jakob Koschel
2022-02-28 11:08 ` [PATCH 6/6] treewide: remove check of list iterator against head past the " Jakob Koschel
2022-02-28 11:22 ` Dominique Martinet
2022-02-28 13:12 ` Dan Carpenter
2022-03-01 20:36 ` Linus Torvalds
2022-03-02 17:14 ` Tvrtko Ursulin [this message]
2022-03-07 15:00 ` [PATCH 0/6] Remove usage of list iterator " Dan Carpenter
2022-03-07 15:26 ` David Laight
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=ed52ce3c-0f4a-a1e8-4176-543657d6228d@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=alsa-devel@alsa-project.org \
--cc=amd-gfx@lists.freedesktop.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bjohannesmeyer@gmail.com \
--cc=c.giuffrida@vu.nl \
--cc=christophe.jaillet@wanadoo.fr \
--cc=dan.carpenter@oracle.com \
--cc=dmaengine@vger.kernel.org \
--cc=drbd-dev@lists.linbit.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gustavo@embeddedor.com \
--cc=h.j.bos@vu.nl \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jakobkoschel@gmail.com \
--cc=jgg@ziepe.ca \
--cc=keescook@chromium.org \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=kvm@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-tegra@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linux1394-devel@lists.sourceforge.net \
--cc=linux@rasmusvillemoes.dk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=nathan@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=rppt@kernel.org \
--cc=samba-technical@lists.samba.org \
--cc=tglx@linutronix.de \
--cc=tipc-discussion@lists.sourceforge.net \
--cc=torvalds@linux-foundation.org \
--cc=v9fs-developer@lists.sourceforge.net \
/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).