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 0B7C2A0565; Mon, 23 Mar 2020 14:37:20 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E0DAA1C067; Mon, 23 Mar 2020 14:37:19 +0100 (CET) Received: from mail-il1-f196.google.com (mail-il1-f196.google.com [209.85.166.196]) by dpdk.org (Postfix) with ESMTP id 7356C1C05C for ; Mon, 23 Mar 2020 14:37:18 +0100 (CET) Received: by mail-il1-f196.google.com with SMTP id r5so8389337ilq.6 for ; Mon, 23 Mar 2020 06:37:18 -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=w/pY8buFFuPdNYeFhbhE96KzjQnEzYWbgVHSSwtVg5A=; b=Kqkic+5lYqiSHJgdNaSof4ySDDybjuTv3bt2VL+o9hy29yeruGyKa88aBYysC6BWHF 3qsIE+rdBKv0lpctAUXm26IZSRjQ86KyZyLm3fErchdB0x3GBHq/36E/OlAao81U8j4l gXMirq8B4T+lo98Axi/esBeZungBoF6PVzDF8yeNiRwpDiFIv9mHx5mwSmsxcNTmR3J8 RRrh10EDeHTcP99I3fpEjlpQE/+x2pgfs7FZOtH++iKX0+QKsv1tIeFVAT8Y7bam33hH kUClH27p+2RCVJF6MpOu3Z/LWtFyDr0KqqP1KpIt1XoJ0JHy6pvuX8OSMlTIYTtO7Tpn CuMA== 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=w/pY8buFFuPdNYeFhbhE96KzjQnEzYWbgVHSSwtVg5A=; b=rmo2vHFSdGUUKT8M2Ln/7ge+YakyBhixK9sXSS4aN4dmlpRqqo196l6+jsFyR8KrgA 7dpIBiL8msL0C+zG0Fhg9NjBA+X46UmY93aEK4wa3RHHiICXfTu2mHLgeAzievKUybdB 0q8L/5JFJ/5cI39EBPaGPNFt6QO3TOZXKucQyngYHpcIsbJ3cAiSq0TayPyaByU2HHu0 ZmuRRqYIRydEAGFGiU/Y1a9LviDRE2lt0DXjsTVvTNuBtemoCY1bTlX8h/V+TKu0q3Qr VIDAVLH5J/Vm4IQFgDcooS42u0KX7hwUMCQCRp4/+apIDWIYh3WkfRJXsqUIWxknuqxm pf4A== X-Gm-Message-State: ANhLgQ3H/Q4stjHa3HhnI4F8RPIzQg+k0C2e7E2Cx0iuUmsARr9HzSLp JhOhDxceXAHnkA7sJ51V4oo35ETdSDRTxtmDqqQ= X-Google-Smtp-Source: ADFU+vtqDfzCtRvM6GXZq0R2EE8GjTU9pLkqnnpvg2vYinwcx1IlnU59fUkK5xfBKxogcLay4+d0DEfbtfQLob5iZ5M= X-Received: by 2002:a05:6e02:90:: with SMTP id l16mr15196653ilm.294.1584970637612; Mon, 23 Mar 2020 06:37:17 -0700 (PDT) MIME-Version: 1.0 References: <20200318190241.3150971-1-jerinj@marvell.com> <20200318190241.3150971-4-jerinj@marvell.com> <5ce75c51-cce7-cee3-da04-b3dc51607864@ericsson.com> In-Reply-To: <5ce75c51-cce7-cee3-da04-b3dc51607864@ericsson.com> From: Jerin Jacob Date: Mon, 23 Mar 2020 19:07:01 +0530 Message-ID: To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Cc: "jerinj@marvell.com" , Thomas Monjalon , Sunil Kumar Kori , "dev@dpdk.org" , "bruce.richardson@intel.com" , "david.marchand@redhat.com" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v1 03/32] eal/trace: implement trace register API 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 3:33 PM Mattias R=C3=B6nnblom wrote: > > On 2020-03-18 20:02, jerinj@marvell.com wrote: > > From: Jerin Jacob > > > > +int > > +__rte_trace_point_register(rte_trace_t handle, const char *name, uint3= 2_t level, > > + void (*fn)(void)) > Maybe a more descriptive name than 'fn' would be in order. OK. I will change to "register_fn" in v2. > > +{ > > + char *field =3D RTE_PER_LCORE(ctf_field); > > + struct trace_point *tp; > > + uint16_t sz; > > + > > + /* Sanity checks of arguments */ > > + if (name =3D=3D NULL || fn =3D=3D NULL || handle =3D=3D NULL) { > > + trace_err("invalid arguments"); > > + rte_errno =3D EINVAL; goto fail; > > + } > > + > > + /* Sanity check of level */ > > + if (level > RTE_LOG_DEBUG || level > UINT8_MAX) { > > Consider a #define for the max level. If the type was uint8_t, you > wouldn't need to check max at all. The reason for keeping level as uint32_t to keep compatibility with rte_log level datatype. For some reason, rte_log defined level as uint32_t, So I thought of sticking to that so that we can migrate the rte_log to rte_trace in future if needed and also making semantics similar to rte_log. > > > + trace_err("invalid log level=3D%d", level); > > + rte_errno =3D EINVAL; goto fail; > > + > > + } > > + > > + /* Check the size of the trace point object */ > > + RTE_PER_LCORE(trace_point_sz) =3D 0; > > + RTE_PER_LCORE(ctf_count) =3D 0; > > + fn(); > > + if (RTE_PER_LCORE(trace_point_sz) =3D=3D 0) { > > + trace_err("missing rte_trace_emit_header() in register fn= "); > > + rte_errno =3D EBADF; goto fail; > > + } > > + > > + /* Is size overflowed */ > > + if (RTE_PER_LCORE(trace_point_sz) > UINT16_MAX) { > > + trace_err("trace point size overflowed"); > > + rte_errno =3D ENOSPC; goto fail; > > + } > > + > > + /* Are we running out of space to store trace points? */ > > + if (trace.nb_trace_points > UINT16_MAX) { > > + trace_err("trace point exceeds the max count"); > > + rte_errno =3D ENOSPC; goto fail; > > + } > > + > > + /* Get the size of the trace point */ > > + sz =3D RTE_PER_LCORE(trace_point_sz); > > + tp =3D calloc(1, sizeof(struct trace_point)); > Not rte_zmalloc()? Are secondary processes accessing this memory? This been called by the constructor at that time memory services are not enabled and it is for the per-process like rte_log scheme. > > + if (tp =3D=3D NULL) { > > + trace_err("fail to allocate trace point memory"); > > + rte_errno =3D ENOMEM; goto fail; > Missing newline. I will fix it in v2. > > + } > > + > > + /* Initialize the trace point */ > > + if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) { > > + trace_err("name is too long"); > > + rte_errno =3D E2BIG; > > + goto free; > > + } > > + > > + /* Copy the field data for future use */ > > + if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) = { > > + trace_err("CTF field size is too long"); > > + rte_errno =3D E2BIG; > > + goto free; > > + } > > + > > + /* Clear field memory for the next event */ > > + memset(field, 0, TRACE_CTF_FIELD_SIZE); > > + > > + /* Form the trace handle */ > > + *handle =3D sz; > > + *handle |=3D trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT; > > + *handle |=3D (uint64_t)level << __RTE_TRACE_FIELD_LEVEL_SHIFT; > If *handle would be a struct, you could use a bitfield instead, and much > simplify this code. I thought that initially, Two reasons why I did not do that 1) The flags have been used in fastpath, I prefer to work with flags in fastpath so that there is no performance impact using bitfields from the compiler _if any_. 2) In some of the places, I can simply operate on APIs like __atomic_and_fetch() with flags.