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 5200FA0565; Mon, 23 Mar 2020 15:29:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1D5991C068; Mon, 23 Mar 2020 15:29:28 +0100 (CET) Received: from mail-il1-f193.google.com (mail-il1-f193.google.com [209.85.166.193]) by dpdk.org (Postfix) with ESMTP id 0EF001C067 for ; Mon, 23 Mar 2020 15:29:26 +0100 (CET) Received: by mail-il1-f193.google.com with SMTP id o16so10697584ilm.2 for ; Mon, 23 Mar 2020 07:29:25 -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:content-transfer-encoding; bh=9xtiYO4Mn+wh3fIY65gZC4B0QaUP+b3ogNw+1PSI8RI=; b=XsqM8v4ujz621TMLmd1FBveelCQo4Ux2YdFuMmzbPLAUnzxz2rTnBxO7TNteXRbDUF QABXTjeSZguWYLpfruUqSFQS3v4yZ1wVHqBm9rPEVJD3odsgPIU+pYerIlqRjo8gjjqQ bmeGxi2uZvOMykGbHGRr7BMSHymKWo115aeX7ifdnbG6KYcvSTzqWMt5JU5mRvsVQsxY WM2Dn9I/qDtiFM0oshJ2xp9sOHNdH4wnaPCNecb6+821iR7HMCFK/dvw3PTaC6ZAiUgu cneEjvVHULdBuUDIlBfF8WPjHMNC37fse4Y46MYc+H4w3xvtItais3gbA35H3y0ussYg N+AQ== 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:content-transfer-encoding; bh=9xtiYO4Mn+wh3fIY65gZC4B0QaUP+b3ogNw+1PSI8RI=; b=tGjF4nxB9y0Ok5eBMfUW5LEOi/uGT8sLE9MO5QzEccJVomPUoFe1tM8vS7GzdC1ntj 9qIrMMd40NSMqFggV7XQyyqu/cYYFK9f5ORcxKh47SjmOezyvb5bscACAPCSAExx102W xzkZH7UQFwEmFlAMvz+DcpsgEbYesWkr3in7gKjs3xtfhj3qBL/7/5U+9tDIoi4DLk+r X5pcotO0PUYpsXs3cnwl9FQOuAYi4rKfRhOZ2VnolUOhv4Y11kaApymPl32pUAWy01UA 7lGZ5BPoQZt/qxI5fHE5Lm8s4t/OvZY70JZRS9vHUR7+mRwWHWWdQ9s0OoQjuiEAsmoV gKCg== X-Gm-Message-State: ANhLgQ1JlvHKUZZBggTuprOOX7/w6RrsLAU8npTDNyGdwqTAx9LDC0fn cYLRssOf4iumA0k5SF+zlEH8NWUxGtBkkaXnZjA= X-Google-Smtp-Source: ADFU+vu+WeweaS7UWlD8+3O/xoH290Cm7XouVEICagR7cLO5tfmaD/yCy68nmT59nluIqAjd8dbruJYnFKLqrJi28LE= X-Received: by 2002:a92:2804:: with SMTP id l4mr3146457ilf.130.1584973765009; Mon, 23 Mar 2020 07:29:25 -0700 (PDT) MIME-Version: 1.0 References: <20200318190241.3150971-1-jerinj@marvell.com> <20200318190241.3150971-3-jerinj@marvell.com> <8b318680-e291-5530-a52e-1f545e48e374@ericsson.com> In-Reply-To: <8b318680-e291-5530-a52e-1f545e48e374@ericsson.com> From: Jerin Jacob Date: Mon, 23 Mar 2020 19:59:08 +0530 Message-ID: To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Cc: "jerinj@marvell.com" , Thomas Monjalon , Bruce Richardson , John McNamara , Marko Kovacevic , Sunil Kumar Kori , "dev@dpdk.org" , "david.marchand@redhat.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v1 02/32] eal: define the public API for trace support 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 Thu, Mar 19, 2020 at 2:28 AM Mattias R=C3=B6nnblom wrote: > > On 2020-03-18 20:02, jerinj@marvell.com wrote: > > From: Jerin Jacob > > + > > +#include > > +#include > > + > > +/** The trace object. The trace APIs are based on this opaque object. = */ > > +typedef uint64_t *rte_trace_t; > > Wouldn't a forward-declared struct, with the definition hidden from the > user, be more appropriate? As a bonus, you'll get some type checking. > > "struct rte_trace;" here and "struct rte_trace*" in all the APIs. > "struct rte_trace { uint64_t val; }; in the implementation. Or just cast > it to a uint64_t *. OK. I will remove the typedef then. > > typdef:ing pointers is generally considered a no-no, at least if you > follow the Linux kernel coding conventions. > > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Enumerate trace mode operation. > > + */ > > +enum rte_trace_mode_e { > > + /** > > + * In this mode, When no space left in trace buffer, the subseque= nt > > + * events overwrite the old events in the trace buffer. > > + */ > > + RTE_TRACE_MODE_OVERWRITE, > > + /** > > + * In this mode, When no space left on trace buffer, the subseque= nt > > + * events shall not be recorded in the trace buffer. > > + */ > > + RTE_TRACE_MODE_DISCARD, > > +}; > > Have you considered having a blocking mode as well, where the thread > will just wait for space to be freed? The trace buffer is per thread. So there is no waiting. The new features can be added later as needed by extending the mode. > > Remove the "_e" from the name. "enum" already tells us it's an enumeratio= n. OK. I will remove it in v2. > > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Test if global trace is enabled. > > + * > > + * @return > > + * true if global trace is enabled, false otherwise. > > + */ > > +__rte_experimental > > +bool rte_trace_global_is_enabled(void); > > My impression is that DPDK does: > > > __rte_experimental bool > > rte_trace_global_is_enabled(void); > > > Now when I check the coding conventions, that's only for function > definition. Why declaration and definition should be different, I don't > know. I see two patterns.(Both cases __rte_experimental in a new line) __rte_experimental bool rte_trace_global_is_enabled(void); or __rte_experimental bool rte_trace_global_is_enabled(void); For the prototype case, I prefer a later option and for definition case the first option. If there are no specific opinions, I would like to stick to this model > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Set the global trace level. > > + * > > + * After this call, trace with a level lower or equal than the level > > + * passed as argument will be captured in the trace buffer. > > + * > > + * @param level > > + * Trace level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG = (8). > > + */ > > +__rte_experimental > > +void rte_trace_global_level_set(uint32_t level); > > uint32_t means a lot of levels. Yes. I did this to make compatibly with rte_log level datatype. > > + * > > + * @param trace > > + * The trace object. > > + * @return > > + * - Zero or positive: Mode encoded as enum rte_trace_mode_e. > > + * - (-EINVAL): Trace object is not registered. > > + */ > > +__rte_experimental > > +int rte_trace_mode_get(rte_trace_t trace); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Enable/Disable a set of tracepoints based on shell pattern. > Shell pattern means what I think is usually referred to as a glob? According, "man 3 fnmatch", it can be both. No preference here, The rte_log is using shell pattern as the comment so I thought of using the same. Thoughts? > > + * > > + * @param pattern > > + * The match pattern identifying the tracepoint. > > + * @param enable > > + * true to enable tracepoint, false to disable the tracepoint, upon= match. > > + * @param[out] found > > + * NULL value allowed, if not NULL, true if match found, false othe= rwise. > > + * @return > > + * - 0: Success. > > + * - (-ERANGE): Trace object is not registered. > > + */ > > +__rte_experimental > > +int rte_trace_pattern(const char *pattern, bool enable, bool *found); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Enable/Disable a set of tracepoints based on regular expression. > > + * > > + * @param regex > > + * A regular expression identifying the tracepoint. > > + * @param enable > > + * true to enable tracepoint, false to disable the tracepoint, upon= match. > > + * @param[out] found > > + * NULL value allowed, if not NULL, true if match found, false othe= rwise. > What's the reason of having this output parameter, as opposed to coding > the information into the return code? I thought making explicit is good . No strong opinion here. How about? 0: Success but no pattern found >0: Sucess and pattern found <0: error > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Search a trace object from its name. > > + * > > + * @param name > > + * The name of the tracepoint. > > + * @return > > + * The tracepoint object or NULL if not found. > > + */ > > +__rte_experimental > > +rte_trace_t rte_trace_from_name(const char *name); > > Would "rte_trace_by_name" be a better name? No strong preference. I will change to rte_trace_by_name(). > > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Dump the trace metadata to a file. > > + * > > + * @param f > > + * A pointer to a file for output > > + * @return > > + * - 0: Success. > > + * - <0 : Failure. > > + */ > > +__rte_experimental > > +int rte_trace_metadata_dump(FILE *f); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * Dump the trace subsystem status to a file. > > + * > > + * @param f > > + * A pointer to a file for output > > + */ > > +__rte_experimental > > +void rte_trace_dump(FILE *f); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Test if the trace datapath compile-time option is enabled. > > + * > > + * @return > > + * A positive value if trace datapath enabled, value zero otherwise. > Switch to a bool return type. I thought of avoiding "bool" in fastpath, I will change to bool if there is no performance impact. > > + */ > > +static __rte_always_inline int > > +rte_trace_is_dp_enabled(void) > > +{ > > +#ifdef RTE_ENABLE_TRACE_DP > > + return RTE_ENABLE_TRACE_DP; > > +#else > > + return 0; > > +#endif > > +} > > +