From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alviro.iskandar@gnuweeb.org>
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on gnuweeb.org
X-Spam-Level: 
X-Spam-Status: No, score=-0.8 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NO_DNS_FOR_FROM,URIBL_BLOCKED
	autolearn=no autolearn_force=no version=3.4.6
Received: from mail-lj1-f182.google.com (mail-lj1-f182.google.com [209.85.208.182])
	by gnuweeb.org (Postfix) with ESMTPSA id 92DEC80B23
	for <gwml@vger.gnuweeb.org>; Mon, 29 Aug 2022 04:42:08 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org;
	s=default; t=1661748128;
	bh=fSXVwVe5bQFg1Bu98D9zSpOCv/q8579RG65OgpawAWM=;
	h=References:In-Reply-To:From:Date:Subject:To:Cc:From;
	b=RdqYfaml7dl/Uh9+uU9O1XNS5XNj1nm4oKLZHb0MYAx+awNkJF/b4za+jiY/cze3m
	 chlR/6yMvnfiRC6MVQzzpyjIkY00/8DzCJ0MHhWh+58Y7pb5q9dwbjzZIKinuMF/1D
	 GzQuouaMxM4lBNmJw81AH8U2HeJafBH/HLjrewPE+A6dH1rSK8T+r92hpK54kSPyVI
	 FWoNo7pMeDCN0q64IxjShvkXwIJW/7M7WhpU1B3d5EWEvFPfmsgRLkb7oMxp2H2WPS
	 1JpXYRelDsCRhrHvTJTaUQxCvppoEAOECvm6e5N/5iYLMtV25p3Nx0WUhXQThAUU+8
	 vv/l7A2sAUpbQ==
Received: by mail-lj1-f182.google.com with SMTP id by6so6875051ljb.11
        for <gwml@vger.gnuweeb.org>; Sun, 28 Aug 2022 21:42:08 -0700 (PDT)
X-Gm-Message-State: ACgBeo3dBWe/tfH0UQ+mi0mgNTnYuY/S7p/pfzP3zQUm26Eq29D9IrDW
	o0UMmZHR6sPpLcxd95vB9WDHHvw8mhP/p8epmwI=
X-Google-Smtp-Source: AA6agR4zqGuGqUCuFPOFCHMszD6Iz7/ikz1xMtNAPi7lLdiaJFNPLI/v3AKiizkygayHKfU8R971gHAHG9iYegJLyfM=
X-Received: by 2002:a05:651c:b2c:b0:25e:6e68:ff51 with SMTP id
 b44-20020a05651c0b2c00b0025e6e68ff51mr4928582ljr.349.1661748126656; Sun, 28
 Aug 2022 21:42:06 -0700 (PDT)
MIME-Version: 1.0
References: <20220829011127.3150320-1-ammarfaizi2@gnuweeb.org> <20220829011127.3150320-3-ammarfaizi2@gnuweeb.org>
In-Reply-To: <20220829011127.3150320-3-ammarfaizi2@gnuweeb.org>
From: Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>
Date: Mon, 29 Aug 2022 11:41:55 +0700
X-Gmail-Original-Message-ID: <CAOG64qOTrV=CRS=UMnXL1PY5EqkcCxtsJNpKD9kU0PpqD2USew@mail.gmail.com>
Message-ID: <CAOG64qOTrV=CRS=UMnXL1PY5EqkcCxtsJNpKD9kU0PpqD2USew@mail.gmail.com>
Subject: Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and
 `put_thread()` function
To: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Cc: Muhammad Rizki <kiizuha@gnuweeb.org>, Kanna Scarlet <knscarlet@gnuweeb.org>, 
	"GNU/Weeb Mailing List" <gwml@vger.gnuweeb.org>
Content-Type: text/plain; charset="UTF-8"
List-Id: <gwml.vger.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