Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.60.0.1.1\))
Subject: Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list
 iterator after the loop
From:   Jakob <jakobkoschel@gmail.com>
In-Reply-To: <CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com>
Date:   Wed, 23 Feb 2022 23:08:53 +0100
Cc:     Jason Gunthorpe <jgg@ziepe.ca>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arch <linux-arch@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Arnd Bergman <arnd@arndb.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Mike Rapoport <rppt@kernel.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BA4ACA21-2368-4BEB-ACF0-F4C2042880F4@gmail.com>
References: <20220217184829.1991035-1-jakobkoschel@gmail.com>
 <20220217184829.1991035-5-jakobkoschel@gmail.com>
 <20220218151216.GE1037534@ziepe.ca>
 <6BA40980-554F-45E2-914D-5E4CD02FF21C@gmail.com>
 <CAHk-=wir=xabJ73Upk1dsuoMKWTTjTfeLFJ=p2S0yRYYaxW4fA@mail.gmail.com>
 <20220223191222.GC10361@ziepe.ca>
 <CAHk-=widDQUjQS2tpaw3j_+Yz8rAY3P0qdqpz+nTNu4-3LaU3w@mail.gmail.com>
 <F9A66F89-66C2-4322-808B-275384C4CC9D@gmail.com>
 <CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Precedence: bulk


> On 23. Feb 2022, at 21:22, Linus Torvalds =
<torvalds@linux-foundation.org> wrote:
>=20
> On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@gmail.com> wrote:
>>=20
>> in such a case you would still have to set the iterator value to
>> NULL when reaching the terminating condition or am I missing =
something?
>=20
> No.
>=20
> Make the rule be "you never use the iterator outside the loop".
>=20
> IOW, the code sequence is
>=20
>        some_struct *ptr, *iter;

with C99 iter would be defined within the loop instead right?

>=20
>        ptr =3D NULL;
>        list_for_each_entry(iter, ...) {
>                if (iter_matches_condition(iter)) {
>                        ptr =3D iter;
>                        break;
>                }
>        }
>=20
>        .. never use 'iter' here - you use 'ptr' and check it for NULL =
..
>=20
> See? Same number of variables as using a separate 'bool found' flag,
> but simpler code, and it matches the rule of 'don't use iter outside
> the loop'.

ah yes this does make sense. I missed the part of using a separate
'ptr' variable. Thanks for clarifying.
I think this is a great idea.

There are cases where pos->member is used (the only legitimate way to
use it right now). I suppose those turn into something like this
(this example is inspired by dev_add_offload() (net/core/gro.c:38)):

       some_struct *ptr, *iter;
       list_head *list_ptr;

       ptr =3D NULL;
       list_for_each_entry(iter, head, list) {
               if (iter_matches_condition(iter)) {
                       ptr =3D iter;
                       break;
               }
       }
      =20

       if (ptr)
               list_ptr =3D head->prev;
       else
               list_ptr =3D iter->list.prev;
       list_add(..., list_ptr);

before it was simply
       list_add(..., iter->list.prev);


The other possibility I suppose would be:

       if (!ptr)
               ptr =3D container_of(head, typeof(*ptr), list)
       list_add(..., ptr->list.prev);

which leaves you with the same type confusion as before, being far from
ideal.

> This is how you'd have to do it anyway if we start using a C99 style
> 'declare iter _in_ the loop' model.
>=20
> And as mentioned, it actually tends to lead to better code, since the
> code outside the loop only has one variable live, not two.
>=20
> Of course, compilers can do a lot of optimizations, so a 'found'
> variable can be made to generate good code too - if the compiler just
> tracks it and notices, and turns the 'break' into a 'goto found', and
> the fallthrough into the 'goto not_found'.
>=20
> So 'better code generation' is debatable, but even if the compiler can
> do as good a job with a separate 'bool' variable and some cleverness,
> I think we should strive for code where we make it easy for the
> compiler to DTRT - and where the generated code is easier to match up
> with what we wrote.
>=20
>                  Linus

If there is interest, I'm happy to send a new patch set once the fixes =
are clear.

	Jakob