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 B4713A0597; Tue, 21 Apr 2020 16:10:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 874261D419; Tue, 21 Apr 2020 16:10:13 +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 31A7B1D412 for ; Tue, 21 Apr 2020 16:10:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587478211; 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=sbDG6IAgiRlp5gEHL0469WuL1fZYB1ZqryhaBfDuD/s=; b=Nuu5CinTqdmmnLpGj63TJ0Z2s7FFpzCO2sg+IbSaUkoR7vBT2e7YVbrw1Iol8ahijUjBV7 LKVB4ExPTXgb9JZo7d8MAhO+zPXz//u1fZvTTbRWPEDFzmVPYOcY50q9dhKntTYgN0Qljr b1FSUH0v0CTzz4YxI70oDV8BhmbOqt4= Received: from mail-vs1-f69.google.com (mail-vs1-f69.google.com [209.85.217.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-74-Iyn-6zpPMwuLc6HplrqFFQ-1; Tue, 21 Apr 2020 10:10:07 -0400 X-MC-Unique: Iyn-6zpPMwuLc6HplrqFFQ-1 Received: by mail-vs1-f69.google.com with SMTP id f22so1405176vsa.7 for ; Tue, 21 Apr 2020 07:10:07 -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=EP3P73B1N+eVnF9G+2v4CP8F4N/piT4FxtjwRbamkQI=; b=hW7Fw9EG0gTph/Ud/4mcnAQyQw4Na8En/RxBjEZJKl8kx3Naj1ssghdYgSW9pDTGTi Cl8MMxwMOk8Vh2IWrYYlgX5TZbc+6azteOCj+0Ps7XzTmYUtZGH6ALfktBZ92VG3xme2 xy4aYzKGarDtV4ssVAVlacLzO4xMsVcWvAO0zxl2rC+RM6RL31gHAshFZ1pFboGbX0VK RXuaBr018gnZgCnqs90uY1bqrnoLNnBiyFoXjKJO1QzXadHRXzWLqltKisfq3sjziEop ywB2uPrLW73t4GY3Xiq9B+J88KIcOMIs+dFL5wOVOdNcb+SAth/igjQRF4TxDkLxOU7X Qmog== X-Gm-Message-State: AGi0PubUTkLUUfYYJI5jbWTfSVPixpsqtG36omB0qS8tZinbcml8NKkN UFGxLrQPH0AJHi7L/hqK9Y7gpWh5DyZj7Ufw32n80+fVWLLYr8JELHrybohEC3BMaZspE0KZFN5 vDWvCWDQYucgWlcqSd2I= X-Received: by 2002:ab0:485:: with SMTP id 5mr11590880uaw.86.1587478206663; Tue, 21 Apr 2020 07:10:06 -0700 (PDT) X-Google-Smtp-Source: APiQypLxBbhAq8Ih/7Z/fjJRx7WsOZDv7Sph8zpxvf80L2J29ZFCq9szs8UXrH4AKJY9Zvt1mTyzzpOxeoa7In5mZgM= X-Received: by 2002:ab0:485:: with SMTP id 5mr11590842uaw.86.1587478206298; Tue, 21 Apr 2020 07:10:06 -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: David Marchand Date: Tue, 21 Apr 2020 16:09:54 +0200 Message-ID: To: Jerin Jacob Cc: Jerin Jacob Kollanukkaran , 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 Tue, Apr 21, 2020 at 3:47 PM Jerin Jacob wrote: > > > @@ -20,6 +22,151 @@ static RTE_DEFINE_PER_LCORE(int, ctf_count); > > > static struct trace_point_head tp_list =3D STAILQ_HEAD_INITIALIZER(t= p_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 =3D=3D RTE_TRACE_MODE_OVERWRITE) > > > + __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_D= ISCARD, > > > + __ATOMIC_RELEASE); > > > + else > > > + __atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DIS= CARD, > > > + __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, sh= ould be: > > if (!rte_trace_is_enabled()) > > I like the ! scheme. I thought, DPDK community like =3D=3D false scheme. > I will change it in v7. Not obvious, but I understand this part as talking about the boolean case: https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers "Do not use ! for tests unless it is a boolean, for example, use:" > > > +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 valid= ity. > > 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. Ok, fair enough. > > > + > > > +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->han= dle); > > > + else > > > + rc =3D rte_trace_point_disable(tp->ha= ndle); > > > + 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->han= dle); > > > + else > > > + rc =3D rte_trace_point_disable(tp->ha= ndle); > > > + 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. > > 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. Would be odd to predict what happened if we broke for this loop on one tracepoint error, but ok to leave as is. --=20 David Marchand