[RFC PATCH v1 0/2] Fixed number of chromium workers

[RFC PATCH v1 0/2] Fixed number of chromium workers

[Ammar Faizi]

Hi Alviro,

Currently, a single chnet instance uses a single dedicated chromium
thread worker to perform an HTTP request. This doesn't scale well
because we need to spawn a new thread for each HTTP request.

Performing 4096 HTTP requests in parallel will spawn 4096 chromium
thread workers, which is too expensive and consuming too much
memory.

A single chromium thread worker can handle multiple HTTP requests.
This series creates a fixed number of chromium workers with ref
count to spread the jobs fairly across the chromium thread workers.

This greatly reduces the context switches and improve the performance.
It also greatly reduces the memory usage.

Implementation:

1) At initialization, when chnet_global_init() is called, create an
   array of pointers to `struct ch_thpool` and initialize those
   pointers to null. The number of elements in the array is
   taken from `std::thread::hardware_concurrency() - 1`.

2) When a new CHNet instance is created, its constructor calls
   `get_thread()` function which will initialize the array of
   pointers to `struct ch_thpool` if needed, then increment
   the ref count and it returns a pointer to the base::Thread
   class in `struct ch_thpool`.

3) When a CHNet instance is destroyed, it calls `put_thread()`
   function that will decrement the ref count of the
   `struct ch_thpool` and delete the object if the ref count
   reaches zero after getting decremented.


[ The implementation detail that fairly spread the job
  across multiple chromium threads worker is in get_thread()
  function, please have a look. ]

For the ring test case, the gained speedup is 33%.

Without this series:

  ammarfaizi2@integral2:~/work/ncns$ time taskset -c 0-7 make -j8 test -s
  Running /home/ammarfaizi2/work/ncns/tests/cpp/ring.t

  real    0m28.184s
  user    0m52.368s
  sys     0m27.582s


With this series:

  ammarfaizi2@integral2:~/work/ncns$ time taskset -c 0-7 make -j8 test -s
  Running /home/ammarfaizi2/work/ncns/tests/cpp/ring.t

  real    0m18.657s
  user    0m35.452s
  sys     0m2.146s

Please review and comment!

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

Ammar Faizi (2):
  chnet: Prepare global struct ch_thpool array
  chnet: Implement `get_thread()` and `put_thread()` function

 chnet/chnet.cc | 128 ++++++++++++++++++++++++++++++++++++++++++++++++-
 chnet/chnet.h  |   2 +-
 2 files changed, 128 insertions(+), 2 deletions(-)


base-commit: e1123a1e7b9526e4b12356bfed222386d4b00a80
-- 
Ammar Faizi

[RFC PATCH v1 1/2] chnet: Prepare global struct ch_thpool array

[Ammar Faizi]

This is a preparation patch to add "fixed number of chromium workers".
Initialize array of pointers `struct ch_thpool` with size
`std::thread::hardware_concurrency() - 1`.

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

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index 155dba8..8662374 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -607,6 +607,51 @@ const char *CHNet::read_buf(void)
 	return ch_->read_buf();
 }
 
+struct ch_thpool {
+public:
+	base::Thread		thread_;
+	uint32_t		ref_count_;
+	uint32_t		idx_;
+
+	ch_thpool(void);
+};
+
+ch_thpool::ch_thpool(void):
+	thread_("chromium_thread"),
+	ref_count_(0)
+{
+	CHECK((void *)this == (void *)&thread_);
+	base::Thread::Options options(base::MessagePumpType::IO, 0);
+	CHECK(thread_.StartWithOptions(std::move(options)));
+}
+
+static uint32_t g_max_ch_thpool;
+static std::mutex g_thpool_lock_;
+static struct ch_thpool **g_thpool;
+
+static void init_g_ch_thpool(void)
+{
+	struct ch_thpool **tmp;
+
+	g_max_ch_thpool = std::thread::hardware_concurrency() - 1;
+	if (g_max_ch_thpool < 1)
+		g_max_ch_thpool = 1;
+
+	tmp = (struct ch_thpool **)calloc(g_max_ch_thpool, sizeof(*tmp));
+	g_thpool_lock_.lock();
+	free(g_thpool);
+	g_thpool = tmp;
+	g_thpool_lock_.unlock();
+}
+
+static void destroy_g_ch_thpool(void)
+{
+	g_thpool_lock_.lock();
+	free(g_thpool);
+	g_thpool = nullptr;
+	g_thpool_lock_.unlock();
+}
+
 extern "C" {
 
 static base::AtExitManager *at_exit_manager;
@@ -641,6 +686,7 @@ __cold void chnet_global_init(void)
 	chnet_fix_chromium_boringssl();
 	SSL_library_init();
 	__chnet_global_init();
+	init_g_ch_thpool();
 	has_initialized = true;
 	init_lock.unlock();
 }
