From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 172978060F for ; Mon, 29 Aug 2022 06:02:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=default; t=1661752920; bh=oURqJpnFLY2xs5ZazUc4NxUYPILslPBOBFYYLvb33yA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=kVduYZFtD4/WORCnNGVP8Aiv0+b+TjllTtR9mMacWQj/7NluFELM02ELB7oYhUS+7 k/fmadZ8N1qZRZ+y9G5qV766/vH810riwlAAK5EDeHkWhTJBFAxHepAeJwIo87UXoo zp8yW0RkESev0JuRiQl1nosm7IEefGr8rJo7C7ws6BdE3C719nkrfUL9h8wCeGu4ZZ 8193Oh5M4G7IBHnMxkH2SrTe+4sUHCs8ZkZtA+4swmeKGzenou9g6W+rNkbag8KUEo 1rLth+MuXRNlwzK0yVBi2SaX3Fxe0ySx5NCyCpawjZIVKq5AMXDJj26beQHz6uzlJO Y234PngO2WNTw== Received: by mail-lj1-f182.google.com with SMTP id z23so4139693ljk.1 for ; Sun, 28 Aug 2022 23:01:59 -0700 (PDT) X-Gm-Message-State: ACgBeo21wdWYg9YAIKehU8x6Mgv5Zm6F70PYtXSaTLx7QZ4jc7iT3tk+ 0iDUlF9lEbsqRsTO6bM872F4y2R5GOeCCJGuCwk= X-Google-Smtp-Source: AA6agR6RMOHZnqpYoRYtlm7w3im14kZF0XXXzG6g4bTqMOJP6yM3IsN+C7PRTjDKilvkA8UUf1J1dyh4yKslQJt+OtE= X-Received: by 2002:a2e:9604:0:b0:25e:4ed7:ef45 with SMTP id v4-20020a2e9604000000b0025e4ed7ef45mr4900916ljh.389.1661752918253; Sun, 28 Aug 2022 23:01:58 -0700 (PDT) MIME-Version: 1.0 References: <20220829011127.3150320-1-ammarfaizi2@gnuweeb.org> <20220829011127.3150320-3-ammarfaizi2@gnuweeb.org> In-Reply-To: From: Alviro Iskandar Setiawan Date: Mon, 29 Aug 2022 13:01:46 +0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function To: Ammar Nofan Faizi Cc: Ammar Faizi , Muhammad Rizki , Kanna Scarlet , "GNU/Weeb Mailing List" Content-Type: text/plain; charset="UTF-8" List-Id: 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