[{"id":31724,"web_url":"https://patchwork.libcamera.org/comment/31724/","msgid":"<20241013155826.GD5212@pendragon.ideasonboard.com>","date":"2024-10-13T15:58:26","subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey, Han-Lin,\n\nThank you for the patch.\n\nOn Sat, Oct 12, 2024 at 12:47:57PM +0000, Harvey Yang wrote:\n> From: Han-Lin Chen \n> \n> Add method to set thread affinity to Thread class.\n> \n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/base/thread.h | 6 +++++\n> src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> 2 files changed, 53 insertions(+)\n> \n> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> index 4f33de63d..c74fe102b 100644\n> --- a/include/libcamera/base/thread.h\n> +++ b/include/libcamera/base/thread.h\n> @@ -35,6 +35,8 @@ public:\n> \tvoid exit(int code = 0);\n> \tbool wait(utils::duration duration = utils::duration::max());\n> \n> +\tvoid setThreadAffinity(const std::vector &cpus);\n> +\n> \tbool isRunning();\n> \n> \tSignal<> finished;\n> @@ -54,6 +56,8 @@ private:\n> \tvoid startThread();\n> \tvoid finishThread();\n> \n> +\tvoid setThreadAffinityInternal();\n> +\n> \tvoid postMessage(std::unique_ptr msg, Object *receiver);\n> \tvoid removeMessages(Object *receiver);\n> \n> @@ -67,6 +71,8 @@ private:\n> \n> \tstd::thread thread_;\n> \tThreadData *data_;\n> +\n> +\tstd::vector cpus_;\n\nYou can move this to ThreadData so it won't affect the API.\n\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 8735670b8..de5423eda 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -254,6 +254,8 @@ void Thread::start()\n> \tdata_->exit_.store(false, std::memory_order_relaxed);\n> \n> \tthread_ = std::thread(&Thread::startThread, this);\n> +\n> +\tsetThreadAffinityInternal();\n> }\n> \n> void Thread::startThread()\n> @@ -410,6 +412,51 @@ bool Thread::wait(utils::duration duration)\n> \treturn hasFinished;\n> }\n> \n> +/**\n> + * \\brief Set the CPU affinity mask of the thread\n> + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n\nAs there's error handling in the function, you should describe what\nhappens in case of errors.\n\n> + */\n> +void Thread::setThreadAffinity(const std::vector &cpus)\n\nMake it a span so the caller can also use an array instead of a vector.\n\n> +{\n> +\tcpus_.clear();\n> +\n> +\tconst unsigned int num_cpus = std::thread::hardware_concurrency();\n> +\tfor (const int &cpu : cpus) {\n> +\t\tif (cpus_.size() == CPU_SETSIZE) {\n> +\t\t\tLOG(Thread, Error) << \"cpus_ already contains \" << CPU_SETSIZE\n> +\t\t\t\t\t << \" cpus. Ignoring the rest.\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (cpu >= static_cast(num_cpus) || cpu < 0) {\n\nWhy is the argument to the function a vector of int if entries can't be\nnegative ? Can't it be a vector of unsigned int instead ? You'll be able\nto drop the cast here and drop the second check.\n\n> +\t\t\tLOG(Thread, Error) << \"Ignore an invalid cpu index: \" << cpu;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tcpus_.push_back(cpu);\n> +\t}\n> +\n> +\tMutexLocker locker(data_->mutex_);\n> +\tif (data_->running_)\n> +\t\tsetThreadAffinityInternal();\n> +}\n> +\n> +void Thread::setThreadAffinityInternal()\n> +{\n> +\tif (cpus_.empty())\n> +\t\treturn;\n> +\n> +\tauto handle = thread_.native_handle();\n> +\n> +\tcpu_set_t cpuset;\n> +\tCPU_ZERO(&cpuset);\n> +\n> +\tfor (auto cpu : cpus_)\n> +\t\tCPU_SET(cpu, &cpuset);\n\nCan't you compute the cpu_set_t and store it in the ThreadData class,\ninstead of storing a vector ? This can be done without holding the lock.\n\n> +\n> +\tpthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n\nsizeof(cpu_set_t)\n\n> +}\n> +\n> /**\n> * \\brief Check if the thread is running\n> *","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4E70FC32F4\n\tfor ;\n\tSun, 13 Oct 2024 15:58:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 295496537B;\n\tSun, 13 Oct 2024 17:58:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E3E46536C\n\tfor ;\n\tSun, 13 Oct 2024 17:58:35 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [23.233.251.139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33F204CE;\n\tSun, 13 Oct 2024 17:56:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"v+tGRezg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728835015;\n\tbh=sr+8t1EFEyvhR4a2I2zWO7emmSxTO1iwL6H1F3L48D4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v+tGRezg+9JnhTIpuvnhra+DM4rryM9X9e4WPoZk7QFV0NALLrAI5AtF0KOtxh0OJ\n\tWTjdVrsh6TPXvnoIoPYLZTP3pAqZm0HnMUW1tDr6yK7cHjLY9GNPa1Ojo9CiZMLjlH\n\tgufIx5M8g7a+MpnYBXX7P6Ug2tF1mQLCBTJAYZ0A=","Date":"Sun, 13 Oct 2024 18:58:26 +0300","From":"Laurent Pinchart ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","Message-ID":"<20241013155826.GD5212@pendragon.ideasonboard.com>","References":"<20241012124926.1049458-1-chenghaoyang@chromium.org>\n\t<20241012124926.1049458-2-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241012124926.1049458-2-chenghaoyang@chromium.org>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31727,"web_url":"https://patchwork.libcamera.org/comment/31727/","msgid":"","date":"2024-10-14T07:00:09","subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Sun, Oct 13, 2024 at 11:58 PM Laurent Pinchart\n wrote:\n>\n> Hi Harvey, Han-Lin,\n>\n> Thank you for the patch.\n>\n> On Sat, Oct 12, 2024 at 12:47:57PM +0000, Harvey Yang wrote:\n> > From: Han-Lin Chen \n> >\n> > Add method to set thread affinity to Thread class.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/base/thread.h | 6 +++++\n> > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > 2 files changed, 53 insertions(+)\n> >\n> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > index 4f33de63d..c74fe102b 100644\n> > --- a/include/libcamera/base/thread.h\n> > +++ b/include/libcamera/base/thread.h\n> > @@ -35,6 +35,8 @@ public:\n> > void exit(int code = 0);\n> > bool wait(utils::duration duration = utils::duration::max());\n> >\n> > + void setThreadAffinity(const std::vector &cpus);\n> > +\n> > bool isRunning();\n> >\n> > Signal<> finished;\n> > @@ -54,6 +56,8 @@ private:\n> > void startThread();\n> > void finishThread();\n> >\n> > + void setThreadAffinityInternal();\n> > +\n> > void postMessage(std::unique_ptr msg, Object *receiver);\n> > void removeMessages(Object *receiver);\n> >\n> > @@ -67,6 +71,8 @@ private:\n> >\n> > std::thread thread_;\n> > ThreadData *data_;\n> > +\n> > + std::vector cpus_;\n>\n> You can move this to ThreadData so it won't affect the API.\n\nGot it. Done.\n\n>\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index 8735670b8..de5423eda 100644\n> > --- a/src/libcamera/base/thread.cpp\n> > +++ b/src/libcamera/base/thread.cpp\n> > @@ -254,6 +254,8 @@ void Thread::start()\n> > data_->exit_.store(false, std::memory_order_relaxed);\n> >\n> > thread_ = std::thread(&Thread::startThread, this);\n> > +\n> > + setThreadAffinityInternal();\n> > }\n> >\n> > void Thread::startThread()\n> > @@ -410,6 +412,51 @@ bool Thread::wait(utils::duration duration)\n> > return hasFinished;\n> > }\n> >\n> > +/**\n> > + * \\brief Set the CPU affinity mask of the thread\n> > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n>\n> As there's error handling in the function, you should describe what\n> happens in case of errors.\n\nAdded description.\n\n>\n> > + */\n> > +void Thread::setThreadAffinity(const std::vector &cpus)\n>\n> Make it a span so the caller can also use an array instead of a vector.\n\nDone.\n\n>\n> > +{\n> > + cpus_.clear();\n> > +\n> > + const unsigned int num_cpus = std::thread::hardware_concurrency();\n> > + for (const int &cpu : cpus) {\n> > + if (cpus_.size() == CPU_SETSIZE) {\n> > + LOG(Thread, Error) << \"cpus_ already contains \" << CPU_SETSIZE\n> > + << \" cpus. Ignoring the rest.\";\n> > + break;\n> > + }\n> > +\n> > + if (cpu >= static_cast(num_cpus) || cpu < 0) {\n>\n> Why is the argument to the function a vector of int if entries can't be\n> negative ? Can't it be a vector of unsigned int instead ? You'll be able\n> to drop the cast here and drop the second check.\n\nRight, I thought CPU_SET only takes int, while I was wrong.\n\n>\n> > + LOG(Thread, Error) << \"Ignore an invalid cpu index: \" << cpu;\n> > + continue;\n> > + }\n> > +\n> > + cpus_.push_back(cpu);\n> > + }\n> > +\n> > + MutexLocker locker(data_->mutex_);\n> > + if (data_->running_)\n> > + setThreadAffinityInternal();\n> > +}\n> > +\n> > +void Thread::setThreadAffinityInternal()\n> > +{\n> > + if (cpus_.empty())\n> > + return;\n> > +\n> > + auto handle = thread_.native_handle();\n> > +\n> > + cpu_set_t cpuset;\n> > + CPU_ZERO(&cpuset);\n> > +\n> > + for (auto cpu : cpus_)\n> > + CPU_SET(cpu, &cpuset);\n>\n> Can't you compute the cpu_set_t and store it in the ThreadData class,\n> instead of storing a vector ? This can be done without holding the lock.\n\nDone, while I still think that it should only be updated when holding the\nlock, to avoid race conditions.\n\n>\n> > +\n> > + pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n>\n> sizeof(cpu_set_t)\n\nSorry, I don't get it: I think I already used `sizeof(cpu_set_t)`.\n\n\n>\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the thread is running\n> > *\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8613DC32F4\n\tfor ;\n\tMon, 14 Oct 2024 07:00:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F06765380;\n\tMon, 14 Oct 2024 09:00:24 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A97DC65374\n\tfor ;\n\tMon, 14 Oct 2024 09:00:21 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id\n\t38308e7fff4ca-2fac3f1287bso34445911fa.1\n\tfor ;\n\tMon, 14 Oct 2024 00:00:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"RjmZmS6x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728889221; x=1729494021;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ruRuEs3s5aGqTFZwI/ATqTF4DSqzObu4AiLYDKfViqM=;\n\tb=RjmZmS6xOHNUfj+2QwGRmwkjK2Q5QMhp8XnT5EG2UZM/REkHA3odF5woeyLS+K/KQb\n\tHys/6oucPtEOQuLfAxqHKsQYNzEpddWSf8RpTv8BZCWDzMX5JQMV/YnhdWKSb57uokeX\n\tfwJN2StYAlFzPLteq0na2phY/E9w0ZIgUfNp4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728889221; x=1729494021;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=ruRuEs3s5aGqTFZwI/ATqTF4DSqzObu4AiLYDKfViqM=;\n\tb=ZwYMQ3C1b/OlGL4xFvYQz0/Q0hxelfGhm+lEuUdaPu+F2ELwDVDMc06oJXMYAxuepN\n\tdoM9YiSMZmWAMfj7ljlMbAB2PZGOEv62tyA1ujNVVsCicKGnzlT1MNJiG/vaBGrTUZN8\n\tapwqr4acovCXhjSzt4sfwGLhhFcZw0my3QbY5mTJ2k+cmzJ2zrMbzWCjYf3lAknD0ioE\n\tR1xtSSGeJLKlN8JO0MsHTfI8z+U3rB8wjJwsm1/oOdnKpx72DxvnDRPMh+gS43Rnqoow\n\tN5cYsvUpuujhZvXLMuDlnUjNeRbWgaTIkfj1IBsAWa+8icIU6L6P2KSYZUs2eZXfplUG\n\tFA6A==","X-Gm-Message-State":"AOJu0YyTUVNBDEIfbdxHorC0YmjHYTlZVWb0JcSR10Y2Akrj14YKCjmk\n\tqIctjg890BYtc9VUXQpJJ9hhdEUVjb0UIvQylC/gr273VCtQ/6OFBX7c5RcLPUtFPfbQq+L2C9s\n\tGiCPPOdAIK3bhcpJ8U9NnxUx9/rxceZiQAddR","X-Google-Smtp-Source":"AGHT+IHW3dizbu+GQG+grixh6LKT5lRIPcROq2jo2Rue8h1A9QMFqWlKYhpQObiddKyQTS4y3REZ0rytnI8vWwxrheQ=","X-Received":"by 2002:a2e:a202:0:b0:2fa:cdd1:4f16 with SMTP id\n\t38308e7fff4ca-2fb3f196d70mr19244381fa.14.1728889220684;\n\tMon, 14 Oct 2024 00:00:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20241012124926.1049458-1-chenghaoyang@chromium.org>\n\t<20241012124926.1049458-2-chenghaoyang@chromium.org>\n\t<20241013155826.GD5212@pendragon.ideasonboard.com>","In-Reply-To":"<20241013155826.GD5212@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 14 Oct 2024 15:00:09 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31739,"web_url":"https://patchwork.libcamera.org/comment/31739/","msgid":"<20241014203931.GH5522@pendragon.ideasonboard.com>","date":"2024-10-14T20:39:31","subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Oct 14, 2024 at 03:00:09PM +0800, Cheng-Hao Yang wrote:\n> On Sun, Oct 13, 2024 at 11:58 PM Laurent Pinchart wrote:\n> > On Sat, Oct 12, 2024 at 12:47:57PM +0000, Harvey Yang wrote:\n> > > From: Han-Lin Chen \n> > >\n> > > Add method to set thread affinity to Thread class.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > include/libcamera/base/thread.h | 6 +++++\n> > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > > 2 files changed, 53 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > > index 4f33de63d..c74fe102b 100644\n> > > --- a/include/libcamera/base/thread.h\n> > > +++ b/include/libcamera/base/thread.h\n> > > @@ -35,6 +35,8 @@ public:\n> > > void exit(int code = 0);\n> > > bool wait(utils::duration duration = utils::duration::max());\n> > >\n> > > + void setThreadAffinity(const std::vector &cpus);\n> > > +\n> > > bool isRunning();\n> > >\n> > > Signal<> finished;\n> > > @@ -54,6 +56,8 @@ private:\n> > > void startThread();\n> > > void finishThread();\n> > >\n> > > + void setThreadAffinityInternal();\n> > > +\n> > > void postMessage(std::unique_ptr msg, Object *receiver);\n> > > void removeMessages(Object *receiver);\n> > >\n> > > @@ -67,6 +71,8 @@ private:\n> > >\n> > > std::thread thread_;\n> > > ThreadData *data_;\n> > > +\n> > > + std::vector cpus_;\n> >\n> > You can move this to ThreadData so it won't affect the API.\n> \n> Got it. Done.\n> \n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > index 8735670b8..de5423eda 100644\n> > > --- a/src/libcamera/base/thread.cpp\n> > > +++ b/src/libcamera/base/thread.cpp\n> > > @@ -254,6 +254,8 @@ void Thread::start()\n> > > data_->exit_.store(false, std::memory_order_relaxed);\n> > >\n> > > thread_ = std::thread(&Thread::startThread, this);\n> > > +\n> > > + setThreadAffinityInternal();\n> > > }\n> > >\n> > > void Thread::startThread()\n> > > @@ -410,6 +412,51 @@ bool Thread::wait(utils::duration duration)\n> > > return hasFinished;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Set the CPU affinity mask of the thread\n> > > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> >\n> > As there's error handling in the function, you should describe what\n> > happens in case of errors.\n> \n> Added description.\n> \n> > > + */\n> > > +void Thread::setThreadAffinity(const std::vector &cpus)\n> >\n> > Make it a span so the caller can also use an array instead of a vector.\n> \n> Done.\n> \n> > > +{\n> > > + cpus_.clear();\n> > > +\n> > > + const unsigned int num_cpus = std::thread::hardware_concurrency();\n> > > + for (const int &cpu : cpus) {\n> > > + if (cpus_.size() == CPU_SETSIZE) {\n> > > + LOG(Thread, Error) << \"cpus_ already contains \" << CPU_SETSIZE\n> > > + << \" cpus. Ignoring the rest.\";\n> > > + break;\n> > > + }\n> > > +\n> > > + if (cpu >= static_cast(num_cpus) || cpu < 0) {\n> >\n> > Why is the argument to the function a vector of int if entries can't be\n> > negative ? Can't it be a vector of unsigned int instead ? You'll be able\n> > to drop the cast here and drop the second check.\n> \n> Right, I thought CPU_SET only takes int, while I was wrong.\n> \n> > > + LOG(Thread, Error) << \"Ignore an invalid cpu index: \" << cpu;\n> > > + continue;\n> > > + }\n> > > +\n> > > + cpus_.push_back(cpu);\n> > > + }\n> > > +\n> > > + MutexLocker locker(data_->mutex_);\n> > > + if (data_->running_)\n> > > + setThreadAffinityInternal();\n> > > +}\n> > > +\n> > > +void Thread::setThreadAffinityInternal()\n> > > +{\n> > > + if (cpus_.empty())\n> > > + return;\n> > > +\n> > > + auto handle = thread_.native_handle();\n> > > +\n> > > + cpu_set_t cpuset;\n> > > + CPU_ZERO(&cpuset);\n> > > +\n> > > + for (auto cpu : cpus_)\n> > > + CPU_SET(cpu, &cpuset);\n> >\n> > Can't you compute the cpu_set_t and store it in the ThreadData class,\n> > instead of storing a vector ? This can be done without holding the lock.\n> \n> Done, while I still think that it should only be updated when holding the\n> lock, to avoid race conditions.\n\nTrue. We could compute a local cpuset variable and then copy it to\ncpuset_, but that's probably overkill. In practice I expect the vector\nto contain a small number of elements.\n\n> > > +\n> > > + pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n> >\n> > sizeof(cpu_set_t)\n> \n> Sorry, I don't get it: I think I already used `sizeof(cpu_set_t)`.\n\nSorry, I meant sizeof(cpuset).\n\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Check if the thread is running\n> > > *","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 313D5C326C\n\tfor ;\n\tMon, 14 Oct 2024 20:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1195065381;\n\tMon, 14 Oct 2024 22:39:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F2096537C\n\tfor ;\n\tMon, 14 Oct 2024 22:39:40 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [23.233.251.139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 105DB667;\n\tMon, 14 Oct 2024 22:37:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"boN7q4i6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728938279;\n\tbh=kqpGiDfezeL7w656va7bw1eUxKYrpyGXDDhoZfVy8hE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=boN7q4i6rO5b8Frfwn6fy/vg1D1UHQv5bepaFYkldSAPzlw7AZLhd+W8pjVhvc3NR\n\txGuen+M5nAAEFM57j+d+/XfhqmleyAOx7nniRreZY6bJ6II8tjqF5WWG/XDWRSTtRZ\n\tldhHhNvaV1n5lMXLgcUX1mcNciKMYe53y/MVaRYs=","Date":"Mon, 14 Oct 2024 23:39:31 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","Message-ID":"<20241014203931.GH5522@pendragon.ideasonboard.com>","References":"<20241012124926.1049458-1-chenghaoyang@chromium.org>\n\t<20241012124926.1049458-2-chenghaoyang@chromium.org>\n\t<20241013155826.GD5212@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31740,"web_url":"https://patchwork.libcamera.org/comment/31740/","msgid":"","date":"2024-10-15T06:55:49","subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 15, 2024 at 4:39 AM Laurent Pinchart\n wrote:\n>\n> On Mon, Oct 14, 2024 at 03:00:09PM +0800, Cheng-Hao Yang wrote:\n> > On Sun, Oct 13, 2024 at 11:58 PM Laurent Pinchart wrote:\n> > > On Sat, Oct 12, 2024 at 12:47:57PM +0000, Harvey Yang wrote:\n> > > > From: Han-Lin Chen \n> > > >\n> > > > Add method to set thread affinity to Thread class.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > include/libcamera/base/thread.h | 6 +++++\n> > > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > > > 2 files changed, 53 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > > > index 4f33de63d..c74fe102b 100644\n> > > > --- a/include/libcamera/base/thread.h\n> > > > +++ b/include/libcamera/base/thread.h\n> > > > @@ -35,6 +35,8 @@ public:\n> > > > void exit(int code = 0);\n> > > > bool wait(utils::duration duration = utils::duration::max());\n> > > >\n> > > > + void setThreadAffinity(const std::vector &cpus);\n> > > > +\n> > > > bool isRunning();\n> > > >\n> > > > Signal<> finished;\n> > > > @@ -54,6 +56,8 @@ private:\n> > > > void startThread();\n> > > > void finishThread();\n> > > >\n> > > > + void setThreadAffinityInternal();\n> > > > +\n> > > > void postMessage(std::unique_ptr msg, Object *receiver);\n> > > > void removeMessages(Object *receiver);\n> > > >\n> > > > @@ -67,6 +71,8 @@ private:\n> > > >\n> > > > std::thread thread_;\n> > > > ThreadData *data_;\n> > > > +\n> > > > + std::vector cpus_;\n> > >\n> > > You can move this to ThreadData so it won't affect the API.\n> >\n> > Got it. Done.\n> >\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > > index 8735670b8..de5423eda 100644\n> > > > --- a/src/libcamera/base/thread.cpp\n> > > > +++ b/src/libcamera/base/thread.cpp\n> > > > @@ -254,6 +254,8 @@ void Thread::start()\n> > > > data_->exit_.store(false, std::memory_order_relaxed);\n> > > >\n> > > > thread_ = std::thread(&Thread::startThread, this);\n> > > > +\n> > > > + setThreadAffinityInternal();\n> > > > }\n> > > >\n> > > > void Thread::startThread()\n> > > > @@ -410,6 +412,51 @@ bool Thread::wait(utils::duration duration)\n> > > > return hasFinished;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the CPU affinity mask of the thread\n> > > > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> > >\n> > > As there's error handling in the function, you should describe what\n> > > happens in case of errors.\n> >\n> > Added description.\n> >\n> > > > + */\n> > > > +void Thread::setThreadAffinity(const std::vector &cpus)\n> > >\n> > > Make it a span so the caller can also use an array instead of a vector.\n> >\n> > Done.\n> >\n> > > > +{\n> > > > + cpus_.clear();\n> > > > +\n> > > > + const unsigned int num_cpus = std::thread::hardware_concurrency();\n> > > > + for (const int &cpu : cpus) {\n> > > > + if (cpus_.size() == CPU_SETSIZE) {\n> > > > + LOG(Thread, Error) << \"cpus_ already contains \" << CPU_SETSIZE\n> > > > + << \" cpus. Ignoring the rest.\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + if (cpu >= static_cast(num_cpus) || cpu < 0) {\n> > >\n> > > Why is the argument to the function a vector of int if entries can't be\n> > > negative ? Can't it be a vector of unsigned int instead ? You'll be able\n> > > to drop the cast here and drop the second check.\n> >\n> > Right, I thought CPU_SET only takes int, while I was wrong.\n> >\n> > > > + LOG(Thread, Error) << \"Ignore an invalid cpu index: \" << cpu;\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + cpus_.push_back(cpu);\n> > > > + }\n> > > > +\n> > > > + MutexLocker locker(data_->mutex_);\n> > > > + if (data_->running_)\n> > > > + setThreadAffinityInternal();\n> > > > +}\n> > > > +\n> > > > +void Thread::setThreadAffinityInternal()\n> > > > +{\n> > > > + if (cpus_.empty())\n> > > > + return;\n> > > > +\n> > > > + auto handle = thread_.native_handle();\n> > > > +\n> > > > + cpu_set_t cpuset;\n> > > > + CPU_ZERO(&cpuset);\n> > > > +\n> > > > + for (auto cpu : cpus_)\n> > > > + CPU_SET(cpu, &cpuset);\n> > >\n> > > Can't you compute the cpu_set_t and store it in the ThreadData class,\n> > > instead of storing a vector ? This can be done without holding the lock.\n> >\n> > Done, while I still think that it should only be updated when holding the\n> > lock, to avoid race conditions.\n>\n> True. We could compute a local cpuset variable and then copy it to\n> cpuset_, but that's probably overkill. In practice I expect the vector\n> to contain a small number of elements.\n\nYeah I considered that, while I found that CPU_COPY is not in the\nstandard library. Let's keep it as is for now.\n\n>\n> > > > +\n> > > > + pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n> > >\n> > > sizeof(cpu_set_t)\n> >\n> > Sorry, I don't get it: I think I already used `sizeof(cpu_set_t)`.\n>\n> Sorry, I meant sizeof(cpuset).\n\nDone\n\nBR,\nHarvey\n\n\n>\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Check if the thread is running\n> > > > *\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 03A27C0F2A\n\tfor ;\n\tTue, 15 Oct 2024 06:56:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A09DC65389;\n\tTue, 15 Oct 2024 08:56:03 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 329DE65381\n\tfor ;\n\tTue, 15 Oct 2024 08:56:01 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id\n\t38308e7fff4ca-2fb57f97d75so12580271fa.2\n\tfor ;\n\tMon, 14 Oct 2024 23:56:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fRXXK67o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728975360; x=1729580160;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=9u6+OUNr8zzAQSTon2GUrkOePbxp515SWF6bqEL+ABM=;\n\tb=fRXXK67o5tBoRarEPSdEY6PNSYy1y35Cxg2ztf0tZiINBJmjJddZjdnq2Uqd0sHoeA\n\tAsC60JQtBI96wC+Wk0N++uCgXU21FUL/iPY+Qerl5pd/CkOOYPTn3FcHmeusXntLCo6T\n\tXcHhechc4TnTCKUQ4Sffu4MFw06Gy5JkStv1Y=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728975360; x=1729580160;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=9u6+OUNr8zzAQSTon2GUrkOePbxp515SWF6bqEL+ABM=;\n\tb=ueoycRcsthntqWO+Kzyp9S7zbG6sLgqQnY0xCG9haaQ9ftz0RkZPvdgrn+zORKlJ10\n\tmK4MGXiG5r7esrMFygd1UwdTF4f0Kqs4D239IyVymNTqTt8UIpIRjeL/zeuRlHGvTDuc\n\t3RBDLbopCHApsoeA7I+7qPAcAm4C/6uqYtmJQubGWKeiPJBDBd8h2mhfETL4rOWfuoDv\n\tCUg6jh9oYNyaCH/BQ4R4nM/3/xDtUFv+zRP29tUbbkEicFykTj1pZczKz4Nn3slz9sWm\n\tZhsZR5Fh0qOG1C5gkwngc9WziXJCIBYtOVds1mdzjwtBPvPq0qjAPMBajIcx+Sjx8jNP\n\tB83Q==","X-Gm-Message-State":"AOJu0Ywol2MtG+8DIHwqWHhPMkVPiAiiXxlN0H4W3pkoUdYvNmvuDJti\n\tKD0jnOHRhj2qKZpTuy9zdcTm4E4eTJS31fxYjIpotkS9xCVcB8/h8ngnJaUI2O7gs6GEoI8gpLY\n\ty/oIOTK4WY3zU/Dl7XWO7e9WyQt9pWALlkqe0","X-Google-Smtp-Source":"AGHT+IEMYw6nFPF8tF7gdqUEoHNqzNLJwC5bTQLThJBf7azyntrFYBEeNO/OwyUXZEo13ilDcIP9//HPXL+eDMYdvyI=","X-Received":"by 2002:a2e:a98a:0:b0:2fb:3a78:190a with SMTP id\n\t38308e7fff4ca-2fb3a781aafmr61597411fa.29.1728975360338;\n\tMon, 14 Oct 2024 23:56:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20241012124926.1049458-1-chenghaoyang@chromium.org>\n\t<20241012124926.1049458-2-chenghaoyang@chromium.org>\n\t<20241013155826.GD5212@pendragon.ideasonboard.com>\n\t\n\t<20241014203931.GH5522@pendragon.ideasonboard.com>","In-Reply-To":"<20241014203931.GH5522@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Tue, 15 Oct 2024 14:55:49 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/1] libcamera: add method to set thread affinity","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]