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 1E6EDA0597; Tue, 21 Apr 2020 14:49:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EDA531C2B7; Tue, 21 Apr 2020 14:49:51 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by dpdk.org (Postfix) with ESMTP id 1F9B31C112 for ; Tue, 21 Apr 2020 14:49:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587473390; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ygunJ/mDxSw80gWhEmTSaCS+GtOrjWoqykyy9gdO3M0=; b=iJ3fHJFUVL+pxoOrJtbydQhHPa06Z0C86wBGqyEk/gE0yeTu6MpOklaVoh4TQRhWOhcgOS SXtTMBOQQ7OX78JffhKFTRonxO+0WiR3XKWRwoDV6pam/hg1sxXV/LNl0cCfMAoSzNE2fr krKOthduE3TbEcTQFam3hJ/gYe6Lpjo= Received: from mail-vk1-f200.google.com (mail-vk1-f200.google.com [209.85.221.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-12-KOUARIbJPI2pyOppB6SKXw-1; Tue, 21 Apr 2020 08:49:48 -0400 X-MC-Unique: KOUARIbJPI2pyOppB6SKXw-1 Received: by mail-vk1-f200.google.com with SMTP id c193so4938730vkc.14 for ; Tue, 21 Apr 2020 05:49:48 -0700 (PDT) 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=kMKHM59P8T+aPDvdvJvONz+3GIV95pWfOip9zcxVvUc=; b=TRyYtZGqjuzJe+B96ZsK+sbvD2xuBhGPqsSxDFlNwnHg7dD/w2rZ6fd14EVI5fZ4dV lT+OWhTr0VnOK+2mupZjc8B70z0wnVo794bgj57iFs+iufRZxe+1aHJuKb+sWCI3T4aJ gLYNwVsYbrL62DJ8/4ISD95J0Tk77ycIpHjgHjr8HEeHvfGeQvVqVWRO7+ryx6Cdc+S1 74nnRxK8zzDtCC4teTCscNtXC0PwjrDVvrW2+ryUSizAWKIgS5JTx+EPYX+pf3rbQ7RG yESbgxL49Cj1IomjuY/3vcyQ5tA8sbhZH6GHe3YwzsQyL7PkQva/SuHe2j4arijztNfc BduQ== X-Gm-Message-State: AGi0PuaPydwmYLfyElA8VoSpQaYnHWEqSZX7zhB+w09gvEyoG7tJWm1Z MX1uSBYNYDWPFOEFoztS4ZUQutpo0cHzTLOGRf3Osmn05y5Zu3WJfWvF904vaBBXaKpsrMdBDQh 41f7tvzpyD5rifGDKx0Q= X-Received: by 2002:a67:487:: with SMTP id 129mr16005083vse.105.1587473388257; Tue, 21 Apr 2020 05:49:48 -0700 (PDT) X-Google-Smtp-Source: APiQypJgP2jN2uC5VZneREwQ1arBuWB9e9hoTY2xFnWvfGNdXCHdDJy+0Bwfjl7Z3MaO21JT5sD0jfLttkJZ2Y5CYAA= X-Received: by 2002:a67:487:: with SMTP id 129mr16005048vse.105.1587473387917; Tue, 21 Apr 2020 05:49:47 -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: <20200419100133.3232316-6-jerinj@marvell.com> From: David Marchand Date: Tue, 21 Apr 2020 14:49:36 +0200 Message-ID: To: Jerin Jacob Kollanukkaran Cc: Sunil Kumar Kori , dev , Thomas Monjalon , Bruce Richardson , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Sun, Apr 19, 2020 at 12:02 PM wrote: > diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/co= mmon/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. > > #include > #include > @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count); > static struct trace_point_head tp_list =3D STAILQ_HEAD_INITIALIZER(tp_li= st); > 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 =3D=3D RTE_TRACE_MODE_OVERWRITE) > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCA= RD, > + __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() =3D=3D false) > + return; rte_trace_is_enabled() returns a boolean, no need to test its value, should= be: if (!rte_trace_is_enabled()) > + > + STAILQ_FOREACH(tp, &tp_list, next) > + trace_mode_set(tp->handle, mode); > + > + trace.mode =3D 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 =3D=3D 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 =3D=3D NULL) || (trace_id_get(t) >=3D trace.nb_trace_poin= ts); } > + > + if (trace_id_get(t) >=3D 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 =3D __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) !=3D 0; > +} > + > +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? > + > +int > +rte_trace_pattern(const char *pattern, bool enable) > +{ > + struct trace_point *tp; > + int rc =3D 0, found =3D 0; > + > + STAILQ_FOREACH(tp, &tp_list, next) { > + if (fnmatch(pattern, tp->name, 0) =3D=3D 0) { > + if (enable) > + rc =3D rte_trace_point_enable(tp->handle)= ; > + else > + rc =3D rte_trace_point_disable(tp->handle= ); > + found =3D 1; > + } > + if (rc < 0) > + return rc; > + } > + > + return rc | found; > +} > + > +int > +rte_trace_regexp(const char *regex, bool enable) > +{ > + struct trace_point *tp; > + int rc =3D 0, found =3D 0; > + regex_t r; > + > + if (regcomp(&r, regex, 0) !=3D 0) > + return -EINVAL; > + > + STAILQ_FOREACH(tp, &tp_list, next) { > + if (regexec(&r, tp->name, 0, NULL, 0) =3D=3D 0) { > + if (enable) > + rc =3D rte_trace_point_enable(tp->handle)= ; > + else > + rc =3D rte_trace_point_disable(tp->handle= ); > + found =3D 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. --=20 David Marchand