refactor(parameters): adopt Beat Küng's suggestion

src/lib/parameters/DynamicSparseLayer.h

@@ -55,20 +55,20 @@ public:
 			slots[i] = {UINT16_MAX, param_value_u{}};
 		}
 
-		_slots.store(slots);
+		_slots = slots;
 	}
 
 	virtual ~DynamicSparseLayer()
 	{
-		if (_slots.load()) {
-			free(_slots.load());
+		if (_slots) {
+			free(_slots);
 		}
 	}
 
 	bool store(param_t param, param_value_u value) override
 	{
 		AtomicTransaction transaction;
-		Slot *slots = _slots.load();
+		Slot *slots = _slots;
 
 		const int index = _getIndex(param);
 
@@ -84,7 +84,7 @@ public:
 				return false;
 			}
 
-			_slots.load()[_next_slot++] = {param, value};
+			_slots[_next_slot++] = {param, value};
 			_sort();
 		}
 
@@ -101,7 +101,7 @@ public:
 	{
 		px4::AtomicBitset<PARAM_COUNT> set;
 		const AtomicTransaction transaction;
-		Slot *slots = _slots.load();
+		Slot *slots = _slots;
 
 		for (int i = 0; i < _next_slot; i++) {
 			set.set(slots[i].param);
@@ -113,7 +113,7 @@ public:
 	param_value_u get(param_t param) const override
 	{
 		const AtomicTransaction transaction;
-		Slot *slots = _slots.load();
+		Slot *slots = _slots;
 
 		const int index = _getIndex(param);
 
@@ -128,7 +128,7 @@ public:
 	{
 		const AtomicTransaction transaction;
 		int index = _getIndex(param);
-		Slot *slots = _slots.load();
+		Slot *slots = _slots;
 
 		if (index < _next_slot) {
 			slots[index] = {UINT16_MAX, param_value_u{}};
@@ -167,14 +167,14 @@ private:
 
 	void _sort()
 	{
-		qsort(_slots.load(), _n_slots, sizeof(Slot), _slotCompare);
+		qsort(_slots, _n_slots, sizeof(Slot), _slotCompare);
 	}
 
 	int _getIndex(param_t param) const
 	{
 		int left = 0;
 		int right = _next_slot - 1;
-		Slot *slots = _slots.load();
+		Slot *slots = _slots;
 
 		while (left <= right) {
 			int mid = (left + right) / 2;
@@ -199,76 +199,53 @@ private:
 			return false;
 		}
 
-#ifdef __PX4_NUTTX
-		int max_retries = 5;
+		unsigned max_retries = 5;
 
-		// On NuttX, malloc/free can't be called with IRQs disabled,
-		// so we unlock around them and use CAS to handle races.
+		// As malloc uses locking, so we need to re-enable IRQ's during malloc/free and
+		// then exchange the buffer inside a critical section.
 		while (_next_slot >= _n_slots && max_retries-- > 0) {
-			Slot *previous_slots = nullptr;
-			Slot *new_slots = nullptr;
-
-			do {
-				previous_slots = _slots.load();
-				int alloc_n_slots = _n_slots;
-				transaction.unlock();
-
-				if (new_slots) {
-					free(new_slots);
-				}
-
-				new_slots = (Slot *) malloc(sizeof(Slot) * (alloc_n_slots + _n_grow));
-				transaction.lock();
-
-				if (new_slots == nullptr) {
-					return false;
-				}
-
-			} while (!_slots.compare_exchange(&previous_slots, new_slots));
-
-			memcpy(new_slots, previous_slots, sizeof(Slot) * _n_slots);
-
-			for (int i = _n_slots; i < _n_slots + _n_grow; i++) {
-				new_slots[i] = {UINT16_MAX, param_value_u{}};
-			}
-
-			_n_slots += _n_grow;
+			const int n_slots_new = _n_slots + _n_grow;
 
 			transaction.unlock();
-			free(previous_slots);
+			Slot *new_slots = (Slot *) malloc(sizeof(Slot) * n_slots_new);
 			transaction.lock();
-		}
-
-#else
-
-		// On POSIX, malloc/free work fine under the mutex, so we can
-		// hold the lock throughout and avoid CAS/ABA race conditions.
-		if (_next_slot >= _n_slots) {
-			Slot *old = _slots.load();
-			Slot *new_slots = (Slot *)malloc(sizeof(Slot) * (_n_slots + _n_grow));
 
 			if (new_slots == nullptr) {
 				return false;
 			}
 
-			memcpy(new_slots, old, sizeof(Slot) * _n_slots);
+			if (_n_slots + _n_grow == n_slots_new) {
+				Slot *previous_slots = _slots;
+				memcpy(new_slots, previous_slots, sizeof(Slot) * _n_slots);
 
-			for (int i = _n_slots; i < _n_slots + _n_grow; i++) {
-				new_slots[i] = {UINT16_MAX, param_value_u{}};
+				for (int i = _n_slots; i < n_slots_new; i++) {
+					new_slots[i] = {UINT16_MAX, param_value_u{}};
+				}
+
+				_slots = new_slots;
+				_n_slots = n_slots_new;
+
+				transaction.unlock();
+				free(previous_slots);
+				transaction.lock();
+				// After freeing previous_slots, we still need to continue the loop, because we just unlocked
+				// the critical section, so the buffer might already be full again at this point.
+
+			} else {
+				// If we end up here then another thread (successfully) increased the buffer already.
+				// So we can drop the new buffer but we will still need to check again if we need to
+				// increase the buffer even more.
+				transaction.unlock();
+				free(new_slots);
+				transaction.lock();
 			}
-
-			_slots.store(new_slots);
-			_n_slots += _n_grow;
-			free(old);
 		}
 
-#endif
-
 		return _next_slot < _n_slots;
 	}
 
 	int _next_slot = 0;
 	int _n_slots = 0;
 	const int _n_grow;
-	px4::atomic<Slot *> _slots{nullptr};
+	Slot *_slots{nullptr};
 };