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 25EC4A2E1B for ; Thu, 5 Sep 2019 09:13:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 904441EDAF; Thu, 5 Sep 2019 09:13:56 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id CC1B72BD8 for ; Thu, 5 Sep 2019 09:13:54 +0200 (CEST) Received: from mail-vs1-f72.google.com (mail-vs1-f72.google.com [209.85.217.72]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1983A2EF168 for ; Thu, 5 Sep 2019 07:13:54 +0000 (UTC) Received: by mail-vs1-f72.google.com with SMTP id e10so59907vsq.14 for ; Thu, 05 Sep 2019 00:13:54 -0700 (PDT) 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=H9PQm+kIyzAnUFnkKYpOYVJ7auNR7LkQ8HV6nQf1MhA=; b=HzAe1CzpdEZ3Q9yI82kLe1rLo3F6EWV4xBhSTm3IcdKte7ijDagnOAcUIypwqfCh6V ZZQpLNaWD0aH9gg38JmOQU6GDGB5i0rHm3YCUf/MjdqBoeWXBRb2xwi8A7B0QRb4URiV jvf+7z5iHMe/IfTcku15jExaxT4j6hXU3aPKZw/hiViplmOhF9RJgBvE+IaFD3+R6Da8 IUpqbRuoF9azob1CjdquUP07ZS/iXUBgAtDo8ArSxr5dOLj+NNKkTFkmim9ND2/waxOt aMKS6Q5goHOvSSvRKTlduCnoAnK86ODHhtBlIzs/6ekALYEqYtY6lo8COvB5V4AAkw+4 aNXQ== X-Gm-Message-State: APjAAAVG+dbWKtH0g5XVbnyyG/OXke+g0Hk2QkGGbsoglVmht9vWlDzK 9nVpwumpHgYpCPougCoLjzLREdlcQB4fC20IYmXjxcWif1dLZDEwNI9fzMdKpqlP5BQ7ZOLaJ93 1csOJ1xCTo1BeHtuVTQ8= X-Received: by 2002:a67:a44f:: with SMTP id p15mr976959vsh.105.1567667633391; Thu, 05 Sep 2019 00:13:53 -0700 (PDT) X-Google-Smtp-Source: APXvYqxl1mfSVxGDcY8QLCiqBx0DMzj4H8u8n6DZRbzzFsf1bvSUvPM1sNr8MnA+1nLuwj7r6ydrpOcfKUCX2JO8p+g= X-Received: by 2002:a67:a44f:: with SMTP id p15mr976941vsh.105.1567667633087; Thu, 05 Sep 2019 00:13:53 -0700 (PDT) MIME-Version: 1.0 References: <1566214919-32250-1-git-send-email-david.marchand@redhat.com> <2169155.oR67J0XmJ6@xps> <16cfddb8998.27dc.d74a604c1342fc6d922a87b3e5a1c1e3@solarflare.com> <4083300.tcgOVcdeWQ@xps> <5b3f331c-4011-63bf-3f38-44b160a6be66@solarflare.com> In-Reply-To: <5b3f331c-4011-63bf-3f38-44b160a6be66@solarflare.com> From: David Marchand Date: Thu, 5 Sep 2019 09:13:41 +0200 Message-ID: To: Andrew Rybchenko Cc: Thomas Monjalon , Ferruh Yigit , dev , Olivier Matz Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH 02/11] log: define logtype register wrapper for drivers 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, Sep 5, 2019 at 8:29 AM Andrew Rybchenko wrote: > > On 9/4/19 11:44 PM, Thomas Monjalon wrote: > > 04/09/2019 21:58, Andrew Rybchenko: > > On September 4, 2019 22:42:12 Thomas Monjalon wrote: > > 04/09/2019 21:21, Andrew Rybchenko: > > On 9/4/19 8:45 PM, Thomas Monjalon wrote: > > 03/09/2019 10:47, Ferruh Yigit: > > On 9/3/2019 9:06 AM, David Marchand wrote: > > On Mon, Sep 2, 2019 at 4:29 PM Ferruh Yigit wrote: > > On 8/19/2019 12:41 PM, David Marchand wrote: > > The function rte_log_register_type_and_pick_level() fills a gap for > dynamically loaded code (especially drivers) who would not pick up > the log level passed at startup. > > > Let's promote it to stable and export it for use by drivers via > a wrapper. > > > Signed-off-by: David Marchand > --- > > [...] > > /** > - * @warning > - * @b EXPERIMENTAL: this API may change without prior notice > - * > * Register a dynamic log type and try to pick its level from EAL options > > [...] > > -__rte_experimental > int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def); > > +1 to remove experimental from the API. > > I am not sure about this function API. > Why we combined register and level setting in one function? > > See [1] > > [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026 > > Sorry, it does not explain why we mix both operations in one function. > > Exactly to have one function instead of repeating two function calls > everywhere. Log level should be set when log type is registered. Yes, it is > possible to factor out a function just to pick log level, but I'm not sure > it makes sense separately. > > In fact may be it makes sense to substitute just register with this one > (I.e. remove simple register from piblic API and do not highlight that > level is picked up in function name). > > Given that we use it in a macro, we could keep two separate functions > for logtype register and minimum log level (note that "minimum" is > currently a missing word in these functions). > Anyway, we will always use the single macro in our libraries. > > > I don't understand why "minimum" is missing. It assigns level specified > for the pattern if it matches, or use level_def if no match. No level > comparison, last match wins. If "minimum" refers to minimum logged > level, it matches rte_log_set_level() which has no "minimum" as well > in neither name nor description. Moreover, EMERG is smaller than > DEBUG and if we treat log levels as numbers, it sets maximum level > which is logged. > > There are two usages right now without a macro, but it is not that > important. What I'm trying to say is that rte_log_register() plus setting > default after register is too error prone. It is the source of the bug > which we tried to workaround introducing the function. > > If user sets log level (using eal command-line options), no special > efforts should be required to assign it on register. So, I think register > should always do it. The function name is not ideal since it is highlights > details which are not really interesting. It is logging support details that > log levels may be set before log types registration. > > if we accept DPDK-wide default, it makes level_def parameter > unnecessary and the functionality to pick level may be simply > moved to rte_log_register() (using helper internal function) and > I see no good reasons why we really need type-specific defaults. Well this would require to touch quite some components that are not aligned, but I can't disagree with you :-). I'll prepare a v2 and identify which drivers are not ready for this. I also have some additional changes in mind. This should prevent the log register from failing (and no need for the "fallback" stuff then). -- David Marchand