From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E1198423B5; Thu, 12 Jan 2023 13:38:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 86B2D42D22; Thu, 12 Jan 2023 13:38:55 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id ABBDD40E25 for ; Thu, 12 Jan 2023 13:38:54 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob Date: Thu, 12 Jan 2023 13:38:51 +0100 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87669@smartserver.smartshare.dk> In-Reply-To: <20230112112140.807233-2-adwivedi@marvell.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 1/6] eal: trace: add trace point emit for blob Thread-Index: AdkmeDvTfJUVehpGSl238It1opuBswABxvuw References: <20221222063306.3383695-1-adwivedi@marvell.com> <20230112112140.807233-1-adwivedi@marvell.com> <20230112112140.807233-2-adwivedi@marvell.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Ankur Dwivedi" , Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Ankur Dwivedi [mailto:adwivedi@marvell.com] > Sent: Thursday, 12 January 2023 12.22 >=20 > Adds a trace point emit function for emitting a blob. The maximum blob > bytes which can be captured is maximum value contained in uint16_t, > which is 65535. >=20 > Also adds test case for emit array tracepoint function. >=20 > Signed-off-by: Ankur Dwivedi > --- Excellent, thank you. [...] > +#define rte_trace_point_emit_blob(in, len) \ > +do { \ > + if (unlikely(in =3D=3D NULL)) \ > + return; \ > + __rte_trace_point_emit(len, uint16_t); \ > + memcpy(mem, in, len); \ > + mem =3D RTE_PTR_ADD(mem, len); \ > +} while (0) I am somewhat unsure about my concerns here, so please forgive me if = they are invalid... Is rte_trace_point_emit_blob() always called with "len" being a = variable, then this is OK. If "len" can be a non-constant formula (e.g. len++), you need a = temporary variable: #define rte_trace_point_emit_blob(in, len) \ do { \ uint16_t _len =3D len; \ if (unlikely(in =3D=3D NULL)) \ return; \ __rte_trace_point_emit(_len, uint16_t); \ memcpy(mem, in, _len); \ mem =3D RTE_PTR_ADD(_mem, _len); \ } while (0) But I don't think this can ever happen. However, if "len" can be a constant (e.g. 6), does = __rte_trace_point_emit(len, uint16_t) accept it? (The = __rte_trace_point_emit() macro is shown below.) A constant has no = pointer to it (i.e. &6 does not exist). Looking deeper at it, I'm afraid this question can be generalized to all = the existing macros/functions calling __rte_trace_point_emit(). And now that I have hijacked your patch with a generalized question, I = wonder if the existing __rte_trace_point_emit() has a bug? It uses = sizeof(in), but I think it should use sizeof(type). It looks like this: #define __rte_trace_point_emit(in, type) \ do { \ memcpy(mem, &(in), sizeof(in)); \ mem =3D RTE_PTR_ADD(mem, sizeof(in)); \ } while (0) Alternatively, __rte_trace_point_emit() should = RTE_BUILD_BUG_ON(typeof(in) !=3D type). If my concerns above are invalid, then: Acked-by: Morten Br=F8rup