From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH 1/7] net/qede: reduce noise in debug logs Date: Mon, 12 Dec 2016 17:50:29 +0000 Message-ID: References: <1480733039-13046-1-git-send-email-harish.patil@qlogic.com> <91ac9846-530c-2dd3-4bd6-2c3b9143405b@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Dept-Eng DPDK Dev , "Mody, Rasesh" To: Harish Patil , "dev@dpdk.org" Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 39280388F for ; Mon, 12 Dec 2016 18:50:32 +0100 (CET) In-Reply-To: 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 12/12/2016 5:15 PM, Harish Patil wrote: > Hi Ferruh, > >> On 12/3/2016 2:43 AM, Harish Patil wrote: >>> From: Rasesh Mody >>> >>> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with >>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value >>> where each bit represent a particular submodule to debug. Also move >>> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO. >>> >>> Signed-off-by: Harish Patil >>> Signed-off-by: Rasesh Mody >>> --- >>> config/common_base | 2 +- >>> doc/guides/nics/qede.rst | 4 ++-- >>> drivers/net/qede/qede_ethdev.c | 4 ++-- >>> drivers/net/qede/qede_logs.h | 21 +++++---------------- >>> 4 files changed, 10 insertions(+), 21 deletions(-) >>> >>> diff --git a/config/common_base b/config/common_base >>> index 4bff83a..2ffd557 100644 >>> --- a/config/common_base >>> +++ b/config/common_base >>> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n >>> CONFIG_RTE_LIBRTE_QEDE_PMD=y >>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n >>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n >>> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n >>> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0 >>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n >>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n >>> #Provides abs path/name of the firmware file. >>> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst >>> index d22ecdd..ddf4248 100644 >>> --- a/doc/guides/nics/qede.rst >>> +++ b/doc/guides/nics/qede.rst >>> @@ -103,9 +103,9 @@ enabling debugging options may affect system >>> performance. >>> >>> Toggle display of generic debugging messages. >>> >>> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**) >>> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**) >> >> Does it make sense to document how DEBUG_VAL used? >> >> Also commit log says bitmapped value to enable/disable a particular >> submodule, you may want to document here which value enable/disable >> which submodule. >> >>> >>> - Toggle display of ecore related messages. >>> + Control driver debug verbosity using 32-bit bitmap flags. >>> >>> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**) >>> > > Not really, I think that would be too much. But if you think it really > helps then perhaps yes we can document. > Otherwise it is just for internal debugging. As a user of your driver, how can I know how to enable / disable a module log? I know VAL enables / disables them, but I don't know what modules exists and what values are required. If this is just for internal debugging and user not need to configure this one, does it needs to be a config option in configuration file? > > >> >> <...> >> >>> diff --git a/drivers/net/qede/qede_logs.h b/drivers/net/qede/qede_logs.h >>> index 45c4af0..08fdf04 100644 >>> --- a/drivers/net/qede/qede_logs.h >>> +++ b/drivers/net/qede/qede_logs.h >>> @@ -16,15 +16,18 @@ >>> (p_dev)->name ? (p_dev)->name : "", \ >>> ##__VA_ARGS__) >>> >>> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO >> >> Is "_INFO" carries any meaning in this config option, why not just >> RTE_LIBRTE_QEDE_DEBUG? > > INFO is used to mean just informational type of messages. > If you think it doesn’t make sense then I can rename it. I don't have a strong opinion, I think that _INFO is not adding value unless you have different config options per each level like _VERBOSE, _INFO, _NOTICE. But if you believe your users will benefit from it, it is your call. > >> >>> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \ >>> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\ >>> "[QEDE PMD: (%s)]%s:" fmt, \ >>> (p_dev)->name ? (p_dev)->name : "", \ >>> __func__, \ >>> ##__VA_ARGS__) >>> +#else >>> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0) >>> +#endif >>> >>> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO >>> - >>> #define DP_INFO(p_dev, fmt, ...) \ >>> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \ >>> "[%s:%d(%s)]" fmt, \ >>> @@ -33,10 +36,8 @@ >>> ##__VA_ARGS__) >>> #else >>> #define DP_INFO(p_dev, fmt, ...) do { } while (0) >>> - >>> #endif >>> >>> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER >> >> Are you sure you want to enable DP_VERBOSE, I guess most verbose log >> macro, by default? Perhaps may want to control it via >> RTE_LIBRTE_QEDE_DEBUG_INFO? > > DP_VERBOSE is enabled but it has a check: > if ((p_dev)->dp_module & module) > which controls what to print. > Here dp_module is controlled by CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL flag. > Hope it is clear. Clear thanks, I guess right now: DEBUG_VAL: enables verbose debug for selected module(s) DEBUG_INFO: Enables NOTICE and INFO level for ? (all modules?) ERR level is always enabled Again it is your call, but I think CONFIG_RTE_LIBRTE_QEDE_DEBUG can be used to enable/disable all debugs, and when enabled QEDE_DEBUG_VAL can select which modules to enable verbose debug ... >> >>> #define DP_VERBOSE(p_dev, module, fmt, ...) \ >>> do { \ >>> if ((p_dev)->dp_module & module) \ >>> @@ -46,9 +47,7 @@ >>> (p_dev)->name ? (p_dev)->name : "", \ >>> ##__VA_ARGS__); \ >>> } while (0) >>> -#else >>> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0) >>> -#endif >>> + >>> >> <...> >> >> >