From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oo1-f45.google.com (mail-oo1-f45.google.com [209.85.161.45]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6343BC8; Sat, 2 Dec 2023 15:55:11 -0800 (PST) Received: by mail-oo1-f45.google.com with SMTP id 006d021491bc7-58d956c8c38so2217451eaf.2; Sat, 02 Dec 2023 15:55:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701561310; x=1702166110; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=yvPGCQtpM8COGZWcKiIG7o+AKmKAg3mE1nqD/jJyBKM=; b=TeTEtjpggNXJQqNW60aYkthEfkil+xaZ3ZXzEUBz2QoX1cBOCa7w4hnq9YSnDxzPcP Nf0UDjWX8QqHvD4GmwmuPAGNvGYjNwCmWIx4rXB9av2z+/4bKvA0A6fVj6A7jieHPP3O Ky2IU219HNQUdPMoRkeTgXXj98iAPTM0fit7swU92qzM25+nYCG0GW6htDR73EB7F5sv ejKK6IjZ/EJY/x06n1cBIuKPfSfrpR5Z/CPLiylqCVPDAtT9fWdmqmm6Gc9QkbbwH+Zv oGd6UmwPzLMC4KnzaWev6e9m8jQahqAlK9V0Fcgou2OewkWfrjjMboeA/9BMNSkW6Q0j QezA== X-Gm-Message-State: AOJu0Yzia/HL/Wl6PWuktYg6UD6bIymJTTSTrqTkv3Vw4PRBA6sQPgf8 /tLBjQeGahSWrM7BegnouIyHc9IeM9tYYWr87LA= X-Google-Smtp-Source: AGHT+IGurGlqmnhod4tK8Jb8JdU1RIuFoNdl9Di3c9eU9P3CQz3pjxz7wUt6N6oWAtWaOt8bhYyoUuQn1/D6sql+euw= X-Received: by 2002:a05:6358:7e98:b0:16e:283:665d with SMTP id o24-20020a0563587e9800b0016e0283665dmr2591404rwn.15.1701561310428; Sat, 02 Dec 2023 15:55:10 -0800 (PST) Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20231127220902.1315692-1-irogers@google.com> <20231127220902.1315692-2-irogers@google.com> In-Reply-To: From: Namhyung Kim Date: Sat, 2 Dec 2023 15:54:59 -0800 Message-ID: Subject: Re: [PATCH v5 01/50] perf comm: Use regular mutex To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Nick Terrell , Kan Liang , Andi Kleen , Kajol Jain , Athira Rajeev , Huacai Chen , Masami Hiramatsu , Vincent Whitchurch , "Steinar H. Gunderson" , Liam Howlett , Miguel Ojeda , Colin Ian King , Dmitrii Dolgov <9erthalion6@gmail.com>, Yang Jihong , Ming Wang , James Clark , K Prateek Nayak , Sean Christopherson , Leo Yan , Ravi Bangoria , German Gomez , Changbin Du , Paolo Bonzini , Li Dong , Sandipan Das , liuwenyu , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, Guilherme Amadio Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Nov 30, 2023 at 10:28=E2=80=AFAM Ian Rogers wr= ote: > > On Wed, Nov 29, 2023 at 4:56=E2=80=AFPM Namhyung Kim wrote: > > > > On Mon, Nov 27, 2023 at 2:09=E2=80=AFPM Ian Rogers = wrote: > > > > > > The rwsem is only after used for writing so switch to a mutex that ha= s > > > better error checking. > > > > > > Fixes: 7a8f349e9d14 ("perf rwsem: Add debug mode that uses a mutex") > > > > I think we talked about fixing this separately, no? > > Sorry, I'm unclear on an action to do. Currently changing the > RWS_ERRORCHECK in tools/perf/util/rwsem.h will break the build without > this change. Can it be like this? #ifdef RWS_ERRORCHECK #define RWSEM_INITIALIZER { .lock =3D PTHREAD_MUTEX_INITIALIZER, } #else #define RWSEM_INITIALIZER { .lock =3D PTHREAD_RWLOCK_INITIALIZER, } #endif > > > > Signed-off-by: Ian Rogers > > > --- > > > tools/perf/util/comm.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c > > > index afb8d4fd2644..4ae7bc2aa9a6 100644 > > > --- a/tools/perf/util/comm.c > > > +++ b/tools/perf/util/comm.c > > > @@ -17,7 +17,7 @@ struct comm_str { > > > > > > /* Should perhaps be moved to struct machine */ > > > static struct rb_root comm_str_root; > > > -static struct rw_semaphore comm_str_lock =3D {.lock =3D PTHREAD_RWLO= CK_INITIALIZER,}; > > > +static struct mutex comm_str_lock =3D {.lock =3D PTHREAD_ERRORCHECK_= MUTEX_INITIALIZER_NP,}; > > > > IIUC it has a problem with musl libc. Actually I think it's better to > > hide the field and the pthread initializer under some macro like > > MUTEX_INITIALIZER or DEFINE_MUTEX() like in the kernel. > > Will there be enough use to justify this? I think ideally we'd not be > having global locks needing global initializers as we run into > problems like we see in metrics needing to mix counting and sampling. I don't know but there might be a reason to use global locks. Then we need to support the initialization and it'd be better to make it easier to handle internal changes like this. Thanks, Namhyung