[PATCH][RFC] kernel.h: provide array iterator

[PATCH][RFC] kernel.h: provide array iterator

[Kieran Bingham]

Simplify array iteration with a helper to iterate each entry in an array.
Utilise the existing ARRAY_SIZE macro to identify the length of the array
and pointer arithmetic to process each item as a for loop.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/linux/kernel.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

The use of static arrays to store data is a common use case throughout the
kernel. Along with that is the obvious need to iterate that data.

In fact there are just shy of 5000 instances of iterating a static array:
	git grep "for .*ARRAY_SIZE" | wc -l
	4943

When working on the UVC driver - I found that I needed to split one such
iteration into two parts, and at the same time felt that this could be
refactored to be cleaner / easier to read. 

I do however worry that this simple short patch might not be desired or could
also be heavily bikeshedded due to it's potential wide spread use (though
perhaps that would be a good thing to have more users) ...  but here it is,
along with an example usage below which is part of a separate series.

The aim is to simplify iteration on static arrays, in the same way that we have
iterators for lists. The use of the ARRAY_SIZE macro, provides all the
protections given by "__must_be_array(arr)" to this macro too.

Regards

Kieran

=============================================================================
Example Usage from a pending UVC development:

+#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
+       for_each_array_element(uvc_urb, uvc_streaming->uvc_urb)
 
 /*
  * Uninitialize isochronous/bulk URBs and free transfer buffers.
  */
 static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
 {
-       struct urb *urb;
-       unsigned int i;
+       struct uvc_urb *uvc_urb;

        uvc_video_stats_stop(stream);

-       for (i = 0; i < UVC_URBS; ++i) {
-               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
+       for_each_uvc_urb(uvc_urb, stream)
+               usb_kill_urb(uvc_urb->urb);

-               urb = uvc_urb->urb;
-               if (urb == NULL)
-                       continue;
+       flush_workqueue(stream->async_wq);

-               usb_kill_urb(urb);
-               usb_free_urb(urb);
+       for_each_uvc_urb(uvc_urb, stream) {
+               usb_free_urb(uvc_urb->urb);
                uvc_urb->urb = NULL;
        }

        if (free_buffers)
                uvc_free_urb_buffers(stream);
 }
=============================================================================




diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ce51455e2adf..95d7dae248b7 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -70,6 +70,16 @@
  */
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
+/**
+ * for_each_array_element - Iterate all items in an array
+ * @elem: pointer of array type for iteration cursor
+ * @array: array to be iterated
+ */
+#define for_each_array_element(elem, array) \
+	for (elem = &(array)[0]; \
+	     elem < &(array)[ARRAY_SIZE(array)]; \
+	     ++elem)
+
 #define u64_to_user_ptr(x) (		\
 {					\
 	typecheck(u64, x);		\
-- 
2.7.4

Re: [PATCH][RFC] kernel.h: provide array iterator

[Kees Cook]

On Thu, Mar 15, 2018 at 3:00 AM, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> Simplify array iteration with a helper to iterate each entry in an array.
> Utilise the existing ARRAY_SIZE macro to identify the length of the array
> and pointer arithmetic to process each item as a for loop.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/linux/kernel.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> The use of static arrays to store data is a common use case throughout the
> kernel. Along with that is the obvious need to iterate that data.
>
> In fact there are just shy of 5000 instances of iterating a static array:
>         git grep "for .*ARRAY_SIZE" | wc -l
>         4943

I suspect the main question is "Does this macro make the code easier to read?"

I think it does, and we have other kinds of iterators like this in the
kernel already. Would it be worth building a Coccinelle script to do
the 5000 replacements?

-Kees

>
> When working on the UVC driver - I found that I needed to split one such
> iteration into two parts, and at the same time felt that this could be
> refactored to be cleaner / easier to read.
>
> I do however worry that this simple short patch might not be desired or could
> also be heavily bikeshedded due to it's potential wide spread use (though
> perhaps that would be a good thing to have more users) ...  but here it is,
> along with an example usage below which is part of a separate series.
>
> The aim is to simplify iteration on static arrays, in the same way that we have
> iterators for lists. The use of the ARRAY_SIZE macro, provides all the
> protections given by "__must_be_array(arr)" to this macro too.
>
> Regards
>
> Kieran
>
> =============================================================================
> Example Usage from a pending UVC development:
>
> +#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
> +       for_each_array_element(uvc_urb, uvc_streaming->uvc_urb)
>
>  /*
>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>   */
>  static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
>  {
> -       struct urb *urb;
> -       unsigned int i;
> +       struct uvc_urb *uvc_urb;
>
>         uvc_video_stats_stop(stream);
>
> -       for (i = 0; i < UVC_URBS; ++i) {
> -               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
> +       for_each_uvc_urb(uvc_urb, stream)
> +               usb_kill_urb(uvc_urb->urb);
>
> -               urb = uvc_urb->urb;
> -               if (urb == NULL)
> -                       continue;
> +       flush_workqueue(stream->async_wq);
>
> -               usb_kill_urb(urb);
> -               usb_free_urb(urb);
> +       for_each_uvc_urb(uvc_urb, stream) {
> +               usb_free_urb(uvc_urb->urb);
>                 uvc_urb->urb = NULL;
>         }
>
>         if (free_buffers)
>                 uvc_free_urb_buffers(stream);
>  }
> =============================================================================
>
>
>
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index ce51455e2adf..95d7dae248b7 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -70,6 +70,16 @@
>   */
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>
> +/**
> + * for_each_array_element - Iterate all items in an array
> + * @elem: pointer of array type for iteration cursor
> + * @array: array to be iterated
> + */
> +#define for_each_array_element(elem, array) \
> +       for (elem = &(array)[0]; \
> +            elem < &(array)[ARRAY_SIZE(array)]; \
> +            ++elem)
> +
>  #define u64_to_user_ptr(x) (           \
>  {                                      \
>         typecheck(u64, x);              \
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

Re: [PATCH][RFC] kernel.h: provide array iterator

[Julia Lawall]

Le 16.03.2018 05:21, Kees Cook a écrit :
> On Thu, Mar 15, 2018 at 3:00 AM, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>> Simplify array iteration with a helper to iterate each entry in an 
>> array.
>> Utilise the existing ARRAY_SIZE macro to identify the length of the 
>> array
>> and pointer arithmetic to process each item as a for loop.
>> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  include/linux/kernel.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> The use of static arrays to store data is a common use case throughout 
>> the
>> kernel. Along with that is the obvious need to iterate that data.
>> 
>> In fact there are just shy of 5000 instances of iterating a static 
>> array:
>>         git grep "for .*ARRAY_SIZE" | wc -l
>>         4943
> 
> I suspect the main question is "Does this macro make the code easier to 
> read?"
> 
> I think it does, and we have other kinds of iterators like this in the
> kernel already. Would it be worth building a Coccinelle script to do
> the 5000 replacements?


Coccinelle should be able to replace the for loop header.  Coccinelle
could create the local macro.  Coccinelle might not put the definition 
in
exactly the right place.  Before the function of the first use would be
possible, or before any function.

I don't think that Coccinelle could figure out how to split one loop 
into
two as done here, unless that specific pattern is very common.  I guess
that the split is to add the flush_workqueue, and is not the main goal?

julia




> -Kees
> 
>> 
>> When working on the UVC driver - I found that I needed to split one 
>> such
>> iteration into two parts, and at the same time felt that this could be
>> refactored to be cleaner / easier to read.
>> 
>> I do however worry that this simple short patch might not be desired 
>> or could
>> also be heavily bikeshedded due to it's potential wide spread use 
>> (though
>> perhaps that would be a good thing to have more users) ...  but here 
>> it is,
>> along with an example usage below which is part of a separate series.
>> 
>> The aim is to simplify iteration on static arrays, in the same way 
>> that we have
>> iterators for lists. The use of the ARRAY_SIZE macro, provides all the
>> protections given by "__must_be_array(arr)" to this macro too.
>> 
>> Regards
>> 
>> Kieran
>> 
>> =============================================================================
>> Example Usage from a pending UVC development:
>> 
>> +#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
>> +       for_each_array_element(uvc_urb, uvc_streaming->uvc_urb)
>> 
>>  /*
>>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>>   */
>>  static void uvc_uninit_video(struct uvc_streaming *stream, int 
>> free_buffers)
>>  {
>> -       struct urb *urb;
>> -       unsigned int i;
>> +       struct uvc_urb *uvc_urb;
>> 
>>         uvc_video_stats_stop(stream);
>> 
>> -       for (i = 0; i < UVC_URBS; ++i) {
>> -               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
>> +       for_each_uvc_urb(uvc_urb, stream)
>> +               usb_kill_urb(uvc_urb->urb);
>> 
>> -               urb = uvc_urb->urb;
>> -               if (urb == NULL)
>> -                       continue;
>> +       flush_workqueue(stream->async_wq);
>> 
>> -               usb_kill_urb(urb);
>> -               usb_free_urb(urb);
>> +       for_each_uvc_urb(uvc_urb, stream) {
>> +               usb_free_urb(uvc_urb->urb);
>>                 uvc_urb->urb = NULL;
>>         }
>> 
>>         if (free_buffers)
>>                 uvc_free_urb_buffers(stream);
>>  }
>> =============================================================================
>> 
>> 
>> 
>> 
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index ce51455e2adf..95d7dae248b7 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -70,6 +70,16 @@
>>   */
>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + 
>> __must_be_array(arr))
>> 
>> +/**
>> + * for_each_array_element - Iterate all items in an array
>> + * @elem: pointer of array type for iteration cursor
>> + * @array: array to be iterated
>> + */
>> +#define for_each_array_element(elem, array) \
>> +       for (elem = &(array)[0]; \
>> +            elem < &(array)[ARRAY_SIZE(array)]; \
>> +            ++elem)
>> +
>>  #define u64_to_user_ptr(x) (           \
>>  {                                      \
>>         typecheck(u64, x);              \
>> --
>> 2.7.4
>> 

Re: [PATCH][RFC] kernel.h: provide array iterator

[Kieran Bingham]

Hi Kees, Julia,

On 16/03/18 07:41, Julia Lawall wrote:
> Le 16.03.2018 05:21, Kees Cook a écrit :
>> On Thu, Mar 15, 2018 at 3:00 AM, Kieran Bingham
>> <kieran.bingham@ideasonboard.com> wrote:
>>> Simplify array iteration with a helper to iterate each entry in an array.
>>> Utilise the existing ARRAY_SIZE macro to identify the length of the array
>>> and pointer arithmetic to process each item as a for loop.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>  include/linux/kernel.h | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> The use of static arrays to store data is a common use case throughout the
>>> kernel. Along with that is the obvious need to iterate that data.
>>>
>>> In fact there are just shy of 5000 instances of iterating a static array:
>>>         git grep "for .*ARRAY_SIZE" | wc -l
>>>         4943
>>
>> I suspect the main question is "Does this macro make the code easier to read?"
>>
>> I think it does, and we have other kinds of iterators like this in the

Great :-D - I'm happy to read that response!


>> kernel already. Would it be worth building a Coccinelle script to do
>> the 5000 replacements?

Perhaps - though I suspect each case may have its nuances.

Looking around - I see some loops depend on their 'i' index variable in
different ways, so it might not always be convenient to remove that automagically.

Or perhaps I'd have to provide an alternative _indexed variant somehow which can
include the current offset... (or just include a scoped index in this iterator
some how)

Is there any policy or precedent on creating variables inside the scope of the
loop only through a macro like this?

(I'm sure I recall something in the back of my memory so I'll dig, but if anyone
has pointers...)



> Coccinelle should be able to replace the for loop header. 

I think that would be the main focus.
We would also need to add an iterator somewhere in the scope of the function...



> Coccinelle
> could create the local macro. 

I made the local macro for better readability of my use case ... Other places
could use the core macro directly - or also create a local macro.

That would be a per use case option I believe. But I guess it wouldn't be too
hard to form a macro using coccinelle with the name of the array or type that it
is iterating though.



> Coccinelle might not put the definition in
> exactly the right place.  Before the function of the first use would be
> possible, or before any function.

IMO - a macro to iterate the array specifically should come directly(ish) after
the declaration of the array where possible. (but not inside any struct if the
array is in there of course)



> I don't think that Coccinelle could figure out how to split one loop into
> two as done here, unless that specific pattern is very common.  I guess
> that the split is to add the flush_workqueue, and is not the main goal?

Yes, the split of this loop into two was very specific to this instance of
adding the flush_workqueue in the middle.

It was this process of splitting the loop in two which led me wanting to
optimise the iterators, rather than just duplicating it.


Regards

Kieran Bingham



> julia
> 
> 
> 
> 
>> -Kees
>>
>>>
>>> When working on the UVC driver - I found that I needed to split one such
>>> iteration into two parts, and at the same time felt that this could be
>>> refactored to be cleaner / easier to read.
>>>
>>> I do however worry that this simple short patch might not be desired or could
>>> also be heavily bikeshedded due to it's potential wide spread use (though
>>> perhaps that would be a good thing to have more users) ...  but here it is,
>>> along with an example usage below which is part of a separate series.
>>>
>>> The aim is to simplify iteration on static arrays, in the same way that we have
>>> iterators for lists. The use of the ARRAY_SIZE macro, provides all the
>>> protections given by "__must_be_array(arr)" to this macro too.
>>>
>>> Regards
>>>
>>> Kieran
>>>
>>> =============================================================================
>>> Example Usage from a pending UVC development:
>>>
>>> +#define for_each_uvc_urb(uvc_urb, uvc_streaming) \
>>> +       for_each_array_element(uvc_urb, uvc_streaming->uvc_urb)
>>>
>>>  /*
>>>   * Uninitialize isochronous/bulk URBs and free transfer buffers.
>>>   */
>>>  static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
>>>  {
>>> -       struct urb *urb;
>>> -       unsigned int i;
>>> +       struct uvc_urb *uvc_urb;
>>>
>>>         uvc_video_stats_stop(stream);
>>>
>>> -       for (i = 0; i < UVC_URBS; ++i) {
>>> -               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
>>> +       for_each_uvc_urb(uvc_urb, stream)
>>> +               usb_kill_urb(uvc_urb->urb);
>>>
>>> -               urb = uvc_urb->urb;
>>> -               if (urb == NULL)
>>> -                       continue;
>>> +       flush_workqueue(stream->async_wq);
>>>
>>> -               usb_kill_urb(urb);
>>> -               usb_free_urb(urb);
>>> +       for_each_uvc_urb(uvc_urb, stream) {
>>> +               usb_free_urb(uvc_urb->urb);
>>>                 uvc_urb->urb = NULL;
>>>         }
>>>
>>>         if (free_buffers)
>>>                 uvc_free_urb_buffers(stream);
>>>  }
>>> =============================================================================
>>>
>>>
>>>
>>>
>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>> index ce51455e2adf..95d7dae248b7 100644
>>> --- a/include/linux/kernel.h
>>> +++ b/include/linux/kernel.h
>>> @@ -70,6 +70,16 @@
>>>   */
>>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>>
>>> +/**
>>> + * for_each_array_element - Iterate all items in an array
>>> + * @elem: pointer of array type for iteration cursor
>>> + * @array: array to be iterated
>>> + */
>>> +#define for_each_array_element(elem, array) \
>>> +       for (elem = &(array)[0]; \
>>> +            elem < &(array)[ARRAY_SIZE(array)]; \
>>> +            ++elem)
>>> +
>>>  #define u64_to_user_ptr(x) (           \
>>>  {                                      \
>>>         typecheck(u64, x);              \
>>> -- 
>>> 2.7.4
>>>

Re: [PATCH][RFC] kernel.h: provide array iterator

[Rasmus Villemoes]

On 2018-03-15 11:00, Kieran Bingham wrote:
> Simplify array iteration with a helper to iterate each entry in an array.
> Utilise the existing ARRAY_SIZE macro to identify the length of the array
> and pointer arithmetic to process each item as a for loop.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/linux/kernel.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> The use of static arrays to store data is a common use case throughout the
> kernel. Along with that is the obvious need to iterate that data.
> 
> In fact there are just shy of 5000 instances of iterating a static array:
> 	git grep "for .*ARRAY_SIZE" | wc -l
> 	4943
> 
> When working on the UVC driver - I found that I needed to split one such
> iteration into two parts, and at the same time felt that this could be
> refactored to be cleaner / easier to read. 

About that, it would be helpful if you first converted to the new
iterator, so that one can more easily see they are equivalent. And then
split in two, adding the flush_workqueue call. Or do it the other way
around. But please don't mix the two in one patch, especially not if
it's supposed to act as an example of how to use the new helper.

> I do however worry that this simple short patch might not be desired or could
> also be heavily bikeshedded due to it's potential wide spread use (though
> perhaps that would be a good thing to have more users) ...  but here it is,
> along with an example usage below which is part of a separate series.

I think it can be useful, and it does have the must_be_array protection
built in, so code doesn't silently break if one changes from a
fixed-size allocation to e.g. a kmalloc-based one. Just don't attempt a
tree-wide mass conversion, but obviously starting to make use of it when
refactoring code anyway is fine.

And now, the bikeshedding you expected :)

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index ce51455e2adf..95d7dae248b7 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -70,6 +70,16 @@
>   */
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>  
> +/**
> + * for_each_array_element - Iterate all items in an array
> + * @elem: pointer of array type for iteration cursor

Hm, "pointer of array type" sounds wrong; it's not a "pointer to array".
But "pointer of array elements' type" is clumsy. Maybe just "@elem:
iteration cursor" is clear enough.

> + * @array: array to be iterated
> + */
> +#define for_each_array_element(elem, array) \
> +	for (elem = &(array)[0]; \
> +	     elem < &(array)[ARRAY_SIZE(array)]; \
> +	     ++elem)
> +

Please parenthesize elem as well.

Rasmus

Re: [PATCH][RFC] kernel.h: provide array iterator

[Joe Perches]

On Fri, 2018-03-16 at 16:27 +0100, Rasmus Villemoes wrote:
> On 2018-03-15 11:00, Kieran Bingham wrote:
> > Simplify array iteration with a helper to iterate each entry in an array.
> > Utilise the existing ARRAY_SIZE macro to identify the length of the array
> > and pointer arithmetic to process each item as a for loop.

I recall getting negative feedback on a similar proposal
a decade ago:

https://lkml.org/lkml/2007/2/13/25

Not sure this is different.

Re: [PATCH][RFC] kernel.h: provide array iterator

[Kieran Bingham]

Hi Joe,

On 16/03/18 19:45, Joe Perches wrote:
> On Fri, 2018-03-16 at 16:27 +0100, Rasmus Villemoes wrote:
>> On 2018-03-15 11:00, Kieran Bingham wrote:
>>> Simplify array iteration with a helper to iterate each entry in an array.
>>> Utilise the existing ARRAY_SIZE macro to identify the length of the array
>>> and pointer arithmetic to process each item as a for loop.
> 
> I recall getting negative feedback on a similar proposal
> a decade ago:
> 
> https://lkml.org/lkml/2007/2/13/25

Thanks for the reference. I didn't know about this.

Your suggestion at https://lkml.org/lkml/2007/2/13/25 looks remarkably similar
to my implementation though :-D  (Perhaps even a bit neater, I may have to
incorporate some of your suggestion)


> Not sure this is different.
> 

I count three disagreements in that series. But I'm sure I have more positive
responses already... (Though no 'official Acks' yet ...)

How many ACKs do I need for this to be accepted ? or do the past-nack's have
full veto?

I still believe the use of an iterator in my case [0] makes *absolute sense*
(thus it must make sense elsewhere)

I'm not suggesting a full tree conversion here (though that has been suggested
earlier in the thread) but the ability to add a convenience macro in a common
location, so that it can be used when desired.

In my instance, I have an array of structures which I want to iterate. I believe
this make my code more readable. I have already had another vote to say that
they thought the same.

I'm certain that throughout the media tree there are a lot of use cases where
arrays of structures define types which must be searched where this macro could
also make sense.

Do I need to start a poll to determine if this is a worthy pursuit? or am I to
give up and stop in my tracks (I'm a bit too tenacious usually to give up - so
someone 'high up' better make a clear statement saying ... just give up...
otherwise I likely won't)

Either way - I intend to add an equivalent macro to the UVC driver [1][2]
(because as I said - I believe it makes sense), and I have the support of the
maintainer there, so It seems a shame to have to duplicate the implementation in
other use cases where this would make the code more friendly.


/me awaits a NACK-FULL-STOP, or now fears if I'm about to be the cause of a
flame war :-S

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?h=kernel/array-iterator

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?h=kernel/array-iterator&id=3dece696e5b19d79c94f88c9df77482542d49a75

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?h=kernel/array-iterator&id=a31a5424a6577e14d46ce24ef0eff35de3e089bc

--
Kieran

Re: [PATCH][RFC] kernel.h: provide array iterator

[Kieran Bingham]

Hi Rasmus,

On 16/03/18 16:27, Rasmus Villemoes wrote:
> On 2018-03-15 11:00, Kieran Bingham wrote:
>> Simplify array iteration with a helper to iterate each entry in an array.
>> Utilise the existing ARRAY_SIZE macro to identify the length of the array
>> and pointer arithmetic to process each item as a for loop.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  include/linux/kernel.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> The use of static arrays to store data is a common use case throughout the
>> kernel. Along with that is the obvious need to iterate that data.
>>
>> In fact there are just shy of 5000 instances of iterating a static array:
>> 	git grep "for .*ARRAY_SIZE" | wc -l
>> 	4943
>>
>> When working on the UVC driver - I found that I needed to split one such
>> iteration into two parts, and at the same time felt that this could be
>> refactored to be cleaner / easier to read. 
> 
> About that, it would be helpful if you first converted to the new
> iterator, so that one can more easily see they are equivalent. And then
> split in two, adding the flush_workqueue call. Or do it the other way
> around. But please don't mix the two in one patch, especially not if
> it's supposed to act as an example of how to use the new helper.
>

My apologies - in the example below I was trying to show the usage and reason
for the macro. This was not meant to be a change to be integrated - the 'example
change' is not how the change will be committed, or included in the patch - but
was added here purely to show the usage / reason for the new macro and promote
the discussion. (So I'll already call that a success)

But that is a good point - the example usage could be much simplified here, and
then included in the commit message.


>> I do however worry that this simple short patch might not be desired or could
>> also be heavily bikeshedded due to it's potential wide spread use (though
>> perhaps that would be a good thing to have more users) ...  but here it is,
>> along with an example usage below which is part of a separate series.
> 
> I think it can be useful, and it does have the must_be_array protection
> built in, so code doesn't silently break if one changes from a
> fixed-size allocation to e.g. a kmalloc-based one. Just don't attempt a
> tree-wide mass conversion, but obviously starting to make use of it when
> refactoring code anyway is fine.

Well it had already been suggested to try to make a coccinelle patch - but I
suspect time and effort required may delay or postpone that currently.

I'll focus on seeing if I can actually get this macro in before expending effort
on a full conversion :-D

I originally anticipated that this would be a 'convert or use as required' style
change.


> And now, the bikeshedding you expected :)

It wouldn't be a discussion without it :D


>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index ce51455e2adf..95d7dae248b7 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -70,6 +70,16 @@
>>   */
>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>  
>> +/**
>> + * for_each_array_element - Iterate all items in an array
>> + * @elem: pointer of array type for iteration cursor
> 
> Hm, "pointer of array type" sounds wrong; it's not a "pointer to array".
> But "pointer of array elements' type" is clumsy. Maybe just "@elem:
> iteration cursor" is clear enough.

"@elem: iteration cursor" sounds good to me.

Depending on how the other conversations go here - I will likely make this
change. (I see there was a previous attempt at including a very similar macro)


>> + * @array: array to be iterated
>> + */
>> +#define for_each_array_element(elem, array) \
>> +	for (elem = &(array)[0]; \
>> +	     elem < &(array)[ARRAY_SIZE(array)]; \
>> +	     ++elem)
>> +
> 
> Please parenthesize elem as well.

That's certainly a good point! Thanks :D

> Rasmus

Regards

Kieran Bingham

Re: [PATCH][RFC] kernel.h: provide array iterator

[Alexey Dobriyan]

Proper form for iteration over an array is below:

	int a[] = {1, 2, 3, 5};
	for (int x: a) {
		...
	}

Your macro forces an iterator to be pointer which is uh-oh.