@@ -653,6 +699,7 @@ __cold void chnet_global_stop(void)
 	delete cleanup;
 	at_exit_manager = NULL;
 	cleanup = NULL;
+	destroy_g_ch_thpool();
 	has_initialized = false;
 	init_lock.unlock();
 }
-- 
Ammar Faizi

Re: [RFC PATCH v1 1/2] chnet: Prepare global struct ch_thpool array

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
> +       g_max_ch_thpool = std::thread::hardware_concurrency() - 1;
> +       if (g_max_ch_thpool < 1)
> +               g_max_ch_thpool = 1;

this is wrong, std::thread::hardware_concurrency() should be treated
as a hint, if this happens to return 0, then the sub with 1 will make
it ~0, that will definitely hit ENOMEM.

this should be:

        g_max_ch_thpool = std::thread::hardware_concurrency();
        if (g_max_ch_thpool <= 1)
                g_max_ch_thpool = 1;
        else
                g_max_ch_thpool -= 1;

tq

-- Viro

Re: [RFC PATCH v1 1/2] chnet: Prepare global struct ch_thpool array

[Ammar Nofan Faizi]

On 8/29/22 11:21 AM, Alviro Iskandar Setiawan wrote:
> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>> +       g_max_ch_thpool = std::thread::hardware_concurrency() - 1;
>> +       if (g_max_ch_thpool < 1)
>> +               g_max_ch_thpool = 1;
> 
> this is wrong, std::thread::hardware_concurrency() should be treated
> as a hint, if this happens to return 0, then the sub with 1 will make
> it ~0, that will definitely hit ENOMEM.
> 
> this should be:
> 
>          g_max_ch_thpool = std::thread::hardware_concurrency();
>          if (g_max_ch_thpool <= 1)
>                  g_max_ch_thpool = 1;
>          else
>                  g_max_ch_thpool -= 1;

Ah yes, you're right. Will fix it in the v2.

-- 
Ammar Faizi

[RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Ammar Faizi]

Currently, a single chnet instance uses a single dedicated chromium
thread worker to perform an HTTP request. This doesn't scale well
because we need to spawn a new thread for each HTTP request.

Performing 4096 HTTP requests in parallel will spawn 4096 chromium
thread workers, which is too expensive and consuming too much
memory.

A single chromium thread worker can handle multiple HTTP requests.
This series creates a fixed number of chromium workers with ref
count to spread the jobs fairly across the chromium thread workers.

This greatly reduces the context switches and improve the performance.
It also greatly reduces the memory usage.

Implementation:

1) At initialization, when chnet_global_init() is called, create an
   array of pointers to `struct ch_thpool` and initialize those
   pointers to null. The number of elements in the array is
   taken from `std::thread::hardware_concurrency() - 1`.

2) When a new CHNet instance is created, its constructor calls
   `get_thread()` function which will initialize the array of
   pointers to `struct ch_thpool` if needed, then increment
   the ref count and it returns a pointer to the base::Thread
   class in `struct ch_thpool`.

3) When a CHNet instance is destroyed, it calls `put_thread()`
   function that will decrement the ref count of the
   `struct ch_thpool` and delete the object if the ref count
   reaches zero after getting decremented.

For the ring test case, the gained speedup is 33%.

Without this patch:
  ammarfaizi2@integral2:~/work/ncns$ time taskset -c 0-7 make -j8 test -s
  Running /home/ammarfaizi2/work/ncns/tests/cpp/ring.t

  real    0m28.184s
  user    0m52.368s
  sys     0m27.582s

