treewide: rework struct gpiod_line_bulk

From: Bartosz Golaszewski <[email protected]>

Current implementation of struct gpiod_line_bulk uses stack memory excessively. The structure is big: it's an array 64 pointers + 4 bytes size. That amounts to 260 bytes on 32-bit and 516 bytes on 64-bit architectures respectively. It's also used everywhere as all functions dealing with single lines eventually end up calling bulk counterparts.

The rework addresses it by making the bulk structure opaque and providing appropriate interfaces for library users while retaining a way for internal users to allocate single line bulks on the stack.

The macro-based loop has been removed. In its place we provide a function iterating over all lines held by a bulk and calling the provided callback function for each line as well as a new line bulk iterator which works similarily to chip and line iterators.

Since bulk operations can now fail, a bunch of test-cases has been added to cover the relevant code.

While at it: using the word offset both when referring to line's HW offset in a chip as well as the offset in a bulk leads to confusion. This patch renames the bulk offset to index.

Quoted 1 line · Bartosz Golaszewski

lib/core.c struct gpiod_chip { · line 87

Quoted 6 lines

lib/core.c +5 -0

+struct gpiod_line_bulk {
+       struct gpiod_chip *owner;
+       unsigned int num_lines;
+       unsigned int max_lines;
+       struct gpiod_line *lines[1];

Why not '[]' as we do in the kernel?

lib/core.c struct gpiod_chip { · line 91

Quoted 4 lines

lib/core.c +3 -0

+};
 
+#define BULK_SINGLE_LINE_INIT(line) \
+               { gpiod_line_get_chip(line), 1, 1, { (line) } }

Hmm... Perhaps union can help here?

union {
struct *lines[];
struct *line;
}

num_lines == 1 exactly defines this.

Quoted 1 line · Andy Shevchenko

On Fri, Oct 23, 2020 at 12:31 PM Bartosz Golaszewski <[email protected]> wrote:

Quoted 8 lines

lib/core.c struct gpiod_chip { · line 87

Quoted 5 lines

lib/core.c +5 -0

+struct gpiod_line_bulk {
+       struct gpiod_chip *owner;
+       unsigned int num_lines;
+       unsigned int max_lines;
+       struct gpiod_line *lines[1];

Why not '[]' as we do in the kernel?

Because I want to be able to store a single line in this structure if I do:

    struct gpiod_line_bulk bulk;

Using [] makes the size of this structure not take into account the last array member.

Quoted 12 lines

lib/core.c struct gpiod_chip { · line 91

Quoted 4 lines

lib/core.c +3 -0

+};
 
+#define BULK_SINGLE_LINE_INIT(line) \
+               { gpiod_line_get_chip(line), 1, 1, { (line) } }

Hmm... Perhaps union can help here?

union {
struct *lines[];
struct *line;
}

Quoted 2 lines

num_lines == 1 exactly defines this.

Nope because gcc will scream:

error: flexible array member in union

Quoted 2 lines · Bartosz Golaszewski

On Fri, Oct 23, 2020 at 12:24 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 3 lines

Nope because gcc will scream:

error: flexible array member in union

Ah, of course. Should be
struct ... **lines;

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Bartosz Golaszewski

On Fri, Oct 23, 2020 at 12:24 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 6 lines

Quoted 3 lines

Nope because gcc will scream:

error: flexible array member in union

Ah, of course. Should be
struct ... **lines;

But it is not gonna work... we need an array here. or just one member

struct *lines;

bulk:
lines = malloc(num_lines * sizeof(lines));
xxx->lines = lines;

single:
xxx->lines = line;

Quoted 24 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Bartosz Golaszewski

On Fri, Oct 23, 2020 at 12:24 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 6 lines

Quoted 3 lines

Nope because gcc will scream:

error: flexible array member in union

Ah, of course. Should be
struct ... **lines;

But it is not gonna work... we need an array here. or just one member

struct *lines;

bulk:
lines = malloc(num_lines * sizeof(lines));
xxx->lines = lines;

Quoted 2 lines

single:
xxx->lines = line;

The definition I used is clearer - it's explicit about using an array with a single member by default and can be extended as needed when allocating.

Quoted 26 lines · Bartosz Golaszewski

Quoted 24 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Bartosz Golaszewski

On Fri, Oct 23, 2020 at 12:24 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 6 lines

Quoted 3 lines

Nope because gcc will scream:

error: flexible array member in union

Ah, of course. Should be
struct ... **lines;

But it is not gonna work... we need an array here. or just one member

struct *lines;

bulk:
lines = malloc(num_lines * sizeof(lines));
xxx->lines = lines;

Quoted 6 lines

Quoted 2 lines

single:
xxx->lines = line;

The definition I used is clearer - it's explicit about using an array with a single member by default and can be extended as needed when allocating.

According to [1] it makes harder to avoid sizeof() type of calculation mistakes for such struct.

From my point of view extending something that has been already predefined is not cool. But it's up to you to decide.

[1]: https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays

From: Bartosz Golaszewski <[email protected]>

Current implementation of struct gpiod_line_bulk uses stack memory excessively. The structure is big: it's an array 64 pointers + 4 bytes size. That amounts to 260 bytes on 32-bit and 516 bytes on 64-bit architectures respectively. It's also used everywhere as all functions dealing with single lines eventually end up calling bulk counterparts.

The rework addresses it by making the bulk structure opaque and providing appropriate interfaces for library users while retaining a way for internal users to allocate single line bulks on the stack.

The macro-based loop has been removed. In its place we provide a function iterating over all lines held by a bulk and calling the provided callback function for each line as well as a new line bulk iterator which works similarily to chip and line iterators.

Since bulk operations can now fail, a bunch of test-cases has been added to cover the relevant code.

While at it: using the word offset both when referring to line's HW offset in a chip as well as the offset in a bulk leads to confusion. This patch renames the bulk offset to index.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 bindings/cxx/gpiod.hpp        |  18 ++-
 bindings/cxx/line_bulk.cpp    | 112 +++++++++--------
 bindings/python/gpiodmodule.c | 182 +++++++++++++++++++--------
 include/gpiod.h               | 169 +++++++++++++------------
 lib/core.c                    | 178 +++++++++++++++++++--------
 lib/ctxless.c                 |  53 +++++---
 lib/helpers.c                 |  61 +++++----
 lib/iter.c                    |  33 +++++
 tests/Makefile.am             |   1 +
 tests/gpiod-test.h            |   5 +
 tests/tests-bulk.c            | 158 ++++++++++++++++++++++++
 tests/tests-chip.c            |  59 +++++----
 tests/tests-event.c           |  73 +++++++----
 tests/tests-iter.c            |  26 ++++
 tests/tests-line.c            | 225 +++++++++++++---------------------
 15 files changed, 888 insertions(+), 465 deletions(-)
 create mode 100644 tests/tests-bulk.c

bindings/cxx/gpiod.hpp +0 -0

index 8dfc172..83f543f 100644

From: Bartosz Golaszewski <[email protected]>

lib/core.c +3 -0

@@ -74,6 +74,106 @@ struct gpiod_chip {
 	char label[32];
 };
 
+/*
+ * The structure is defined in a way that allows internal users to allocate
+ * bulk objects that hold a single line on the stack - that way we can reuse

lib/iter.c +23 -0

+struct gpiod_line_bulk_iter {
+	struct gpiod_line_bulk *bulk;
+	unsigned int num_lines;
+	unsigned int index;
+};
+
 static int dir_filter(const struct dirent *dir)
 {
 	return !strncmp(dir->d_name, "gpiochip", 8);

@@ -169,3 +175,30 @@ struct gpiod_line *gpiod_line_iter_next(struct gpiod_line_iter *iter)
 	return iter->offset < (iter->num_lines)
 					? iter->lines[iter->offset++] : NULL;
 }
+
+struct gpiod_line_bulk_iter *
+gpiod_line_bulk_iter_new(struct gpiod_line_bulk *bulk)
+{
+	struct gpiod_line_bulk_iter *iter;
+
+	iter = malloc(sizeof(*iter));
+	if (!iter)
+		return NULL;
+
+	iter->bulk = bulk;
+	iter->index = 0;
+	iter->num_lines = gpiod_line_bulk_num_lines(bulk);
+
+	return iter;
+}
+

Quoted 2 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

[snip]

Quoted 13 lines

lib/core.c +9 -0

@@ -74,6 +74,106 @@ struct gpiod_chip {
      char label[32];
 };
 
+/*
+ * The structure is defined in a way that allows internal users to allocate
+ * bulk objects that hold a single line on the stack - that way we can reuse
+ * a lot of code between functions that take single lines and those that take
+ * bulks as arguments while not unnecessarily allocating memory dynamically.
+ */
+struct gpiod_line_bulk {
+     struct gpiod_chip *owner;
+     unsigned int num_lines;

lib/core.c struct gpiod_chip { · line 89

Quoted 4 lines

lib/core.c +4 -0

+     unsigned int max_lines;
+     struct gpiod_line *lines[1];
+};
+

owner is only used to check that added lines are on same chip. Just compare with lines[0]->chip in gpiod_line_bulk_add_line() instead?

Also, line_bulk_same_chip() is now redundant as lines can only be added to the bulk via gpiod_line_bulk_add_line() which performs the check, so remove it and all uses of it throughout.

I question the value of the struct gpiod_line_bulk_iter, and also struct gpiod_line_iter.
They seem worse than the user performing a for-loop over the bulk indicies or chip offsets and getting each line themselves. They add a malloc overhead, in the case of gpiod_line_iter both a malloc and a calloc, as well as the corresponding frees.

What benefit do they provide?

Similarly gpiod_line_bulk_foreach_line().

And I'm not sure about the utility of the struct gpiod_chip_iter either as it bails if opening any of the chips fails. There a several reasons that could occur, e.g. permissions or unplugging, so you might want to leave it to the user to decide what to do.

So I would prefer an iter that provides prospective chip names, or just a scandir() wrapper that returns an array of names.

Wrt rethinking the libgpiod API for v2, I'd like to either reduce the API to a minimal function set, for example stripping out the iters, or at least identify the minimal set that everything else is built on, and then look at how to rework those to expose the uAPI v2 features. e.g. given lines in a bulk now have to all be on the same chip, and given uAPI v2 supports per-line configs even within a bulk, the whole line and bulk lifecycle, as well as their relationship the chip and the line request, can be rethought.

I'd also like to see ALL libgpiod types become opaque.

But that is getting way outside the scope of this patch...