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

[Tomi Valkeinen]

On 31/05/2022 08:51, Tomi Valkeinen wrote:
> 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;
> 
>               {

I think this could use std::atomic_signal_fence, but I'm pretty sure 
there's no real life performance difference.

  Tomi