From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 23C72A0597; Tue, 21 Apr 2020 15:41:15 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DA02E1D5C8; Tue, 21 Apr 2020 15:41:13 +0200 (CEST) Received: from mail-il1-f194.google.com (mail-il1-f194.google.com [209.85.166.194]) by dpdk.org (Postfix) with ESMTP id C799F1D5C0 for ; Tue, 21 Apr 2020 15:41:12 +0200 (CEST) Received: by mail-il1-f194.google.com with SMTP id t12so329141ile.9 for ; Tue, 21 Apr 2020 06:41:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KzALkvpre+VnsyfVecao1BROHMWwr1cC87Mqui3ENQE=; b=oWYH7ouOcuYEnTwnhcCMt/XEgoIIqT7O6sjGr234l3m26wmGx1X3x5meebRPteqTfQ Ai/Hhnv9JyCLFIuYx0miruU5kSCeJ3tmNA3SVFIL97PytgUCPmrKjGY79GNFE+LZdkl/ 6Xc7pjtyRv8L9AHpkHJStA2iVSu1GWK9+y9zjDwwKkh6NJ1LO0yIRwuT+LFd3cR6UIKd 51rVkkHWyqNQH9ATFX2imRS04Tj6poqUIyfyyi4VZVbjVBa+3xfZX9720W/BFDLlnZ71 n2VcJz7VzXjON8xDQHWn1lSsD2XPfVH6/JTz9Ayvpduuq0FwfhEo/PNIR3ndF6ZXHGgf G8mw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KzALkvpre+VnsyfVecao1BROHMWwr1cC87Mqui3ENQE=; b=p3pW/mOwodN2U5gpVkqYv6wSM93ZASZ/stqxc2tcZaC59nM+97h7HhNXi2TZ9UdKAR /neYsNZl/x15oge/L+4Ada5g3Fg/Sc6lvb25k0nNHO2LUF8ldGqbQHqUqx5wyjhgCu8e 2eVhxU+XHWkYP88323mbbIJ51ooam6TTgQ0oVO1+db7xClhvXQQhToPonsdbBLDELF8V vpfF72dOiI/Zdq0Hp7qIOto3Fjjw9fB+WsKbB3geia7l5PquGP9+S7NLEPUqjrlKuOLS KkjrDr+a6c7MObBXzNQkGdALCUYjAYFoXf4fK/yjEVsxUmajCoHJYH2rCm2UKgiBNgHK LHjQ== X-Gm-Message-State: AGi0PuZ3UKhmu3Mqd3YaI6Owvlv1Fw4vivcAUxZ6X7fbAXJ8UmDoCCgH qBYiSWw8TiPxsJ6xWL/n7rYzzSMZKSRW0n+hgbI= X-Google-Smtp-Source: APiQypIiFRhn/dcHu1JcKCIOvVxzOnzevygkIhq4T7j87N3lHJSBvScKTd3JxHJFdtx1rrG11VdFJ2K1K2HNz8/AODc= X-Received: by 2002:a92:485b:: with SMTP id v88mr20998766ila.271.1587476471900; Tue, 21 Apr 2020 06:41:11 -0700 (PDT) MIME-Version: 1.0 References: <20200413150116.734047-1-jerinj@marvell.com> <20200419100133.3232316-1-jerinj@marvell.com> <20200419100133.3232316-6-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Tue, 21 Apr 2020 19:10:55 +0530 Message-ID: To: David Marchand Cc: Jerin Jacob Kollanukkaran , Sunil Kumar Kori , dev , Thomas Monjalon , Bruce Richardson , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v6 05/33] eal/trace: implement trace operation APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Apr 21, 2020 at 6:19 PM David Marchand wrote: > > On Sun, Apr 19, 2020 at 12:02 PM wrote: > > diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c > > index 5c5cbd2a1..1ca702f68 100644 > > --- a/lib/librte_eal/common/eal_common_trace.c > > +++ b/lib/librte_eal/common/eal_common_trace.c > > @@ -3,7 +3,9 @@ > > */ > > > > #include > > +#include > > #include > > +#include > > Alphabetical sort when possible. Ack. Wil fix it v7 > > > > > #include > > #include > > @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count); > > static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list); > > static struct trace trace; > > > > +bool > > +rte_trace_is_enabled(void) > > +{ > > + return trace.status; > > +} > > + > > +static void > > +trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode) > > +{ > > + if (mode == RTE_TRACE_MODE_OVERWRITE) > > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD, > > + __ATOMIC_RELEASE); > > + else > > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD, > > + __ATOMIC_RELEASE); > > +} > > + > > +void > > +rte_trace_mode_set(enum rte_trace_mode mode) > > +{ > > + struct trace_point *tp; > > + > > + if (rte_trace_is_enabled() == false) > > + return; > > rte_trace_is_enabled() returns a boolean, no need to test its value, should be: > if (!rte_trace_is_enabled()) I like the ! scheme. I thought, DPDK community like == false scheme. I will change it in v7. > > > > + > > + STAILQ_FOREACH(tp, &tp_list, next) > > + trace_mode_set(tp->handle, mode); > > + > > + trace.mode = mode; > > +} > > + > > +enum > > +rte_trace_mode rte_trace_mode_get(void) > > +{ > > + return trace.mode; > > +} > > + > > +static bool > > +trace_point_is_invalid(rte_trace_point_t *t) > > +{ > > + if (t == NULL) > > + return false; > > Should be "return true"? > > Or maybe simply rewrite as: > > static bool > trace_point_is_invalid(rte_trace_point_t *t) > { > return (t == NULL) || (trace_id_get(t) >= trace.nb_trace_points); > } Ack. > > > + > > + if (trace_id_get(t) >= trace.nb_trace_points) > > + return true; > > + > > + return false; > > +} > > + > > +bool > > +rte_trace_point_is_enabled(rte_trace_point_t *trace) > > +{ > > + uint64_t val; > > + > > + if (trace_point_is_invalid(trace)) > > + return false; > > + > > + val = __atomic_load_n(trace, __ATOMIC_ACQUIRE); > > + return val & __RTE_TRACE_FIELD_ENABLE_MASK; > > We return a boolean, should be > return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0; Ack. > > > > +} > > + > > +int > > +rte_trace_point_enable(rte_trace_point_t *trace) > > +{ > > + if (trace_point_is_invalid(trace)) > > + return -ERANGE; > > + > > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK, > > + __ATOMIC_RELEASE); > > + return 0; > > +} > > + > > +int > > +rte_trace_point_disable(rte_trace_point_t *trace) > > +{ > > + if (trace_point_is_invalid(trace)) > > + return -ERANGE; > > + > > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK, > > + __ATOMIC_RELEASE); > > + return 0; > > +} > > For both rte_trace_point_enable/disable, we only check tracepoint validity. > Can't we just return a boolean, do we expect many error codes? I think, it is better to return "int" so that we may not ABI the change in future. > > > > + > > +int > > +rte_trace_pattern(const char *pattern, bool enable) > > +{ > > + struct trace_point *tp; > > + int rc = 0, found = 0; > > + > > + STAILQ_FOREACH(tp, &tp_list, next) { > > + if (fnmatch(pattern, tp->name, 0) == 0) { > > + if (enable) > > + rc = rte_trace_point_enable(tp->handle); > > + else > > + rc = rte_trace_point_disable(tp->handle); > > + found = 1; > > + } > > + if (rc < 0) > > + return rc; > > + } > > + > > + return rc | found; > > +} > > + > > +int > > +rte_trace_regexp(const char *regex, bool enable) > > +{ > > + struct trace_point *tp; > > + int rc = 0, found = 0; > > + regex_t r; > > + > > + if (regcomp(&r, regex, 0) != 0) > > + return -EINVAL; > > + > > + STAILQ_FOREACH(tp, &tp_list, next) { > > + if (regexec(&r, tp->name, 0, NULL, 0) == 0) { > > + if (enable) > > + rc = rte_trace_point_enable(tp->handle); > > + else > > + rc = rte_trace_point_disable(tp->handle); > > + found = 1; > > + } > > + if (rc < 0) > > + return rc; > > + } > > + regfree(&r); > > + > > + return rc | found; > > +} > > For both rte_trace_pattern/rte_trace_regexp, tp is a member of tp_list. > It means this tracepoint went through proper registration. > Hence, checking for a return value from rte_trace_point_(en|dis)able > is unneeded. Yes. I thought of it, But I think, it is safe to check the return code as it absorbs any future rte_trace_point_enable() changes. It slow-path code, so it OK IMO. > > > -- > David Marchand >