[{"id":2099,"web_url":"https://patchwork.libcamera.org/comment/2099/","msgid":"<20190701233618.GB9228@bigcity.dyn.berto.se>","date":"2019-07-01T23:36:18","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: ipc: unix: Make\n\tsocket operation asynchronous","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-07-02 02:23:39 +0300, Laurent Pinchart wrote:\n> Blocking socket operation when receiving messages may lead to long\n> delays, and possibly a complete deadlock, if the remote side delays\n> sending of the payload after the header, or doesn't send the payload at\n> all. To avoid this, make the socket non-blocking and implement a simple\n> state machine to receive the header synchronously with the socket read\n> notification. The payload read is still synchronous with the receive()\n> method to avoid data copies.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/include/ipc_unixsocket.h | 2 +\n> src/libcamera/ipc_unixsocket.cpp | 88 ++++++++++++++++++--------\n> 2 files changed, 63 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/include/ipc_unixsocket.h b/src/libcamera/include/ipc_unixsocket.h\n> index ef166d742554..03e9fe492bde 100644\n> --- a/src/libcamera/include/ipc_unixsocket.h\n> +++ b/src/libcamera/include/ipc_unixsocket.h\n> @@ -49,6 +49,8 @@ private:\n> \tvoid dataNotifier(EventNotifier *notifier);\n> \n> \tint fd_;\n> +\tbool headerReceived_;\n> +\tstruct Header header_;\n> \tEventNotifier *notifier_;\n> };\n> \n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> index c11f116093c5..def08eef00f8 100644\n> --- a/src/libcamera/ipc_unixsocket.cpp\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"ipc_unixsocket.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -49,10 +50,10 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)\n> * transporting entire payloads with guaranteed ordering.\n> *\n> * The IPC design is asynchronous, a message is queued to a receiver which gets\n> - * notified that a message is ready to be consumed by a signal. The queuer of\n> - * the message gets no notification when a message is delivered nor processed.\n> - * If such interactions are needed a protocol specific to the users use-case\n> - * should be implemented on top of the IPC objects.\n> + * notified that a message is ready to be consumed by the \\ref readyRead\n> + * signal. The sender of the message gets no notification when a message is\n> + * delivered nor processed. If such interactions are needed a protocol specific\n> + * to the users use-case should be implemented on top of the IPC objects.\n> *\n> * Establishment of an IPC channel is asymmetrical. The side that initiates\n> * communication first instantiates a local side socket and creates the channel\n> @@ -64,7 +65,7 @@ LOG_DEFINE_CATEGORY(IPCUnixSocket)\n> */\n> \n> IPCUnixSocket::IPCUnixSocket()\n> -\t: fd_(-1), notifier_(nullptr)\n> +\t: fd_(-1), headerReceived_(false), notifier_(nullptr)\n> {\n> }\n> \n> @@ -89,7 +90,7 @@ int IPCUnixSocket::create()\n> \tint sockets[2];\n> \tint ret;\n> \n> -\tret = socketpair(AF_UNIX, SOCK_DGRAM, 0, sockets);\n> +\tret = socketpair(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0, sockets);\n> \tif (ret) {\n> \t\tret = -errno;\n> \t\tLOG(IPCUnixSocket, Error)\n> @@ -142,6 +143,7 @@ void IPCUnixSocket::close()\n> \t::close(fd_);\n> \n> \tfd_ = -1;\n> +\theaderReceived_ = false;\n> }\n> \n> /**\n> @@ -193,38 +195,38 @@ int IPCUnixSocket::send(const Payload &payload)\n> * \\param[out] payload Payload where to write the received message\n> *\n> * This method receives the message payload from the IPC channel and writes it\n> - * to the \\a payload. It blocks until one message is received, if an\n> - * asynchronous behavior is desired this method should be called when the\n> - * readyRead signal is emitted.\n> + * to the \\a payload. If no message payload is available, it returns\n> + * immediately with -EAGAIN. The \\ref readyRead signal shall be used to receive\n> + * notification of message availability.\n> *\n> * \\todo Add state machine to make sure we don't block forever and that\n> * a header is always followed by a payload.\n> *\n> * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EAGAIN No message payload is available\n> + * \\retval -ENOTCONN The socket is not connected (neither create() nor bind()\n> + * has been called)\n> */\n> int IPCUnixSocket::receive(Payload *payload)\n> {\n> -\tHeader hdr;\n> -\tint ret;\n> -\n> \tif (!isBound())\n> \t\treturn -ENOTCONN;\n> \n> -\tif (!payload)\n> -\t\treturn -EINVAL;\n> +\tif (!headerReceived_)\n> +\t\treturn -EAGAIN;\n> \n> -\tret = ::recv(fd_, &hdr, sizeof(hdr), 0);\n> -\tif (ret < 0) {\n> -\t\tret = -errno;\n> -\t\tLOG(IPCUnixSocket, Error)\n> -\t\t\t<< \"Failed to recv header: \" << strerror(-ret);\n> +\tpayload->data.resize(header_.data);\n> +\tpayload->fds.resize(header_.fds);\n> +\n> +\tint ret = recvData(payload->data.data(), header_.data,\n> +\t\t\t payload->fds.data(), header_.fds);\n> +\tif (ret < 0)\n> \t\treturn ret;\n> -\t}\n> \n> -\tpayload->data.resize(hdr.data);\n> -\tpayload->fds.resize(hdr.fds);\n> +\theaderReceived_ = false;\n> +\tnotifier_->setEnabled(true);\n> \n> -\treturn recvData(payload->data.data(), hdr.data, payload->fds.data(), hdr.fds);\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -232,7 +234,8 @@ int IPCUnixSocket::receive(Payload *payload)\n> * \\brief A Signal emitted when a message is ready to be read\n> */\n> \n> -int IPCUnixSocket::sendData(const void *buffer, size_t length, const int32_t *fds, unsigned int num)\n> +int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> +\t\t\t const int32_t *fds, unsigned int num)\n> {\n> \tstruct iovec iov[1];\n> \tiov[0].iov_base = const_cast(buffer);\n> @@ -266,7 +269,8 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length, const int32_t *fd\n> \treturn 0;\n> }\n> \n> -int IPCUnixSocket::recvData(void *buffer, size_t length, int32_t *fds, unsigned int num)\n> +int IPCUnixSocket::recvData(void *buffer, size_t length,\n> +\t\t\t int32_t *fds, unsigned int num)\n> {\n> \tstruct iovec iov[1];\n> \tiov[0].iov_base = buffer;\n> @@ -291,8 +295,9 @@ int IPCUnixSocket::recvData(void *buffer, size_t length, int32_t *fds, unsigned\n> \n> \tif (recvmsg(fd_, &msg, 0) < 0) {\n> \t\tint ret = -errno;\n> -\t\tLOG(IPCUnixSocket, Error)\n> -\t\t\t<< \"Failed to recvmsg: \" << strerror(-ret);\n> +\t\tif (ret != -EAGAIN)\n> +\t\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t\t<< \"Failed to recvmsg: \" << strerror(-ret);\n> \t\treturn ret;\n> \t}\n> \n> @@ -303,6 +308,35 @@ int IPCUnixSocket::recvData(void *buffer, size_t length, int32_t *fds, unsigned\n> \n> void IPCUnixSocket::dataNotifier(EventNotifier *notifier)\n> {\n> +\tint ret;\n> +\n> +\tif (!headerReceived_) {\n> +\t\t/* Receive the header. */\n> +\t\tret = ::recv(fd_, &header_, sizeof(header_), 0);\n> +\t\tif (ret < 0) {\n> +\t\t\tret = -errno;\n> +\t\t\tLOG(IPCUnixSocket, Error)\n> +\t\t\t\t<< \"Failed to receive header: \" << strerror(-ret);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\theaderReceived_ = true;\n> +\t}\n> +\n> +\t/*\n> +\t * If the payload has arrived, disable the notifier and emit the\n> +\t * readyRead signal. The notifier will be reenabled by the receive()\n> +\t * method.\n> +\t */\n> +\tstruct pollfd fds = { fd_, POLLIN, 0 };\n> +\tret = poll(&fds, 1, 0);\n> +\tif (ret < 0)\n> +\t\treturn;\n> +\n> +\tif (!(fds.revents & POLLIN))\n> +\t\treturn;\n> +\n> +\tnotifier_->setEnabled(false);\n> \treadyRead.emit(this);\n> }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B49760BF8\n\tfor ;\n\tTue, 2 Jul 2019 01:36:20 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id a21so14963205ljh.7\n\tfor ;\n\tMon, 01 Jul 2019 16:36:19 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\tr24sm4000907ljb.72.2019.07.01.16.36.18\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tMon, 01 Jul 2019 16:36:18 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=812yY3fiC2uUi5lW7y1kAGs7NCP+ufL379B6Sc3JIkc=;\n\tb=AgysA8UOt6bT1m8wvcigVGVAp+/HBsBbYMhw/2xIkb0WW7QpBpv0jiAhEqryKxfvc8\n\tKFPbBX6b0+EMtqJ/dp0DsEA9V/YIUMb0Dg9yaAEXbExXizdEaQqp+kAmiuBCnlNpMPCs\n\tTkZfnQVOX3KOU8y3BkerL5cXDGDUbFVVZF2hgGJYyHKjXUi7gJ58UUvn+JUFYWqGR+3j\n\tZ06OC8svzbqq8kt8fnp6rfI7n7JsV28ufo/K7+lgSc2VX8+9RsQB5SF5YU9gw1RoxXK6\n\tCQoN1Ii5X2WyYgyObBzmjgCkABoJAWtxtc3IzwZeK0YJwdP3Gd/Mpo/bGNASxdq2SVqv\n\tc8+w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=812yY3fiC2uUi5lW7y1kAGs7NCP+ufL379B6Sc3JIkc=;\n\tb=PpNCayi0kdDILc2iF2/7nqL54eN/qdFbo+ebm/UmdyFTBA071KzTBJmpE1v4QvgRKA\n\tisdS/W8BZvGRVbKposp350UV3nfVQO2VkpdrvlqecEtl6RNqsClmFzvgUHLb7LLo52WZ\n\tt6WdEXIMS6l0dLuv7uCdwhFMyhbAp5vzXCfZbtprPbWzfB9Ug5uT6M9E5R5qgZnU2gDo\n\t4s2vzweyELPMoFAgva+xrRU0yJjhQjTGh/6ndf06VWdhwV4CCBchTvsPBrsehakkAMZI\n\tpUP46tXHSi6C4p2PcMjp2raV8IekoE8qdthHCjux7NSYwluci/GVwzhRNohTrjQpgvjq\n\tq/xQ==","X-Gm-Message-State":"APjAAAX+lQGesriq9JFCvpBsy9rcI4xbkLTVvLYtlxewV0fknEmbkwb/\n\tHSfLSWFbzORk1uWrSFwahWCavg==","X-Google-Smtp-Source":"APXvYqyrIW5ejzejWciq+y6d0DCDogURby3z1iM3zepEHxfd6yQEOrb4UY71b3ZEnumGWLIvohTFAw==","X-Received":"by 2002:a2e:5d46:: with SMTP id\n\tr67mr14818574ljb.187.1562024179326; \n\tMon, 01 Jul 2019 16:36:19 -0700 (PDT)","Date":"Tue, 2 Jul 2019 01:36:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190701233618.GB9228@bigcity.dyn.berto.se>","References":"<20190701232339.5191-1-laurent.pinchart@ideasonboard.com>\n\t<20190701232339.5191-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190701232339.5191-4-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: ipc: unix: Make\n\tsocket operation asynchronous","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 01 Jul 2019 23:36:20 -0000"}}]