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

[Kees Cook]

On Thu, Mar 15, 2018 at 3:00 AM, 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.
---
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.
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
=============================================================================
+#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
+ */
+#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

[Julia Lawall]

Post by Kees Cook
On Thu, Mar 15, 2018 at 3:00 AM, 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.
---
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.
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

Post by Kees Cook
-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
=============================================================================
+#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
+ */
+#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

[Kieran Bingham]

Hi Kees, Julia,

Post by Julia Lawall

Post by Kees Cook
On Thu, Mar 15, 2018 at 3:00 AM, 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.
---
 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.
        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!

Post by Julia Lawall

Post by Kees Cook
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...)

Post by Julia Lawall
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...

Post by Julia Lawall
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.

Post by Julia Lawall
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)

Post by Julia Lawall
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

Post by Julia Lawall
julia

Post by Kees Cook
-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
=============================================================================
+#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
+ */
+#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