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

[Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>]

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