Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

[Jakob <jakobkoschel@gmail.com>]

> On 23. Feb 2022, at 21:22, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@gmail.com> wrote:
>> 
>> 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?
> 
> No.
> 
> Make the rule be "you never use the iterator outside the loop".
> 
> IOW, the code sequence is
> 
>        some_struct *ptr, *iter;

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

> 
>        ptr = NULL;
>        list_for_each_entry(iter, ...) {
>                if (iter_matches_condition(iter)) {
>                        ptr = iter;
>                        break;
>                }
>        }
> 
>        .. never use 'iter' here - you use 'ptr' and check it for NULL ..
> 
> 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 = NULL;
       list_for_each_entry(iter, head, list) {
               if (iter_matches_condition(iter)) {
                       ptr = iter;
                       break;
               }
       }
       

       if (ptr)
               list_ptr = head->prev;
       else
               list_ptr = 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 = 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.
> 
> And as mentioned, it actually tends to lead to better code, since the
> code outside the loop only has one variable live, not two.
> 
> 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'.
> 
> 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.
> 
>                  Linus

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

	Jakob