With this patch:
  ammarfaizi2@integral2:~/work/ncns$ time taskset -c 0-7 make -j8 test -s
  Running /home/ammarfaizi2/work/ncns/tests/cpp/ring.t

  real    0m18.657s
  user    0m35.452s
  sys     0m2.146s

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 chnet/chnet.cc | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-
 chnet/chnet.h  |  2 +-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index 8662374..782e4fe 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -12,6 +12,9 @@
 #include <cstdlib>
 using namespace std::chrono_literals;
 
+static base::Thread *get_thread(void);
+static void put_thread(void *thread);
+
 namespace net {
 
 CHNetSimplePayload::CHNetSimplePayload(const char *payload, size_t payload_len):
@@ -251,7 +254,7 @@ net::DefineNetworkTrafficAnnotation("CHNetDelegate", R"(
 	})");
 
 CHNetDelegate::CHNetDelegate(void):
-	thread_("chromium_thread"),
+	thread_(*get_thread()),
 	method_("GET"),
 	err_("")
 {
@@ -287,6 +290,7 @@ CHNetDelegate::~CHNetDelegate(void)
 	r->PostTask(FROM_HERE, base::BindOnce(CHNetDelegateDestruct, &url_req_,
 					      &url_req_ctx_, &sig));
 	sig.Wait();
+	put_thread(&thread_);
 }
 
 template <typename T, typename... Types>
@@ -629,6 +633,81 @@ static uint32_t g_max_ch_thpool;
 static std::mutex g_thpool_lock_;
 static struct ch_thpool **g_thpool;
 
+
+static base::Thread *get_thread(void)
+{
+	const uint32_t max_ch_thpool = g_max_ch_thpool;
+	const uint32_t nr_ref_split = 2048;
+	struct ch_thpool **thp;
+	struct ch_thpool *ret = nullptr;
+	struct ch_thpool *tmp;
+	uint32_t min_ref_idx;
+	uint32_t min_ref;
+	uint32_t i;
+
+	g_thpool_lock_.lock();
+	thp = g_thpool;
+	if (!thp) {
+		g_thpool_lock_.unlock();
+		return nullptr;
+	}
+
+	tmp = thp[0];
+	if (!tmp) {
+		ret = new struct ch_thpool;
+		ret->idx_ = 0;
+		thp[0] = ret;
+		goto out;
+	}
+
+	min_ref = tmp->ref_count_;
+	min_ref_idx = 0;
+	for (i = 1; i < max_ch_thpool; i++) {
+		uint32_t ref;
+
+		tmp = thp[i];
+		if (!tmp) {
+			ret = new struct ch_thpool;
+			ret->idx_ = i;
+			thp[i] = ret;
+			goto out;
+		}
+
+		ref = tmp->ref_count_;
+		if (ref < nr_ref_split) {
+			ret = tmp;
+			break;
+		}
+
+		if (ref < min_ref) {
+			min_ref = ref;
+			min_ref_idx = i;
+		}
+	}
+
+	if (!ret)
+		ret = thp[min_ref_idx];
+
+out:
+	ret->ref_count_++;
+	g_thpool_lock_.unlock();
+	return &ret->thread_;
+}
+
+static void put_thread(void *thread)
+{
+	struct ch_thpool *th = (struct ch_thpool *)thread;
+
+	g_thpool_lock_.lock();
+	if (--th->ref_count_ == 0) {
+		if (g_thpool)
+			g_thpool[th->idx_] = nullptr;
+
+		delete th;
+	}
+	g_thpool_lock_.unlock();
+}
+
 static void init_g_ch_thpool(void)
 {
 	struct ch_thpool **tmp;
diff --git a/chnet/chnet.h b/chnet/chnet.h
index 2332596..8f7b098 100644
--- a/chnet/chnet.h
+++ b/chnet/chnet.h
@@ -275,7 +275,7 @@ private:
 	std::unique_ptr<URLRequestContext>		url_req_ctx_;
 	std::unique_ptr<URLRequest>			url_req_;
 	scoped_refptr<IOBufferWithSize>			read_buf_;
-	base::Thread					thread_;
+	base::Thread					&thread_;
 	std::atomic<int>				read_ret_;
 	std::atomic<int>				status_;
 	std::string					method_;
-- 
Ammar Faizi

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
> @@ -251,7 +254,7 @@ net::DefineNetworkTrafficAnnotation("CHNetDelegate", R"(
>         })");
>
>  CHNetDelegate::CHNetDelegate(void):
> -       thread_("chromium_thread"),
> +       thread_(*get_thread()),
>         method_("GET"),
>         err_("")
>  {
> @@ -287,6 +290,7 @@ CHNetDelegate::~CHNetDelegate(void)
>         r->PostTask(FROM_HERE, base::BindOnce(CHNetDelegateDestruct, &url_req_,
>                                               &url_req_ctx_, &sig));
>         sig.Wait();
> +       put_thread(&thread_);
>  }

