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 E8202A0598; Tue, 21 Apr 2020 16:59:01 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CA7B41D445; Tue, 21 Apr 2020 16:59:01 +0200 (CEST) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by dpdk.org (Postfix) with ESMTP id BED261D171 for ; Tue, 21 Apr 2020 16:59:00 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id i3so15260355ioo.13 for ; Tue, 21 Apr 2020 07:59:00 -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=om9f6c5QZllQ6eIrYb3+zoHgt/M6HFsm315vjnPSG5o=; b=YTzU6x7sI5CVZYnqcLrX2EaZrebbcX6DlE7XT16p7XbcY03l7HrNm9GVsXQWpmSMmm tlDDvfvA4lgV9JWcdPjbqxwVDVPIWuLFdjBW6Zg3vafX58H/U9fOrlycWdHlphZMa8WT ++2XEpG+BdLmWa+Ika0QIMnhQDOK8rboquYl94lTTGj81uxHY3D4BKel0N0JCxUGxy8z Xwg+CZ0klBJ2KFDzKoIsfTS/YnVL4GKRgonbqst/1IIoWAaWNsbTPSvS8ymxcmkZmKep qcvB3IEZ5ghGyMZ3qkQTlQlZCy8vZwtJl8a2EazoNj5zJRHbJ+oOs48NQowcCFcKAdOq YTyA== 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=om9f6c5QZllQ6eIrYb3+zoHgt/M6HFsm315vjnPSG5o=; b=Bdlu2PV6Kd2+U3EKhAvFxl1+dyKFcXqI6Lw29Jm/43ighcaQ+mqNsVLtvNm7IvXS6O z8BEAMiKNPX5tOZCPSsYI81qbT2pJVECWMtjSXcOsxDTpcaf0/ljqhjkdTMrngPDgokK oqmvhNO7autsRk3gO/ousZBy6YVknxNfYrcNlHCDXfk1XUlpY/LQttJTUw3VhX6pxsMb TSZE/kMEfGLTVQbBKTxkfazeNNfAx2SVDFlz4ySWttorC+KUU358gqEk5HTmlxp3NFWm 1qH5Imydu8gtPrPY7atXdW7/bk9yrm0XVC8IdHNwNxc+Ga1UtRmSY52u7u/SECwaw4Bm WqVA== X-Gm-Message-State: AGi0PuZ0YmeoTOnzqkzZnH6c+dz2yhCfCFcNooXTHDFYuCnHXNBwKtBS 5dU1YTRITNtdCEyzotdhhdik9RO5FeZAsDgwryU= X-Google-Smtp-Source: APiQypIkQtyoP6n6jG69O4Wak16mdeuoqh/FVEX0cGZ0u8XTx7hE7Tn9eKytKHnLq913PyRym/t4TJ0lwSggqnU9V1c= X-Received: by 2002:a05:6602:21c6:: with SMTP id c6mr20736585ioc.163.1587481139800; Tue, 21 Apr 2020 07:58:59 -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 20:28:43 +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 7:55 PM David Marchand wrote: > > On Tue, Apr 21, 2020 at 3:54 PM Jerin Jacob wrote: > > > > 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? > > - At least putting in fast_tests will get us basic checks on the rte_trace_ API. I will add UT and performance test cases to meson UT in v7. > > - Running with the traces on and checking against a reference (but > ignoring the timestamps) would be nice. > I did not think this through. Probably we need to install the babeltrace app in CI to check the sanity of the trace. I think, we can take it up later. > > > > > > 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 > > > > Maybe summarize this in doc/guides/prog_guide/trace_lib.rst: > > the trace function and its name to be similar. However, it is recommended to > have a similar name for a better naming convention. > > +The special macro ``RTE_TRACE_POINT_REGISTER_SELECT`` must be defined before > +including the header for the tracepoint registration to work properly. > + > The user must register the tracepoint before the ``rte_eal_init`` invocation. > The user can use the ``RTE_INIT`` construction scheme to achieve the same. Yes. I planned to add the same in .rst based on Thomas's comment. > > > -- > David Marchand >