[libcamera-devel] [PATCH v4 11/16] py: merge read_event() and get_ready_requests()

[Tomi Valkeinen]

On 30/05/2022 19:57, Tomi Valkeinen wrote:

>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>> index fcf009f0..505cc3dc 100644
>>> --- a/src/py/libcamera/py_main.cpp
>>> +++ b/src/py/libcamera/py_main.cpp
>>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)
>>>               return gEventfd;
>>>           })
>>> -        .def("read_event", [](CameraManager &) {
>>> +        .def("get_ready_requests", [](CameraManager &) {
>>>               uint8_t buf[8];
>>> -            int ret = read(gEventfd, buf, 8);
>>> -            if (ret != 8)
>>> +            if (read(gEventfd, buf, 8) != 8)
>>>                   throw std::system_error(errno, 
>>> std::generic_category());
>>
>> We need a memory barrier here to prevent reordering. I'm quite sure
> 
> We then need the same in handleRequestCompleted().
> 
>> there's one somewhere due to the read() call and the lock below, but I
>> haven't been able to figure out the C++ rule that guarantees this. Does
>> anyone know ?
> 
> https://en.cppreference.com/w/cpp/atomic/memory_order
> 
> So the mutex brings the normal acquire-release ordering. I would think a 
> syscall includes a barrier, but I can't find any details right away.

Perhaps something like this? I don't know if the fences are needed, but maybe
better safe than sorry.

diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
index 505cc3dc..b035a101 100644
--- a/src/py/libcamera/py_main.cpp
+++ b/src/py/libcamera/py_main.cpp
@@ -5,6 +5,7 @@
   * Python bindings
   */
  
+#include <atomic>
  #include <mutex>
  #include <stdexcept>
  #include <sys/eventfd.h>
@@ -116,6 +117,12 @@ static void handleRequestCompleted(Request *req)
  		gReqList.push_back(req);
  	}
  
+	/*
+	 * Prevent the write below from possibly being reordered to happen
+	 * before the push_back() above.
+	 */
+	std::atomic_thread_fence(std::memory_order_acquire);
+
  	uint64_t v = 1;
  	size_t s = write(gEventfd, &v, 8);
  	/*
@@ -219,6 +226,12 @@ PYBIND11_MODULE(_libcamera, m)
  			if (read(gEventfd, buf, 8) != 8)
  				throw std::system_error(errno, std::generic_category());
  
+			/*
+			 * Prevent the read above from possibly being reordered
+			 * to happen after the swap() below.
+			 */
+			std::atomic_thread_fence(std::memory_order_release);
+
  			std::vector<Request *> v;
  
  			{