if @url_req_ and @url_req_ctx_ are both nullptr, this put_thread()
won't be called and we have a ref count leak

>  template <typename T, typename... Types>
> @@ -629,6 +633,81 @@ static uint32_t g_max_ch_thpool;
>  static std::mutex g_thpool_lock_;
>  static struct ch_thpool **g_thpool;
>
> +
> +static base::Thread *get_thread(void)
> +{
> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
> +       const uint32_t nr_ref_split = 2048;
> +       struct ch_thpool **thp;
> +       struct ch_thpool *ret = nullptr;
> +       struct ch_thpool *tmp;
> +       uint32_t min_ref_idx;
> +       uint32_t min_ref;
> +       uint32_t i;
> +
> +       g_thpool_lock_.lock();
> +       thp = g_thpool;
> +       if (!thp) {
> +               g_thpool_lock_.unlock();
> +               return nullptr;
> +       }

in what situation @thp can be nullptr?

> +       tmp = thp[0];
> +       if (!tmp) {
> +               ret = new struct ch_thpool;
> +               ret->idx_ = 0;
> +               thp[0] = ret;
> +               goto out;
> +       }
> +
> +       min_ref = tmp->ref_count_;
> +       min_ref_idx = 0;
> +       for (i = 1; i < max_ch_thpool; i++) {
> +               uint32_t ref;
> +
> +               tmp = thp[i];
> +               if (!tmp) {
> +                       ret = new struct ch_thpool;
> +                       ret->idx_ = i;
> +                       thp[i] = ret;
> +                       goto out;
> +               }
> +
> +               ref = tmp->ref_count_;
> +               if (ref < nr_ref_split) {
> +                       ret = tmp;
> +                       break;
> +               }
> +
> +               if (ref < min_ref) {
> +                       min_ref = ref;
> +                       min_ref_idx = i;
> +               }
> +       }
> +
> +       if (!ret)
> +               ret = thp[min_ref_idx];
> +
> +out:
> +       ret->ref_count_++;
> +       g_thpool_lock_.unlock();
> +       return &ret->thread_;
> +}

this unlock() call will behave as a full memory barrier for that
@ref_count_ increment, is this really needed? you can have the
increment after unlock() tho

tq

-- Viro

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Ammar Nofan Faizi]

On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>> @@ -251,7 +254,7 @@ net::DefineNetworkTrafficAnnotation("CHNetDelegate", R"(
>>          })");
>>
>>   CHNetDelegate::CHNetDelegate(void):
>> -       thread_("chromium_thread"),
>> +       thread_(*get_thread()),
>>          method_("GET"),
>>          err_("")
>>   {
>> @@ -287,6 +290,7 @@ CHNetDelegate::~CHNetDelegate(void)
>>          r->PostTask(FROM_HERE, base::BindOnce(CHNetDelegateDestruct, &url_req_,
>>                                                &url_req_ctx_, &sig));
>>          sig.Wait();
>> +       put_thread(&thread_);
>>   }
> 
> if @url_req_ and @url_req_ctx_ are both nullptr, this put_thread()
> won't be called and we have a ref count leak

Yes, you're right. Will fix it in the v2 revision.

