[{"id":31719,"web_url":"https://patchwork.libcamera.org/comment/31719/","msgid":"<172872596648.877857.5648117250875770@ping.linuxembedded.co.uk>","date":"2024-10-12T09:39:26","subject":"Re: [PATCH 1/1] libcamera: add method to set thread affinity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-10-11 18:00:48)\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 | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 37 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> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 8735670b8..2417043a3 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,35 @@ 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> +void Thread::setThreadAffinity(const std::vector &cpus)\n> +{\n> + cpus_ = cpus;\n> +\n\nShould there be any checks here to verify the CPU list is valid? What\nhappens if I try to call Thread::setThreadAffinity(-1, 1000000) ?\n\nPerhaps it should also ensure cpus.size() is never bigger than\nCPU_SETSIZE?\n\nThe asynchronous nature here means we won't know if the call to\npthread_setaffinity_np() is successful until the thread is started, but\nis there anything we can do to validate the input as this is extending\nthe Thread API? \n\nI'm not sure if there is anything worthwhile yet, but is there anything\naround this that should then be added to the unit tests?\n\n\nI know all of the above is really unlikely to be called incorrectly in\nthe code you'll add to use these - but we should treat libcamera-base as\nif it were an entirely separate helper library so we shouldn't skimp on\nlayer checks and validation.\n\n\n> + MutexLocker locker(data_->mutex_);\n> + if (data_->running_)\n\nI'ts equivalent, so it's fine like the above, but there's also the\nhelper which could make this:\n\n\tif (isRunning())\n\nBut both are fine with me so either way.\n\n--\nKieran\n\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> + pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n> +}\n> +\n> /**\n> * \\brief Check if the thread is running\n> *\n> -- \n> 2.47.0.rc1.288.g06298d1525-goog\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 17C7FC32F2\n\tfor ;\n\tSat, 12 Oct 2024 09:39:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F330865373;\n\tSat, 12 Oct 2024 11:39:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C591263525\n\tfor ;\n\tSat, 12 Oct 2024 11:39:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 487DE63D;\n\tSat, 12 Oct 2024 11:37:50 +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=\"Fv5Ucm8E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728725870;\n\tbh=8ddOXsMira/gJMtgo8mJn/F6WGvBYYmuBpzA+i/gY44=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Fv5Ucm8ExCvYI/QRAm2JeoggpasE7mb9PGpkLY/80o4n0vrMtUKuqLpiObxPwHkNB\n\tWEwa6kaWXXmmtqFH0FMIJeIV0a+F0bs4dy1zAMCoW9KNpd9HtVMBJu/V9lI0wyTYYq\n\tsnoUTiaB0wH0REeMlyNAiT65rx8YFOaxsoOtd5Lc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241011171930.3225255-2-chenghaoyang@chromium.org>","References":"<20241011171930.3225255-1-chenghaoyang@chromium.org>\n\t<20241011171930.3225255-2-chenghaoyang@chromium.org>","Subject":"Re: [PATCH 1/1] libcamera: add method to set thread affinity","From":"Kieran Bingham ","Cc":"Han-Lin Chen ,\n\tHarvey Yang ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 12 Oct 2024 10:39:26 +0100","Message-ID":"<172872596648.877857.5648117250875770@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31720,"web_url":"https://patchwork.libcamera.org/comment/31720/","msgid":"","date":"2024-10-12T12:49:35","subject":"Re: [PATCH 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 Kieran,\n\nOn Sat, Oct 12, 2024 at 5:39 PM Kieran Bingham\n wrote:\n>\n> Quoting Harvey Yang (2024-10-11 18:00:48)\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 | 31 +++++++++++++++++++++++++++++++\n> > 2 files changed, 37 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> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index 8735670b8..2417043a3 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,35 @@ 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> > +void Thread::setThreadAffinity(const std::vector &cpus)\n> > +{\n> > + cpus_ = cpus;\n> > +\n>\n> Should there be any checks here to verify the CPU list is valid? What\n> happens if I try to call Thread::setThreadAffinity(-1, 1000000) ?\n\nAdded checks to ensure the value is within [0,\n`std::thread::hardware_concurrency()`).\n\n>\n> Perhaps it should also ensure cpus.size() is never bigger than\n> CPU_SETSIZE?\n\nAdded logic to ignore the rest of cpu indices when `cpus_` is full.\n\n>\n> The asynchronous nature here means we won't know if the call to\n> pthread_setaffinity_np() is successful until the thread is started, but\n> is there anything we can do to validate the input as this is extending\n> the Thread API?\n>\n> I'm not sure if there is anything worthwhile yet, but is there anything\n> around this that should then be added to the unit tests?\n\nShould we add return values or getter functions in Thread class to\nallow checking the results in unit tests?\n\n>\n>\n> I know all of the above is really unlikely to be called incorrectly in\n> the code you'll add to use these - but we should treat libcamera-base as\n> if it were an entirely separate helper library so we shouldn't skimp on\n> layer checks and validation.\n>\n>\n> > + MutexLocker locker(data_->mutex_);\n> > + if (data_->running_)\n>\n> I'ts equivalent, so it's fine like the above, but there's also the\n> helper which could make this:\n>\n> if (isRunning())\n>\n> But both are fine with me so either way.\n>\n\n\nActually it's a bit different, as we call\n`Thread::setThreadAffinityInternal` within the lock. Using\n`Thread::isRunning` is more complicated IIUC to achieve the\nsame logic. So let's keep it this way :)\n\nBR,\nHarvey\n\n\n\n\n> --\n> Kieran\n>\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> > + pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the thread is running\n> > *\n> > --\n> > 2.47.0.rc1.288.g06298d1525-goog\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 4B1C0C326B\n\tfor ;\n\tSat, 12 Oct 2024 12:49:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 089226537F;\n\tSat, 12 Oct 2024 14:49:50 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCDAF6537A\n\tfor ;\n\tSat, 12 Oct 2024 14:49:47 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2fb3110b964so18771081fa.1\n\tfor ;\n\tSat, 12 Oct 2024 05:49:47 -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=\"oHGdQPRS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728737387; x=1729342187;\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=ulibTAcIRwKyZyEmJM0UkvK1RHq2MB8zGDRZNXv3rqs=;\n\tb=oHGdQPRSrlkXsJmHVUblHN3tAvWw/DH/LpR1VyRKhuYq22aHG+sya25qVlbNxfoV3Y\n\tvgjcztOW9x/mntVcPhjO7Xpjyxf/VTGK1d9LiKfEsQnRizznzURD0MaJA6+lzBK6ZWLq\n\ted7G5L99CFSAI5mf1CF9LFhRzvhhzlrDyOUGQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728737387; x=1729342187;\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=ulibTAcIRwKyZyEmJM0UkvK1RHq2MB8zGDRZNXv3rqs=;\n\tb=J8oUY2WTHsGlkgapLHn5GgYCg3XSvl+XxgEUTVi+onfHJ2kb1xXCdrZwBJw2POOzJ1\n\tIc16K+bGM1OIDsYOp6woVq1iRYyyZN6uAgYzV6+RtsaHIFmxjZ6Eje3avy3Q+jHvxqbq\n\tp269T9tkpzqUZ4WhZssw8p89f1m3ja7gXSAx8k79AG9FWOf029FJMLmec81FGUfFas0f\n\tKckuyilwCYhsQ8O+xzxoeeOlYNDSMA/nMlw+ASsL4/YiRKIgvkDQ9PZQIaCXeiGrnFjQ\n\t1n2EQUVpToe/x2PQ8sseoIqKyKk974zOc+w55Q8K4HjnDeuadUwLwbqgNc7LvxV6m+i3\n\t9MEg==","X-Gm-Message-State":"AOJu0YwHz4hMWsPbHmagjSHN3cAqH4FA0nggnux1B11VdgDw1+BdIn0B\n\tqGUzICuSg8AyZzl/WQB4BlTNd5GJNtRmo5RrrMT462jN7PZRMukzbOaLMwP1Uoz1uuWs2uEvKFw\n\tatW/TBgw+Zso31OmEZqD/ARpjHv47Vq2ZO6hk","X-Google-Smtp-Source":"AGHT+IEVkFV62Q6MoGHmjpAdFhzVfMemvHTRNm6ZnbBl5wOiidmMOwdMYvpjwsrsK2OpglzfqKZe3YEfkXYZcq+Zvrw=","X-Received":"by 2002:a2e:8891:0:b0:2f3:eeab:7f17 with SMTP id\n\t38308e7fff4ca-2fb3f30d16fmr7924101fa.41.1728737386887;\n\tSat, 12 Oct 2024 05:49:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20241011171930.3225255-1-chenghaoyang@chromium.org>\n\t<20241011171930.3225255-2-chenghaoyang@chromium.org>\n\t<172872596648.877857.5648117250875770@ping.linuxembedded.co.uk>","In-Reply-To":"<172872596648.877857.5648117250875770@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Sat, 12 Oct 2024 20:49:35 +0800","Message-ID":"","Subject":"Re: [PATCH 1/1] libcamera: add method to set thread affinity","To":"Kieran Bingham ","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\" "}}]