[{"id":5367,"web_url":"https://patchwork.libcamera.org/comment/5367/","msgid":"<20200624000243.GQ5870@pendragon.ideasonboard.com>","date":"2020-06-24T00:02:43","subject":"Re: [libcamera-devel] [PATCH v3 22/22] v4l2: v4l2_camera_proxy:\n\tSerialize accesses to the proxy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jun 24, 2020 at 04:08:36AM +0900, Paul Elder wrote:\n> Make the V4L2 compatibility layer thread-safe by serializing accesses to\n> the V4L2CameraProxy with a lock. Release the lock when blocking for\n> dqbuf.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v3\n> ---\n> src/v4l2/v4l2_camera_proxy.cpp | 21 +++++++++++++++++----\n> src/v4l2/v4l2_camera_proxy.h | 5 ++++-\n> 2 files changed, 21 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index ed3bcbc..fb3a1fc 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -43,6 +43,8 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index,\n> \n> int V4L2CameraProxy::open(V4L2CameraFile *file)\n> {\n> +\tMutexLocker locker(proxyMutex_);\n> +\n\nYou could move the lock after the LOG() lines to slightly reduced the\namount of code protected by it.\n\n> \tLOG(V4L2Compat, Debug) << \"Servicing open fd = \" << file->efd();\n> \n> \tfiles_.insert(file);\n> @@ -75,6 +77,8 @@ int V4L2CameraProxy::open(V4L2CameraFile *file)\n> \n> void V4L2CameraProxy::close(V4L2CameraFile *file)\n> {\n> +\tMutexLocker locker(proxyMutex_);\n> +\n> \tLOG(V4L2Compat, Debug) << \"Servicing close fd = \" << file->efd();\n> \n> \tfiles_.erase(file);\n> @@ -90,6 +94,8 @@ void V4L2CameraProxy::close(V4L2CameraFile *file)\n> void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> \t\t\t off64_t offset)\n> {\n> +\tMutexLocker locker(proxyMutex_);\n> +\n> \tLOG(V4L2Compat, Debug) << \"Servicing mmap\";\n> \n> \t/* \\todo Validate prot and flags properly. */\n> @@ -124,6 +130,8 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,\n> \n> int V4L2CameraProxy::munmap(void *addr, size_t length)\n> {\n> +\tMutexLocker locker(proxyMutex_);\n> +\n> \tLOG(V4L2Compat, Debug) << \"Servicing munmap\";\n> \n> \tauto iter = mmaps_.find(addr);\n> @@ -592,7 +600,8 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> \treturn ret;\n> }\n> \n> -int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> +int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file,\n> +\t\t\t\t struct v4l2_buffer *arg, MutexLocker &locker)\n\nWe usually passed parameters that can be modified by pointer, not by\nreference.\n\nAnd you could keep the first two parameters on the first line.\n\n> {\n> \tLOG(V4L2Compat, Debug) << \"Servicing vidioc_dqbuf fd = \" << file->efd();\n> \n> @@ -609,9 +618,11 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)\n> \t !validateMemoryType(arg->memory))\n> \t\treturn -EINVAL;\n> \n> -\tif (!(file->nonBlocking()))\n> +\tif (!(file->nonBlocking())) {\n> +\t\tlocker.unlock();\n> \t\tvcam_->waitForBufferAvailable();\n> -\telse if (!vcam_->isBufferAvailable())\n> +\t\tlocker.lock();\n\nThere's a potential race condition here. waitForBufferAvailable() is\nimplemented as\n\nvoid V4L2Camera::waitForBufferAvailable()\n{\n\tMutexLocker locker(bufferMutex_);\n bufferCV_.wait(locker, [&] {\n\t\t\treturn bufferAvailableCount_ >= 1 || !isRunning_;\n\t\t\t});\n\tif (isRunning_)\n\t\tbufferAvailableCount_--;\n}\n\nand in V4L2Camera::streamOff(), you have\n\n\tisRunning_ = false;\n\tbufferCV_.notify_all();\n\nwithout taking the bufferMutex_ lock. streamOff() can run in a parallel\nthread to waitForBufferAvailable() as you release the locker before\ncalling waitForBufferAvailable(). Unless I'm mistaken, the notify_all()\ncall doesn't count as a release operation (in the C++ memory model\nsense, see https://people.cs.pitt.edu/~xianeizhang/notes/cpp11_mem.html\nfor a local resource :-)). The write to isRunning_ could then be visible\nto the thread waiting on bufferCV_ only after bufferCV_.notify_all()\ngets visible. The wait() would wake up, not see !isRunning_, go back to\nsleep, and stay there forever. I initially thought the global lock would\nsolve this, but after further analysis, I don't think that's the case.\n\nFortunately, I think the fix should be a simple as\n\n\t{\n\t\tMutexLocker locker(buferMutex_);\n\t\tisRunning_ = false;\n\t}\n\tbufferCV_notify_all();\n\nin V4L2Camera::streamOff() in the patch that dropped usage of the\nSemaphore class.\n\nI'll let you confirm my analysis :-)\n\nFor this patch,\n\nReviewed-by: Laurent Pinchart \n\n> +\t} else if (!vcam_->isBufferAvailable())\n> \t\treturn -EAGAIN;\n> \n> \t/*\n> @@ -706,6 +717,8 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \n> int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> {\n> +\tMutexLocker locker(proxyMutex_);\n> +\n> \tif (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n> \t\terrno = EFAULT;\n> \t\treturn -1;\n> @@ -766,7 +779,7 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar\n> \t\tret = vidioc_qbuf(file, static_cast(arg));\n> \t\tbreak;\n> \tcase VIDIOC_DQBUF:\n> -\t\tret = vidioc_dqbuf(file, static_cast(arg));\n> +\t\tret = vidioc_dqbuf(file, static_cast(arg), locker);\n> \t\tbreak;\n> \tcase VIDIOC_STREAMON:\n> \t\tret = vidioc_streamon(file, static_cast(arg));\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 041f536..b6d2c2c 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -61,7 +61,7 @@ private:\n> \tint vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);\n> \tint vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> \tint vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> -\tint vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);\n> +\tint vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg, MutexLocker &locker);\n> \tint vidioc_streamon(V4L2CameraFile *file, int *arg);\n> \tint vidioc_streamoff(V4L2CameraFile *file, int *arg);\n> \n> @@ -105,6 +105,9 @@ private:\n> \t * g/s_priority, enum/g/s_input\n> \t */\n> \tV4L2CameraFile *owner_;\n> +\n> +\t/* This mutex is to serialize access to the proxy. */\n> +\tMutex proxyMutex_;\n> };\n> \n> #endif /* __V4L2_CAMERA_PROXY_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86D25603B9\n\tfor ;\n\tWed, 24 Jun 2020 02:03:10 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0B002A9;\n\tWed, 24 Jun 2020 02:03:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ab9mWiN3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592956990;\n\tbh=1r51mlAFS8ANNG/wJt+YchU+mcXTCTelHY2FUdOW12o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ab9mWiN3u4vuF9KhHy5q1k2ww96hDeB/aYrxaNkuRMARDYjgboLR3ydI20MAeixys\n\tRZ0VZqG8F6iVIcZBJ7ka1VP/5bjYEqOVSosyW0kO73hXvJHdUD1+S4BagHOtD5KlsM\n\t6iiLAORII7CDQziizeF8G0peLSWC46XpQcOQW1Bo=","Date":"Wed, 24 Jun 2020 03:02:43 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200624000243.GQ5870@pendragon.ideasonboard.com>","References":"<20200623190836.53446-1-paul.elder@ideasonboard.com>\n\t<20200623190836.53446-23-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200623190836.53446-23-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 22/22] v4l2: v4l2_camera_proxy:\n\tSerialize accesses to the proxy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 24 Jun 2020 00:03:10 -0000"}}]