>>   template <typename T, typename... Types>
>> @@ -629,6 +633,81 @@ static uint32_t g_max_ch_thpool;
>>   static std::mutex g_thpool_lock_;
>>   static struct ch_thpool **g_thpool;
>>
>> +
>> +static base::Thread *get_thread(void)
>> +{
>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>> +       const uint32_t nr_ref_split = 2048;
>> +       struct ch_thpool **thp;
>> +       struct ch_thpool *ret = nullptr;
>> +       struct ch_thpool *tmp;
>> +       uint32_t min_ref_idx;
>> +       uint32_t min_ref;
>> +       uint32_t i;
>> +
>> +       g_thpool_lock_.lock();
>> +       thp = g_thpool;
>> +       if (!thp) {
>> +               g_thpool_lock_.unlock();
>> +               return nullptr;
>> +       }
> 
> in what situation @thp can be nullptr?

When the chnet_global_destroy() is called.

>> +       tmp = thp[0];
>> +       if (!tmp) {
>> +               ret = new struct ch_thpool;
>> +               ret->idx_ = 0;
>> +               thp[0] = ret;
>> +               goto out;
>> +       }
>> +
>> +       min_ref = tmp->ref_count_;
>> +       min_ref_idx = 0;
>> +       for (i = 1; i < max_ch_thpool; i++) {
>> +               uint32_t ref;
>> +
>> +               tmp = thp[i];
>> +               if (!tmp) {
>> +                       ret = new struct ch_thpool;
>> +                       ret->idx_ = i;
>> +                       thp[i] = ret;
>> +                       goto out;
>> +               }
>> +
>> +               ref = tmp->ref_count_;
>> +               if (ref < nr_ref_split) {
>> +                       ret = tmp;
>> +                       break;
>> +               }
>> +
>> +               if (ref < min_ref) {
>> +                       min_ref = ref;
>> +                       min_ref_idx = i;
>> +               }
>> +       }
>> +
>> +       if (!ret)
>> +               ret = thp[min_ref_idx];
>> +
>> +out:
>> +       ret->ref_count_++;
>> +       g_thpool_lock_.unlock();
>> +       return &ret->thread_;
>> +}
> 
> this unlock() call will behave as a full memory barrier for that
> @ref_count_ increment, is this really needed? you can have the
> increment after unlock() tho

No, the ref_count needs to be protected by a mutex. Otherwise, we
have a use-after-free bug.

Possible UAF scenario:

       Thread1                Thread2
       ----                   -------
--> get_thread()
     lock()
     unlock()                 --> put_thread()
     # preempted away         lock()
                              decrement
                              delete
                              unlock()
     increment # UAF!!!       return
     return

-- 
Ammar Faizi

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>> @@ -251,7 +254,7 @@ net::DefineNetworkTrafficAnnotation("CHNetDelegate", R"(
>>>          })");
>>>
>>>   CHNetDelegate::CHNetDelegate(void):
>>> -       thread_("chromium_thread"),
>>> +       thread_(*get_thread()),
>>>          method_("GET"),
>>>          err_("")
>>>   {
>>> @@ -287,6 +290,7 @@ CHNetDelegate::~CHNetDelegate(void)
>>>          r->PostTask(FROM_HERE, base::BindOnce(CHNetDelegateDestruct, &url_req_,
>>>                                                &url_req_ctx_, &sig));
>>>          sig.Wait();
>>> +       put_thread(&thread_);
>>>   }
>>
>> if @url_req_ and @url_req_ctx_ are both nullptr, this put_thread()
>> won't be called and we have a ref count leak
>
> Yes, you're right. Will fix it in the v2 revision.
>
>>>   template <typename T, typename... Types>
>>> @@ -629,6 +633,81 @@ static uint32_t g_max_ch_thpool;
>>>   static std::mutex g_thpool_lock_;
>>>   static struct ch_thpool **g_thpool;
>>>
>>> +
>>> +static base::Thread *get_thread(void)
>>> +{
>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>> +       const uint32_t nr_ref_split = 2048;
>>> +       struct ch_thpool **thp;
>>> +       struct ch_thpool *ret = nullptr;
>>> +       struct ch_thpool *tmp;
>>> +       uint32_t min_ref_idx;
>>> +       uint32_t min_ref;
>>> +       uint32_t i;
>>> +
>>> +       g_thpool_lock_.lock();
>>> +       thp = g_thpool;
>>> +       if (!thp) {
>>> +               g_thpool_lock_.unlock();
>>> +               return nullptr;
>>> +       }
>>
>> in what situation @thp can be nullptr?
>
> When the chnet_global_destroy() is called.
>
>>> +       tmp = thp[0];
>>> +       if (!tmp) {
>>> +               ret = new struct ch_thpool;
>>> +               ret->idx_ = 0;
>>> +               thp[0] = ret;
>>> +               goto out;
>>> +       }
>>> +
>>> +       min_ref = tmp->ref_count_;
>>> +       min_ref_idx = 0;
>>> +       for (i = 1; i < max_ch_thpool; i++) {
>>> +               uint32_t ref;
>>> +
>>> +               tmp = thp[i];
>>> +               if (!tmp) {
>>> +                       ret = new struct ch_thpool;
>>> +                       ret->idx_ = i;
>>> +                       thp[i] = ret;
>>> +                       goto out;
>>> +               }
>>> +
>>> +               ref = tmp->ref_count_;
>>> +               if (ref < nr_ref_split) {
>>> +                       ret = tmp;
>>> +                       break;
>>> +               }
>>> +
>>> +               if (ref < min_ref) {
>>> +                       min_ref = ref;
>>> +                       min_ref_idx = i;
>>> +               }
>>> +       }
>>> +
>>> +       if (!ret)
>>> +               ret = thp[min_ref_idx];
>>> +
>>> +out:
>>> +       ret->ref_count_++;
>>> +       g_thpool_lock_.unlock();
>>> +       return &ret->thread_;
>>> +}
>>
>> this unlock() call will behave as a full memory barrier for that
>> @ref_count_ increment, is this really needed? you can have the
>> increment after unlock() tho
>
> No, the ref_count needs to be protected by a mutex. Otherwise, we
> have a use-after-free bug.
>
> Possible UAF scenario:
>
>        Thread1                Thread2
>        ----                   -------
> --> get_thread()
>      lock()
>      unlock()                 --> put_thread()
>      # preempted away         lock()
>                               decrement
>                               delete
>                               unlock()
>      increment # UAF!!!       return
>      return

