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 B7325A056D; Thu, 12 Mar 2020 15:53:08 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F39731BFA5; Thu, 12 Mar 2020 15:53:07 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 451FE2BE6 for ; Thu, 12 Mar 2020 15:53:05 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Mar 2020 07:52:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,545,1574150400"; d="scan'208";a="236857973" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.251.84.123]) ([10.251.84.123]) by orsmga008.jf.intel.com with ESMTP; 12 Mar 2020 07:52:48 -0700 To: David Marchand , Olivier Matz Cc: Hemant Agrawal , Sachin Saxena , Fiona Trahe , John Griffin , Deepak Kumar Jain , John Daley , Hyong Youb Kim , Alfredo Cardigliano , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Bernard Iremonger , dev , Thomas Monjalon , Stephen Hemminger References: <20200303132512.4138303-1-ferruh.yigit@intel.com> <20200303181817.3448334-1-ferruh.yigit@intel.com> From: Ferruh Yigit Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEAFiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl1meboFCQlupOoACgkQ+TPrQ98T YR9ACBAAv2tomhyxY0Tp9Up7mNGLfEdBu/7joB/vIdqMRv63ojkwr9orQq5V16V/25+JEAD0 60cKodBDM6HdUvqLHatS8fooWRueSXHKYwJ3vxyB2tWDyZrLzLI1jxEvunGodoIzUOtum0Ce gPynnfQCelXBja0BwLXJMplM6TY1wXX22ap0ZViC0m714U5U4LQpzjabtFtjT8qOUR6L7hfy YQ72PBuktGb00UR/N5UrR6GqB0x4W41aZBHXfUQnvWIMmmCrRUJX36hOTYBzh+x86ULgg7H2 1499tA4o6rvE13FiGccplBNWCAIroAe/G11rdoN5NBgYVXu++38gTa/MBmIt6zRi6ch15oLA Ln2vHOdqhrgDuxjhMpG2bpNE36DG/V9WWyWdIRlz3NYPCDM/S3anbHlhjStXHOz1uHOnerXM 1jEjcsvmj1vSyYoQMyRcRJmBZLrekvgZeh7nJzbPHxtth8M7AoqiZ/o/BpYU+0xZ+J5/szWZ aYxxmIRu5ejFf+Wn9s5eXNHmyqxBidpCWvcbKYDBnkw2+Y9E5YTpL0mS0dCCOlrO7gca27ux ybtbj84aaW1g0CfIlUnOtHgMCmz6zPXThb+A8H8j3O6qmPoVqT3qnq3Uhy6GOoH8Fdu2Vchh TWiF5yo+pvUagQP6LpslffufSnu+RKAagkj7/RSuZV25Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXWZ5wAUJB3FgggAKCRD5M+tD3xNhH2O+D/9OEz62YuJQLuIuOfL67eFTIB5/1+0j8Tsu o2psca1PUQ61SZJZOMl6VwNxpdvEaolVdrpnSxUF31kPEvR0Igy8HysQ11pj8AcgH0a9FrvU /8k2Roccd2ZIdpNLkirGFZR7LtRw41Kt1Jg+lafI0efkiHKMT/6D/P1EUp1RxOBNtWGV2hrd 0Yg9ds+VMphHHU69fDH02SwgpvXwG8Qm14Zi5WQ66R4CtTkHuYtA63sS17vMl8fDuTCtvfPF HzvdJLIhDYN3Mm1oMjKLlq4PUdYh68Fiwm+boJoBUFGuregJFlO3hM7uHBDhSEnXQr5mqpPM 6R/7Q5BjAxrwVBisH0yQGjsWlnysRWNfExAE2sRePSl0or9q19ddkRYltl6X4FDUXy2DTXa9 a+Fw4e1EvmcF3PjmTYs9IE3Vc64CRQXkhujcN4ZZh5lvOpU8WgyDxFq7bavFnSS6kx7Tk29/ wNJBp+cf9qsQxLbqhW5kfORuZGecus0TLcmpZEFKKjTJBK9gELRBB/zoN3j41hlEl7uTUXTI JQFLhpsFlEdKLujyvT/aCwP3XWT+B2uZDKrMAElF6ltpTxI53JYi22WO7NH7MR16Fhi4R6vh FHNBOkiAhUpoXRZXaCR6+X4qwA8CwHGqHRBfYFSU/Ulq1ZLR+S3hNj2mbnSx0lBs1eEqe2vh cA== Message-ID: Date: Thu, 12 Mar 2020 14:52:48 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2] log: add API to check if a logtype can log in a given level 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 3/12/2020 1:41 PM, David Marchand wrote: > On Tue, Mar 3, 2020 at 7:18 PM Ferruh Yigit wrote: >> >> This is a helper function in case components would like to do more work >> than just logging a message based on log level, like for example >> collecting some stats if the log type is DEBUG etc.. >> >> A few existing relevant usage converted to this new API. >> >> Signed-off-by: Ferruh Yigit >> --- >> Cc: David Marchand >> Cc: Thomas Monjalon >> Cc: Stephen Hemminger >> >> v2: >> * Convert API return type to 'bool'. Removed custom definitions from >> 'ionic' PMD for this. >> --- >> drivers/bus/fslmc/fslmc_bus.c | 7 +------ >> drivers/common/qat/qat_logs.c | 7 ++----- >> drivers/net/enic/enic_fm_flow.c | 2 +- >> drivers/net/ionic/ionic_osdep.h | 6 ------ >> drivers/net/mlx5/mlx5_mr.c | 2 +- >> lib/librte_eal/common/eal_common_log.c | 18 ++++++++++++++++++ >> lib/librte_eal/common/include/rte_log.h | 14 ++++++++++++++ >> lib/librte_eal/rte_eal_version.map | 3 +++ >> lib/librte_flow_classify/rte_flow_classify.c | 7 ++----- >> 9 files changed, 42 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c >> index b3e964aa9..afbd82e8d 100644 >> --- a/drivers/bus/fslmc/fslmc_bus.c >> +++ b/drivers/bus/fslmc/fslmc_bus.c >> @@ -115,14 +115,9 @@ static void >> dump_device_list(void) >> { >> struct rte_dpaa2_device *dev; >> - uint32_t global_log_level; >> - int local_log_level; >> >> /* Only if the log level has been set to Debugging, print list */ >> - global_log_level = rte_log_get_global_level(); >> - local_log_level = rte_log_get_level(dpaa2_logtype_bus); >> - if (global_log_level == RTE_LOG_DEBUG || >> - local_log_level == RTE_LOG_DEBUG) { >> + if (rte_log_can_log(dpaa2_logtype_bus, RTE_LOG_DEBUG)) { > > Here, this is a change in behavior, but it aligns this driver to the > rest of dpdk, and we got a ack from Hemant. > > >> DPAA2_BUS_LOG(DEBUG, "List of devices scanned on bus:"); >> TAILQ_FOREACH(dev, &rte_fslmc_bus.device_list, next) { >> DPAA2_BUS_LOG(DEBUG, "\t\t%s", dev->device.name); >> diff --git a/drivers/common/qat/qat_logs.c b/drivers/common/qat/qat_logs.c >> index f97aba19d..dfd0cbe5d 100644 >> --- a/drivers/common/qat/qat_logs.c >> +++ b/drivers/common/qat/qat_logs.c >> @@ -14,12 +14,9 @@ int >> qat_hexdump_log(uint32_t level, uint32_t logtype, const char *title, >> const void *buf, unsigned int len) >> { >> - if (level > rte_log_get_global_level()) >> - return 0; >> - if (level > (uint32_t)(rte_log_get_level(logtype))) >> - return 0; >> + if (rte_log_can_log(logtype, level)) >> + rte_hexdump(rte_log_get_stream(), title, buf, len); >> >> - rte_hexdump(rte_log_get_stream(), title, buf, len); >> return 0; >> } >> >> diff --git a/drivers/net/enic/enic_fm_flow.c b/drivers/net/enic/enic_fm_flow.c >> index c0ddfe9ba..d815f369e 100644 >> --- a/drivers/net/enic/enic_fm_flow.c >> +++ b/drivers/net/enic/enic_fm_flow.c >> @@ -1504,7 +1504,7 @@ enic_fm_dump_tcam_entry(const struct fm_tcam_match_entry *fm_match, >> const struct fm_action *fm_action, >> uint8_t ingress) >> { >> - if (rte_log_get_level(enic_pmd_logtype) < (int)RTE_LOG_DEBUG) >> + if (!rte_log_can_log(enic_pmd_logtype, RTE_LOG_DEBUG)) >> return; >> enic_fm_dump_tcam_match(fm_match, ingress); >> enic_fm_dump_tcam_actions(fm_action); >> diff --git a/drivers/net/ionic/ionic_osdep.h b/drivers/net/ionic/ionic_osdep.h >> index ecdbc24e6..e04bb8f65 100644 >> --- a/drivers/net/ionic/ionic_osdep.h >> +++ b/drivers/net/ionic/ionic_osdep.h >> @@ -44,12 +44,6 @@ typedef uint16_t __le16; >> typedef uint32_t __le32; >> typedef uint64_t __le64; >> >> -#ifndef __cplusplus >> -typedef uint8_t bool; >> -#define false 0 >> -#define true 1 >> -#endif >> - >> static inline uint32_t div_round_up(uint32_t n, uint32_t d) >> { >> return (n + d - 1) / d; >> diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c >> index cb97c876e..6aa578646 100644 >> --- a/drivers/net/mlx5/mlx5_mr.c >> +++ b/drivers/net/mlx5/mlx5_mr.c >> @@ -1597,7 +1597,7 @@ mlx5_mr_release(struct mlx5_ibv_shared *sh) >> { >> struct mlx5_mr *mr_next; >> >> - if (rte_log_get_level(mlx5_logtype) == RTE_LOG_DEBUG) >> + if (rte_log_can_log(mlx5_logtype, RTE_LOG_DEBUG)) >> mlx5_mr_dump_dev(sh); >> rte_rwlock_write_lock(&sh->mr.rwlock); >> /* Detach from MR list and move to free list. */ >> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c >> index c0efd5214..679e7326f 100644 >> --- a/lib/librte_eal/common/eal_common_log.c >> +++ b/lib/librte_eal/common/eal_common_log.c >> @@ -112,6 +112,24 @@ rte_log_get_level(uint32_t type) >> return rte_logs.dynamic_types[type].loglevel; >> } >> >> +bool >> +rte_log_can_log(uint32_t logtype, uint32_t level) >> +{ >> + int log_level; >> + >> + if (level > rte_log_get_global_level()) >> + return false; >> + >> + log_level = rte_log_get_level(logtype); >> + if (log_level < 0) >> + return false; >> + >> + if (level > (uint32_t)log_level) >> + return false; >> + >> + return true; >> +} >> + >> int >> rte_log_set_level(uint32_t type, uint32_t level) >> { >> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h >> index 1bb0e6694..2d1bae5c4 100644 >> --- a/lib/librte_eal/common/include/rte_log.h >> +++ b/lib/librte_eal/common/include/rte_log.h >> @@ -20,6 +20,7 @@ extern "C" { >> #include >> #include >> #include >> +#include >> #include >> >> #include >> @@ -143,6 +144,19 @@ uint32_t rte_log_get_global_level(void); >> */ >> int rte_log_get_level(uint32_t logtype); >> >> +/** >> + * Check if a given `level` can be printed by a given `logtype` > > The description is a bit odd to me. > A "level" is not printed. > A "logtype" does not print. Agree, what about following, is it more clear: " For a given `logtype`, check if log with `loglevel` will be printed or not. " > > >> + * >> + * @param logtype >> + * The log type identifier >> + * @param level >> + * Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8). >> + * @return >> + * Returns 'true' if log can be printed and 'false' if it can't. >> + */ >> +__rte_experimental >> +bool rte_log_can_log(uint32_t logtype, uint32_t level); >> + > > Would it make sense to actually change rte_log_get_level() to always > take the global level into account? Not sure, it can make 'rte_log_get_level()' confusing > Then we would not introduce a new API, but this means deferring this > patch to 20.11. Apart from global level check, I think it is good to have this helper instead of applications getting log level and comparing themselves, this API simplifies it. > > > If we go with this helper, we can replace the check we have in rte_vlog: > https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_common_log.c#n420 Right, why not, I can update in next version. > > >> /** >> * Set the log level for a given type based on shell pattern. >> * >> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map >> index 6cf507068..f9ede5b41 100644 >> --- a/lib/librte_eal/rte_eal_version.map >> +++ b/lib/librte_eal/rte_eal_version.map >> @@ -335,4 +335,7 @@ EXPERIMENTAL { >> >> # added in 20.02 >> rte_thread_is_intr; >> + >> + # added in 20.05 >> + rte_log_can_log; >> }; >> diff --git a/lib/librte_flow_classify/rte_flow_classify.c b/lib/librte_flow_classify/rte_flow_classify.c >> index 5ff585803..6022064d8 100644 >> --- a/lib/librte_flow_classify/rte_flow_classify.c >> +++ b/lib/librte_flow_classify/rte_flow_classify.c >> @@ -417,7 +417,6 @@ static struct rte_flow_classify_rule * >> allocate_acl_ipv4_5tuple_rule(struct rte_flow_classifier *cls) >> { >> struct rte_flow_classify_rule *rule; >> - int log_level; >> >> rule = malloc(sizeof(struct rte_flow_classify_rule)); >> if (!rule) >> @@ -466,9 +465,7 @@ allocate_acl_ipv4_5tuple_rule(struct rte_flow_classifier *cls) >> cls->ntuple_filter.dst_port_mask; >> rule->rules.u.ipv4_5tuple.dst_port = cls->ntuple_filter.dst_port; >> >> - log_level = rte_log_get_level(librte_flow_classify_logtype); >> - >> - if (log_level == RTE_LOG_DEBUG) >> + if (rte_log_can_log(librte_flow_classify_logtype, RTE_LOG_DEBUG)) > > Idem, behavior change, but looks sane. > > >> print_acl_ipv4_key_add(&rule->u.key.key_add); >> >> /* key delete values */ >> @@ -476,7 +473,7 @@ allocate_acl_ipv4_5tuple_rule(struct rte_flow_classifier *cls) >> &rule->u.key.key_add.field_value[PROTO_FIELD_IPV4], >> NUM_FIELDS_IPV4 * sizeof(struct rte_acl_field)); >> >> - if (log_level == RTE_LOG_DEBUG) >> + if (rte_log_can_log(librte_flow_classify_logtype, RTE_LOG_DEBUG)) >> print_acl_ipv4_key_delete(&rule->u.key.key_del); >> >> return rule; >> -- >> 2.24.1 >> > >