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 4B1E7A051C; Fri, 26 Jun 2020 14:06:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DFEF51BF0B; Fri, 26 Jun 2020 14:06:50 +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 BA96E1BEE7 for ; Fri, 26 Jun 2020 14:06:49 +0200 (CEST) Received: by mail-io1-f66.google.com with SMTP id e64so4593902iof.12 for ; Fri, 26 Jun 2020 05:06:49 -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=uSqps8iSWj26x0yej5zw8O+Q+No4gEyn+tAjql0pFxc=; b=fHopnkkmttowNDDVlcsAgNAnOSa+oihIRX6NYA3PIbZH4V7vrtc5Zi8lISFaI5+BlA bqZqDe2xhBFmsnMcccc0Rt9sl+lcesgrPeBWIhuZAf6k1ZD46ZURIaDk2/Z3/SaLlqOK mxF/fCCC4jqJ7Y8sgupCCUNoaxHhddzKfZydc+wz1meQzW26bi6g0HTnVgn5dA6m2Avf bN88NUcKD+8wr3v1W01uveG5ZVJOldYcfiZGoh3g630SbsOWdHgWiif4yb/0/mQKq3GR o1B1lE9VwNuwYf2lVy/kRNKt22IUTsyvpUxzwo9pmhO0sB7sXDRpotyD9wovdX/3c8+9 fNzw== 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=uSqps8iSWj26x0yej5zw8O+Q+No4gEyn+tAjql0pFxc=; b=GinnBVkIP0iPbu30RWH0Ypjm5BRzdl2/WG5GZ28/eHXlR8RlCfXLnqSk3eJFRkVXyy PTPqG1riGirFJng6xS8BFSA6SXRT+zDi0LoNaWgQdSfvR7HBDBLNdzvQW84NbHm+YE2x teQRS5RMxArhXFVDGPXT7orcKyl0S67g4cm/sMMVr52SDISFtjB40+Syeq/SY1NPq+4e lsklBahQlTBuj9xZDIaX31fZtx618OJ+QgqOvpN/X7mk0f8J2qz364zE8hszxE85wK9d MmQHo2BewSSgiDtvtDwNMBmtzRNf/lqLFWFZfXcP131jOxhnRZ9lDs6KUAYvu08jfvIq LpCA== X-Gm-Message-State: AOAM531DUSfrX7pS0Rvd7dC8OA7wsNfpnzM7K60pBbsU5V4KXFqDEp/N iGoYVIpj+fVHab4SJWK2A7bGUieKRsHT6Kxkojk= X-Google-Smtp-Source: ABdhPJxzNV6oEdnjlQICwxxN9CeUwOaFEy5aIjymT1Ztx6AZTQctqWJXcK3l7Rbu8ba5RzhNUWnqGuS0L2tMrcXK1xY= X-Received: by 2002:a6b:b344:: with SMTP id c65mr3156125iof.123.1593173208766; Fri, 26 Jun 2020 05:06:48 -0700 (PDT) MIME-Version: 1.0 References: <20200617063047.1555518-1-jerinj@marvell.com> <20200617063047.1555518-2-jerinj@marvell.com> <4becf100-7f0f-d051-a40c-3944e101381a@solarflare.com> <11e13bf9-8400-50de-4638-cdd1286915e4@solarflare.com> In-Reply-To: From: Jerin Jacob Date: Fri, 26 Jun 2020 17:36:32 +0530 Message-ID: To: David Marchand Cc: Andrew Rybchenko , Jerin Jacob Kollanukkaran , dev , Thomas Monjalon , Olivier Matz Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH 01/13] eal/log: introduce log register macro 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 Fri, Jun 26, 2020 at 5:13 PM David Marchand wrote: > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob wrote: > > > > Alternative is to keep variable declaration outside, > > > > as David suggested, and I tend to agree that it is a > > > > bit better. Macro name says 'register'. It is not > > > > 'declare and register'. Also it avoids static-vs-extern > > > > problem completely. The solution allows to keep the > > > > variable declaration untouched and put constructor (macro) > > > > at the end of fine where constructors typically reside. > > > > > > My only concern with that approach is that, We can not save a lot of > > > code duplication > > > with that scheme. ie. it is [1] vs [2]. We can change the MACRO name > > > accordingly if that is a concern. Any suggestions? > > > > > > Let me know your preference on [1] vs [2], I will stick with for the > > > next version. > > > > If there are no other comments, I change RTE_LOG_REGISTER to static version > > and RTE_LOG_REGISTER_EXTERN for a non-static version and send the next version. > > - Having a macro that does more than what its name tells is inconvenient. I agree. What could be that name if we want to declare and register? RTE_LOG_DECLARE_AND_REGISTER_EXTERN? > The trace framework differentiates declaration and registration. > Why merge things in the log framework? Not merging both at the functional level. it has just helper macro to avoid duplicating the code usage pattern. > > Saving one line is not worth it. It is one line per log registration. Please check the patch change status, at ,http://mails.dpdk.org/archives/dev/2020-June/170531.html It is: 122 files changed, 229 insertions(+), 1322 deletions(-) Why to add yet another 229 lines, if we can change the name. > > > - Having components set log levels at init time in the macro is a bug to me. > This has been worked around with > rte_log_register/rte_log_register_and_pick_level but the initial > problem is that rte_log_set_level* should only be called by the user. I agree with the below stuff, That's is not introduced by this patch. It is already there. Be it macro or no macro code. I think this patch helps to change to new scheme as it takes care of changing the registration part to commonplace so that we can set to the same level in the future if everyone agrees to it > > There should be a default level for all dpdk components (which means a > common interpretation of each level), then the user chooses which logs > he wants to see. > > At the moment, let's say I am looking at a live system, by default, we have: > > id 0: lib.eal, level is info > id 1: lib.malloc, level is info > id 2: lib.ring, level is info > id 3: lib.mempool, level is info > id 4: lib.timer, level is info > id 5: pmd, level is info > ... > id 32: lib.bbdev, level is notice > id 33: lib.bpf, level is info > id 34: bus.dpaa, level is notice > id 35: bus.fslmc, level is notice > id 36: bus.ifpga, level is notice > id 37: bus.vdev, level is notice > id 38: bus.vmbus, level is notice > id 39: lib.cfgfile, level is info > id 40: pmd.common.dpaax, level is error > ... > > I enable the logs for a component, set it to debug, I get my logs. > If now I want to reset the logs to the initial state, err... well > unless I took note of it, I don't know what the default level is. > But I should not have to care. > > Restarting dpdk sure is something you can do with testpmd but not with > real systems. > > > -- > David Marchand >