[PATCH ncns v1 0/2] Small optimization and improve debugging experience

[PATCH ncns v1 0/2] Small optimization and improve debugging experience

[Ammar Faizi]

Hi,

There are two patches in this series. The first one is just a small
optimization and the second one is a debugging experience improvement.

# Patch 1
chnet: Optimize put_thread() path 

The `delete` call doesn't have to be protected by mutex. Move it
to the outside of the locked region to reduce the `put_thread()`
critical section time.

# Patch 2
chnet: Add thread index specifier for chromium thread

Currently, all threads created by the chromium thread pool have task
comm name "chromium_thread". This naming is not helpful when we're
debugging multiple chromium threads. Set the name to "chromium-%u"
where %u is the thread index in the pool. It creates a way to
identify each chromium thread uniquely.

Please review!

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  chnet: Optimize `put_thread()` path
  chnet: Add thread index specifier for chromium thread

 chnet/chnet.cc | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

-- 
Ammar Faizi

[PATCH ncns v1 1/2] chnet: Optimize `put_thread()` path

[Ammar Faizi]

The `delete` call doesn't have to be protected by mutex. Move it
to the outside of the locked region to reduce the `put_thread()`
critical section time.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 chnet/chnet.cc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index e6b4c64..ba3c076 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -746,10 +746,13 @@ static void put_thread(void *thread)
 	if (--th->ref_count_ == 0) {
 		if (g_thpool)
 			g_thpool[th->idx_] = nullptr;
-
-		delete th;
+	} else {
+		th = nullptr;
 	}
 	g_thpool_lock_.unlock();
+
+	if (th)
+		delete th;
 }
 
 static void init_g_ch_thpool(void)
-- 
Ammar Faizi

[PATCH ncns v1 2/2] chnet: Add thread index specifier for chromium thread

[Ammar Faizi]

Currently, all threads created by the chromium thread pool have task
comm name "chromium_thread". This naming is not helpful when we're
debugging multiple chromium threads. Set the name to "chromium-%u"
where %u is the thread index in the pool. It creates a way to
identify each chromium thread uniquely.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 chnet/chnet.cc | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index ba3c076..32078cc 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -664,11 +664,20 @@ public:
 	uint32_t		ref_count_;
 	uint32_t		idx_;
 
-	ch_thpool(void);
+	ch_thpool(uint32_t i);
+
+private:
+	inline static std::string gen_thread_name(uint32_t i)
+	{
+		char name[sizeof("chromium-xxxxxxxx")];
+
+		snprintf(name, sizeof(name), "chromium-%u", i);
+		return std::string(name);
+	}
 };
 
-ch_thpool::ch_thpool(void):
-	thread_("chromium_thread"),
+ch_thpool::ch_thpool(uint32_t i):
+	thread_(gen_thread_name(i)),
 	ref_count_(0)
 {
 	CHECK((void *)this == (void *)&thread_);
@@ -698,7 +707,7 @@ static base::Thread *get_thread(void)
 
 	tmp = thp[0];
 	if (!tmp) {
-		ret = new struct ch_thpool;
+		ret = new struct ch_thpool(0);
 		ret->idx_ = 0;
 		thp[0] = ret;
 		goto out;
@@ -711,7 +720,7 @@ static base::Thread *get_thread(void)
 
 		tmp = thp[i];
 		if (!tmp) {
-			ret = new struct ch_thpool;
+			ret = new struct ch_thpool(i);
 			ret->idx_ = i;
 			thp[i] = ret;
 			goto out;
-- 
Ammar Faizi

Re: [PATCH ncns v1 0/2] Small optimization and improve debugging experience

[Alviro Iskandar Setiawan]

On Wed, Oct 19, 2022 at 11:43 PM Ammar Faizi wrote:
> # Patch 1
> chnet: Optimize put_thread() path
>
> The `delete` call doesn't have to be protected by mutex. Move it
> to the outside of the locked region to reduce the `put_thread()`
> critical section time.

or we shouldn't delete it at all?

-- Viro

Re: [PATCH ncns v1 2/2] chnet: Add thread index specifier for chromium thread

[Alviro Iskandar Setiawan]

On Wed, Oct 19, 2022 at 11:43 PM Ammar Faizi wrote:
> Currently, all threads created by the chromium thread pool have task
> comm name "chromium_thread". This naming is not helpful when we're
> debugging multiple chromium threads. Set the name to "chromium-%u"
> where %u is the thread index in the pool. It creates a way to
> identify each chromium thread uniquely.
>
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>

Acked-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>

tq

-- Viro

Re: [PATCH ncns v1 0/2] Small optimization and improve debugging experience

[Ammar Faizi]

On 10/20/22 5:36 AM, Alviro Iskandar Setiawan wrote:
> or we shouldn't delete it at all?

We should delete it when the ref count reaches zero for
the memory reclamation guarantee. We don't want to keep
eating memory while not using the allocated thread pool.

-- 
Ammar Faizi

Re: [PATCH ncns v1 0/2] Small optimization and improve debugging experience

[Alviro Iskandar Setiawan]

On Thu, Oct 20, 2022 at 5:53 AM Ammar Faizi wrote:
> On 10/20/22 5:36 AM, Alviro Iskandar Setiawan wrote:
> > or we shouldn't delete it at all?
>
> We should delete it when the ref count reaches zero for
> the memory reclamation guarantee. We don't want to keep
> eating memory while not using the allocated thread pool.

ic ic

Acked-by: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>

tq

-- Viro