Skip to contentSkip to conversation

treewide: rework struct gpiod_line_bulk

PATCH In review Bartosz Golaszewski 2 participants 2 messages Oct 30, 2020 – Nov 4, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 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.

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.

Some additional improvements to the patch by:
Kent Gibson <[email protected]>

---
v1 -> v2:
- make offset -> index in all bulk-related interfaces and docs
- drop the owner chip reference from struct gpiod_line_bulk and reuse the owner of the first line
- drop bulk_same_chip helper

v2 -> v3:
- drop bulk iterators in favor of looping over bulk indices

 bindings/cxx/gpiod.hpp        |  20 ++-
 bindings/cxx/line_bulk.cpp    |  89 ++++++-------
 bindings/python/gpiodmodule.c | 169 ++++++++++++++++---------
 include/gpiod.h               | 143 +++++++++------------
 lib/core.c                    | 211 +++++++++++++++++++------------
 lib/ctxless.c                 |  53 +++++---
 lib/helpers.c                 |  61 +++++----
 tests/Makefile.am             |   1 +
 tests/gpiod-test.h            |   2 +
 tests/tests-bulk.c            | 158 ++++++++++++++++++++++++
 tests/tests-chip.c            |  59 +++++----
 tests/tests-event.c           |  73 +++++++----
 tests/tests-line.c            | 226 +++++++++++++---------------------
 13 files changed, 759 insertions(+), 506 deletions(-)
 create mode 100644 tests/tests-bulk.c
bindings/cxx/gpiod.hpp +15 -5
index 8dfc172..897d8c0 100644
@@ -626,18 +626,20 @@ public:
 
 	/**
 	 * @brief Get the line at given offset.
-	 * @param offset Offset of the line to get.
+	 * @param index Index of the line to get.
 	 * @return Reference to the line object.
+	 * @note This method will throw if index is equal or greater than the
+	 *       number of lines currently held by this bulk.
 	 */
-	GPIOD_API line& get(unsigned int offset);
+	GPIOD_API line& get(unsigned int index);
 
 	/**
 	 * @brief Get the line at given offset without bounds checking.
-	 * @param offset Offset of the line to get.
+	 * @param index Offset of the line to get.
 	 * @return Reference to the line object.
 	 * @note No bounds checking is performed.
 	 */
-	GPIOD_API line& operator[](unsigned int offset);
+	GPIOD_API line& operator[](unsigned int index);
 
 	/**
 	 * @brief Get the number of lines currently held by this object.
@@ -839,8 +841,16 @@ public:
 
 private:
 
+	struct line_bulk_deleter
+	{
+		void operator()(::gpiod_line_bulk *bulk);
+	};
+
 	void throw_if_empty(void) const;
-	void to_line_bulk(::gpiod_line_bulk* bulk) const;
+
+	using line_bulk_ptr = ::std::unique_ptr<::gpiod_line_bulk, line_bulk_deleter>;
+
+	line_bulk_ptr to_line_bulk(void) const;
 
 	::std::vector<line> _m_bulk;
 };
bindings/cxx/line_bulk.cpp +40 -49

206 lines not on this page · raw message

bindings/python/gpiodmodule.c +113 -56

380 lines not on this page · raw message

include/gpiod.h +60 -83

208 lines not on this page · raw message

lib/core.c +132 -79

380 lines not on this page · raw message

lib/ctxless.c +36 -17

156 lines not on this page · raw message

lib/helpers.c +40 -21

106 lines not on this page · raw message

tests/Makefile.am +1 -0
index f79f8d5..4a3befd 100644
@@ -22,6 +22,7 @@ bin_PROGRAMS = gpiod-test
 gpiod_test_SOURCES =			\
 		gpiod-test.c		\
 		gpiod-test.h		\
+		tests-bulk.c		\
 		tests-chip.c		\
 		tests-ctxless.c		\
 		tests-event.c		\
tests/gpiod-test.h +2 -0
index 8308547..a43109a 100644
@@ -24,10 +24,12 @@
  * regular 'struct typename' syntax.
  */
 typedef struct gpiod_chip gpiod_chip_struct;
+typedef struct gpiod_line_bulk gpiod_line_bulk_struct;
 typedef struct gpiod_chip_iter gpiod_chip_iter_struct;
 typedef struct gpiod_line_iter gpiod_line_iter_struct;
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(gpiod_chip_struct, gpiod_chip_close);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(gpiod_line_bulk_struct, gpiod_line_bulk_free);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(gpiod_chip_iter_struct, gpiod_chip_iter_free);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(gpiod_line_iter_struct, gpiod_line_iter_free);
 
tests/tests-bulk.c new file +158 -0

158 lines not on this page · raw message

tests/tests-chip.c +29 -30

114 lines not on this page · raw message

tests/tests-event.c +47 -26

202 lines not on this page · raw message

tests/tests-line.c +86 -140

416 lines not on this page · raw message

Signature · 1 line
2.29.1
#1 lore raw
Quoted 35 lines · Bartosz Golaszewski

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.

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.

Some additional improvements to the patch by:
Kent Gibson <[email protected]>

---
v1 -> v2:
- make offset -> index in all bulk-related interfaces and docs
- drop the owner chip reference from struct gpiod_line_bulk and reuse the owner of the first line
- drop bulk_same_chip helper

Quoted 3 lines

v2 -> v3:
- drop bulk iterators in favor of looping over bulk indices

Applied this patch. We can add some tweaks later.

Bartosz