ic ic, i understand now, i didn't see that coming

tq for explaining

tq

-- Viro

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 12:17 PM Alviro Iskandar Setiawan wrote:
> On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
>> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>>> +static base::Thread *get_thread(void)
>>>> +{
>>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>>> +       const uint32_t nr_ref_split = 2048;
>>>> +       struct ch_thpool **thp;
>>>> +       struct ch_thpool *ret = nullptr;
>>>> +       struct ch_thpool *tmp;
>>>> +       uint32_t min_ref_idx;
>>>> +       uint32_t min_ref;
>>>> +       uint32_t i;
>>>> +
>>>> +       g_thpool_lock_.lock();
>>>> +       thp = g_thpool;
>>>> +       if (!thp) {
>>>> +               g_thpool_lock_.unlock();
>>>> +               return nullptr;
>>>> +       }
>>>
>>> in what situation @thp can be nullptr?
>>
>> When the chnet_global_destroy() is called.

btw, i missed a case on the chnet construction, you do

   thread_(*get_thread()),

if get_thread() returns a nullptr, the @thread_ will become a
reference to a nullptr, it will definitely break, how do you handle
this?

tq

-- Viro

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Ammar Nofan Faizi]

On 8/29/22 12:24 PM, Alviro Iskandar Setiawan wrote:
> On Mon, Aug 29, 2022 at 12:17 PM Alviro Iskandar Setiawan wrote:
>> On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
>>> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>>>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>>>> +static base::Thread *get_thread(void)
>>>>> +{
>>>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>>>> +       const uint32_t nr_ref_split = 2048;
>>>>> +       struct ch_thpool **thp;
>>>>> +       struct ch_thpool *ret = nullptr;
>>>>> +       struct ch_thpool *tmp;
>>>>> +       uint32_t min_ref_idx;
>>>>> +       uint32_t min_ref;
>>>>> +       uint32_t i;
>>>>> +
>>>>> +       g_thpool_lock_.lock();
>>>>> +       thp = g_thpool;
>>>>> +       if (!thp) {
>>>>> +               g_thpool_lock_.unlock();
>>>>> +               return nullptr;
>>>>> +       }
>>>>
>>>> in what situation @thp can be nullptr?
>>>
>>> When the chnet_global_destroy() is called.
> 
> btw, i missed a case on the chnet construction, you do
> 
>     thread_(*get_thread()),
> 
> if get_thread() returns a nullptr, the @thread_ will become a
> reference to a nullptr, it will definitely break, how do you handle
> this?

I don't think we need to handle that. If @thp ever be a nullptr in a
get_thread() call, then it's a misuse of the API. We must never call
get_thread() after chnet_global_destroy() is called. It doesn't make
much sense to have this situation.

-- 
Ammar Faizi

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 12:29 PM Ammar Nofan Faizi wrote:
> On 8/29/22 12:24 PM, Alviro Iskandar Setiawan wrote:
>> On Mon, Aug 29, 2022 at 12:17 PM Alviro Iskandar Setiawan wrote:
>>> On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
>>>> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>>>>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>>>>> +static base::Thread *get_thread(void)
>>>>>> +{
>>>>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>>>>> +       const uint32_t nr_ref_split = 2048;
>>>>>> +       struct ch_thpool **thp;
>>>>>> +       struct ch_thpool *ret = nullptr;
>>>>>> +       struct ch_thpool *tmp;
>>>>>> +       uint32_t min_ref_idx;
>>>>>> +       uint32_t min_ref;
>>>>>> +       uint32_t i;
>>>>>> +
>>>>>> +       g_thpool_lock_.lock();
>>>>>> +       thp = g_thpool;
>>>>>> +       if (!thp) {
>>>>>> +               g_thpool_lock_.unlock();
>>>>>> +               return nullptr;
>>>>>> +       }
>>>>>
>>>>> in what situation @thp can be nullptr?
>>>>
>>>> When the chnet_global_destroy() is called.
>>
>> btw, i missed a case on the chnet construction, you do
>>
>>     thread_(*get_thread()),
>>
>> if get_thread() returns a nullptr, the @thread_ will become a
>> reference to a nullptr, it will definitely break, how do you handle
>> this?
>
> I don't think we need to handle that. If @thp ever be a nullptr in a
> get_thread() call, then it's a misuse of the API. We must never call
> get_thread() after chnet_global_destroy() is called. It doesn't make
> much sense to have this situation.

if this should never happen, then why do you do a null check there?
your null check doesn't make much sense too! why are we returning
nullptr if we exactly know that the caller will randomly fault if that
happens, i can't accept your reasoning

-- Viro

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Ammar Nofan Faizi]

On 8/29/22 12:38 PM, Alviro Iskandar Setiawan wrote:
> On Mon, Aug 29, 2022 at 12:29 PM Ammar Nofan Faizi wrote:
>> On 8/29/22 12:24 PM, Alviro Iskandar Setiawan wrote:
>>> On Mon, Aug 29, 2022 at 12:17 PM Alviro Iskandar Setiawan wrote:
>>>> On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
>>>>> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>>>>>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>>>>>> +static base::Thread *get_thread(void)
>>>>>>> +{
>>>>>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>>>>>> +       const uint32_t nr_ref_split = 2048;
>>>>>>> +       struct ch_thpool **thp;
>>>>>>> +       struct ch_thpool *ret = nullptr;
>>>>>>> +       struct ch_thpool *tmp;
>>>>>>> +       uint32_t min_ref_idx;
>>>>>>> +       uint32_t min_ref;
>>>>>>> +       uint32_t i;
>>>>>>> +
>>>>>>> +       g_thpool_lock_.lock();
>>>>>>> +       thp = g_thpool;
>>>>>>> +       if (!thp) {
>>>>>>> +               g_thpool_lock_.unlock();
>>>>>>> +               return nullptr;
>>>>>>> +       }
>>>>>>
>>>>>> in what situation @thp can be nullptr?
>>>>>
>>>>> When the chnet_global_destroy() is called.
>>>
>>> btw, i missed a case on the chnet construction, you do
>>>
>>>      thread_(*get_thread()),
>>>
>>> if get_thread() returns a nullptr, the @thread_ will become a
>>> reference to a nullptr, it will definitely break, how do you handle
>>> this?
>>
>> I don't think we need to handle that. If @thp ever be a nullptr in a
>> get_thread() call, then it's a misuse of the API. We must never call
>> get_thread() after chnet_global_destroy() is called. It doesn't make
>> much sense to have this situation.
> 
> if this should never happen, then why do you do a null check there?
> your null check doesn't make much sense too! why are we returning
> nullptr if we exactly know that the caller will randomly fault if that
> happens, i can't accept your reasoning

OK.

Fine, let's omit that nullptr return. To assert that path, we perform a
CHECK() assertion, so it will just crash the whole program with a full
call traces if the contract is violated. This way also makes debugging
process easier. Are you happy with this change?

Incremental is:

diff --git a/chnet/chnet.cc b/chnet/chnet.cc
index 782e4fe..fc536d2 100644
--- a/chnet/chnet.cc
+++ b/chnet/chnet.cc
@@ -647,10 +647,7 @@ static base::Thread *get_thread(void)
  
  	g_thpool_lock_.lock();
  	thp = g_thpool;
-	if (!thp) {
-		g_thpool_lock_.unlock();
-		return nullptr;
-	}
+	CHECK(thp);
  
  	tmp = thp[0];
  	if (!tmp) {

-- 
Ammar Faizi

Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan]

On Mon, Aug 29, 2022 at 12:48 PM Ammar Nofan Faizi wrote:
> On 8/29/22 12:38 PM, Alviro Iskandar Setiawan wrote:
>> On Mon, Aug 29, 2022 at 12:29 PM Ammar Nofan Faizi wrote:
>>> On 8/29/22 12:24 PM, Alviro Iskandar Setiawan wrote:
>>>> On Mon, Aug 29, 2022 at 12:17 PM Alviro Iskandar Setiawan wrote:
>>>>> On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
>>>>>> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>>>>>>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>>>>>>> +static base::Thread *get_thread(void)
>>>>>>>> +{
>>>>>>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>>>>>>> +       const uint32_t nr_ref_split = 2048;
>>>>>>>> +       struct ch_thpool **thp;
>>>>>>>> +       struct ch_thpool *ret = nullptr;
>>>>>>>> +       struct ch_thpool *tmp;
>>>>>>>> +       uint32_t min_ref_idx;
>>>>>>>> +       uint32_t min_ref;
>>>>>>>> +       uint32_t i;
>>>>>>>> +
>>>>>>>> +       g_thpool_lock_.lock();
>>>>>>>> +       thp = g_thpool;
>>>>>>>> +       if (!thp) {
>>>>>>>> +               g_thpool_lock_.unlock();
>>>>>>>> +               return nullptr;
>>>>>>>> +       }
>>>>>>>
>>>>>>> in what situation @thp can be nullptr?
>>>>>>
>>>>>> When the chnet_global_destroy() is called.
>>>>
>>>> btw, i missed a case on the chnet construction, you do
>>>>
>>>>      thread_(*get_thread()),
>>>>
>>>> if get_thread() returns a nullptr, the @thread_ will become a
>>>> reference to a nullptr, it will definitely break, how do you handle
>>>> this?
>>>
>>> I don't think we need to handle that. If @thp ever be a nullptr in a
>>> get_thread() call, then it's a misuse of the API. We must never call
>>> get_thread() after chnet_global_destroy() is called. It doesn't make
>>> much sense to have this situation.
>>
>> if this should never happen, then why do you do a null check there?
>> your null check doesn't make much sense too! why are we returning
>> nullptr if we exactly know that the caller will randomly fault if that
>> happens, i can't accept your reasoning
>
> OK.
>
> Fine, let's omit that nullptr return. To assert that path, we perform a
> CHECK() assertion, so it will just crash the whole program with a full
> call traces if the contract is violated. This way also makes debugging
> process easier. Are you happy with this change?
>
> Incremental is:
>
> diff --git a/chnet/chnet.cc b/chnet/chnet.cc
> index 782e4fe..fc536d2 100644
> --- a/chnet/chnet.cc
> +++ b/chnet/chnet.cc
> @@ -647,10 +647,7 @@ static base::Thread *get_thread(void)
>
>          g_thpool_lock_.lock();
>          thp = g_thpool;
> -        if (!thp) {
> -                g_thpool_lock_.unlock();
> -                return nullptr;
> -        }
> +        CHECK(thp);
>
>          tmp = thp[0];
>          if (!tmp) {

oc, ack, it looks fine to me now

tq

-- Viro