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 8EBBEA0597; Tue, 21 Apr 2020 15:54:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 103071D5D4; Tue, 21 Apr 2020 15:54:22 +0200 (CEST) Received: from mail-io1-f68.google.com (mail-io1-f68.google.com [209.85.166.68]) by dpdk.org (Postfix) with ESMTP id CF9D61D5D3 for ; Tue, 21 Apr 2020 15:54:20 +0200 (CEST) Received: by mail-io1-f68.google.com with SMTP id f19so15055511iog.5 for ; Tue, 21 Apr 2020 06:54:20 -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=6hC8Q7BA02j4767mfuAtSP6btMmdfg5myBB3p3hoOL0=; b=fZ9MDRFR+sUADvYWaSAgO6U6m3AeM6QFDbAqhnuRGg5xrIdA+/I2EcsMEoCS4h8n3Y ymktgLPWsEm3ilP+AV0gKyP+FOndQm26SsEpf1j/D+XNPjL18UoV4pGN2gADKSity9H5 SbTn0EdRrS9C02Da04l6e3+pHokrelvLQYic+boAdwLcXSaFsDLZ2YQsK6X4jF6f8PKt I3IsQwITGIYQA2VYsREO7hEU14qCi/+XGwmYbCBUSXq2vwvVz/D4ys42GHLxHhwq2vxE 4Xh38upwmoAiiJgL/1cihXrc23KiOZ7D8xpl9rctpzv7LcBin942+TFXdhAKmr3jbzJc PKwg== 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=6hC8Q7BA02j4767mfuAtSP6btMmdfg5myBB3p3hoOL0=; b=j3GprW33yjb4hDDYNl7AjQQcYhjj/cx6+dFIGwOZbOTLgPuxfrjjZ87ygQxcYcrB9f EJgyvRiw/0hQYyvVoz8OtqyHUPNXfsD0n4raO73Rl2lq6mw0ThnmipsZsIppzeCya7w1 kFUoRIAkWsKJtR0+YBoJMzElptvEI1zumG6mhzFRio+el66RQaz0/0Qvx97drLkUkUOP w3cSVwrC7e9r4TR+OUaYIXKte/0APc6umdJUYv/KH5+4sJ1aBU7OwhQ11KXdfWb5OCPy BQI+VA1zATCsTLKfTZq+50szgpuqSVh9Z2bwDQ4TvSA6yG2iTrbfTj+npYDiUAXY7ozj 8IdQ== X-Gm-Message-State: AGi0PuZ6NCVKYwWpOhWhK/FKv0BH+086HpSbTMsfNQzuK4Xc8qYgXcFT NTVR+tUGVAwtqsP2Is64ZM3Okj7Fq1jIPM6hL5w= X-Google-Smtp-Source: APiQypLY11AmEEQkpZTr1AaG2h+hXFOkVszC3YTLTHqrrpcDWIlm5yMdIV8T3gprsbpkN6QrjdM26PvlwOnd+zXtroA= X-Received: by 2002:a05:6638:2a2:: with SMTP id d2mr20995496jaq.104.1587477260073; Tue, 21 Apr 2020 06:54:20 -0700 (PDT) MIME-Version: 1.0 References: <20200413150116.734047-1-jerinj@marvell.com> <20200419100133.3232316-1-jerinj@marvell.com> <20200419100133.3232316-28-jerinj@marvell.com> In-Reply-To: From: Jerin Jacob Date: Tue, 21 Apr 2020 19:24:03 +0530 Message-ID: To: David Marchand Cc: Jerin Jacob Kollanukkaran , dev , Thomas Monjalon , Bruce Richardson , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , Sunil Kumar Kori Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v6 27/33] eal/trace: add unit test cases 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:23 PM David Marchand wrote: > > On Sun, Apr 19, 2020 at 12:03 PM wrote: > > > > From: Sunil Kumar Kori > > > > Example commands to run UT and check the traces with babeltrace viewer. > > > > - Delete the existing /root/dpdk-traces/ directory if needed. > > > sudo rm -rf /root/dpdk-traces/ > > > > - Start the dpdk-test > > > sudo ./build/app/test/dpdk-test -c 0x3 - --trace=.* > > Unit tests are there to do sanity/non regression checks. > Here you describe a manual test procedure. > Could it work in the CI for the functional parts? Except for test_trace_mode() all test cases run with trace disabled. The trace memory will be allocated per thread IFF the trace is enabled. I would like to disable trace by default and enable only if " --trace=.*" provided. Not sure what is the next step here? 1) Add this test case in app/test/meson.build under "fast_tests" ? 2) Can CI start with --trace=? If yes, is it required? what would the integration challenges like filesystem need for storing the traces etc. Thoughts? > > > > > > - Run trace_autotest > > > trace_autotest > > > > - View the traces with babletrace viewer. > > > sudo babeltrace /root/dpdk-traces/ > > > > Signed-off-by: Sunil Kumar Kori > > [snip] > > > diff --git a/app/test/test_trace.c b/app/test/test_trace.c > > new file mode 100644 > > index 000000000..26b403e62 > > --- /dev/null > > +++ b/app/test/test_trace.c > > @@ -0,0 +1,223 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(C) 2020 Marvell International Ltd. > > + */ > > + > > +#include > > +#include > > +#include > > + > > +#include "test.h" > > +#include "test_trace.h" > > + > > +static int32_t > > +test_trace_point_globbing(void) > > +{ > > + bool val; > > + int rc; > > + > > + rc = rte_trace_pattern("app.dpdk.test*", false); > > + if (rc != 1) > > + goto failed; > > + > > + val = rte_trace_point_is_enabled(&__app_dpdk_test_tp); > > + if (val == true) > > + goto failed; > > No need to test a boolean against true/false values. > Idem for the rest of this code. Ack. > > > > + > > + rc = rte_trace_pattern("app.dpdk.test*", true); > > + if (rc != 1) > > + goto failed; > > + > > + val = rte_trace_point_is_enabled(&__app_dpdk_test_tp); > > + if (val == false) > > + goto failed; > > + > > [snip] > > > +static int > > +test_trace_mode(void) > > +{ > > + enum rte_trace_mode current; > > + > > + current = rte_trace_mode_get(); > > + > > + if (rte_trace_is_enabled() == false) > > + return TEST_SKIPPED; > > This test would always be skipped if we called it from the CI, as it > requires the dpdk-test binary to be started with traces on. See above. > > > > + > > + rte_trace_mode_set(RTE_TRACE_MODE_DISCARD); > > + if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD) > > + goto failed; > > + > > [snip] > > > diff --git a/app/test/test_trace_register.c b/app/test/test_trace_register.c > > new file mode 100644 > > index 000000000..1735149a2 > > --- /dev/null > > +++ b/app/test/test_trace_register.c > > @@ -0,0 +1,17 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(C) 2020 Marvell International Ltd. > > + */ > > +#define RTE_TRACE_POINT_REGISTER_SELECT /* Select trace point register macros */ > > I noticed this comment is copy/pasted in all trace points "register" code. > This does not help. > > The documentation on the other side does not describe this macro which > is quite important for developers adding tracepoints to their code > (Thomas already commented about it on the doc patch). We have the following comments in the header file. Let me know, what else you would like to mention here? /** * Macro to select rte_trace_point_emit_* definition for trace register function * * rte_trace_point_emit_* emits different definitions for trace function. * Application must define RTE_TRACE_POINT_REGISTER_SELECT before including * rte_trace_point.h in the C file where RTE_TRACE_POINT_REGISTER used. * * @see RTE_TRACE_POINT_REGISTER */ #define RTE_TRACE_POINT_REGISTER_SELECT