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

[Tomi Valkeinen]

On 04/06/2022 02:16, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thu, Jun 02, 2022 at 11:27:35AM +0300, Tomi Valkeinen wrote:
>> 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.
> 
> Thanks for all the information. If I understand things correctly, the
> mutex should be enough. See below (where I'm sure I'll say many stupid,
> or at least incorrect, things).
> 
>>> 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)
> 
> Adding some more context:
> 
>            {
>                std::lock_guard guard(gReqlistMutex);
> 	                      ^---- [1]
>>>            gReqList.push_back(req);
>>>        }
>            ^---- [2]
> 
> [1] locks the mutex, which is an acquire operation. It means that no
> read or write in the same thread after the lock() can be reordered
> before it.
> 
> [2] unlocks the mutex, which is a release operation. It means that no
> read or write in the same thread before the unlock() can be reordered
> after it.
> 
> On the reader side, we could thus observe the write() below before the
> push_back, but never before the lock(). Effectively, what we could see
> on the reader thread, assuming that the write() doesn't contain any
> barrier (which is likely incorrect), is
> 
> 	{
> 		std::lock_guard guard(gReqlistMutex);
> 		write(gEventfd, &v, 8);
> 		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;
>>>
>>>                {
>                            std::lock_guard guard(gReqlistMutex);
> 			                  ^---- [3]
>                            swap(v, gReqList);
>                    }
> 		  ^---- [4]
> 
> Similarly, [3] is an acquire operation, [3] is a release operation. What
> could thus be observed by the writer is
> 
> 	{
> 		std::lock_guard guard(qReglistMutex);
> 		swap(v, gReqList);
> 		read(gEventfd, buf, 8);
> 	}
> 
> If get_ready_requests() gets called because select() wakes up due to an
> event on the eventfd, it means the write() has been performed. While it
> may be seen by the reader before the .push_back(), the lock will ensure
> sequential consistency between the reader and the writer, the reader
> won't be able to acquire the lock before the writer releases it, so
> swap() will not occur before the .push_back() is observed.
> 
> If get_ready_requests() gets called in blocking mode, read() would
> block, so there's no way it can get reordered after the lock() by the
> CPU as the latter won't be executed yet. I'm pretty sure the "as-is"
> rule (https://en.cppreference.com/w/cpp/language/as_if) prevents the
> compiler from reordering the read() after the lock() :-) This should
> thus also be safe.

I agree. I also studied all possible ordering combinations, and I cannot 
find a case which wouldn't work (without any fences).

And I also find it difficult to believe the read/write could happen 
inside the locks, although I still don't know why.

Consider two threads, each of which runs a function which does lock(); 
access-shared-variable; unlock(). If you then add a read() or write() 
call to one of those functions, and the call blocks for a longer time, 
and that call is moved to happen inside the lock, it would cause the 
other thread to be blocked too.

I don't think I would ever figure out that kind of bug, if it would 
happen. But why it would not happen?

Words of wisdom: Single threaded programming is hard. Multi-threaded 
programming is impossible.

> Tl;dr:
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Now the real question is: how many mistakes or inaccuracies does the
> above explanation contain ? :-)

In ten years, we happen to encounter this mail via some random googling, 
and, oh man, the shame of writing all this nonsense when we were young 
and ignorant...

  Tomi