On 3/12/2020 1:41 PM, David Marchand wrote:
> On Tue, Mar 3, 2020 at 7:18 PM Ferruh Yigit <ferruh.yi...@intel.com> 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 <ferruh.yi...@intel.com>
>> ---
>> Cc: David Marchand <david.march...@redhat.com>
>> Cc: Thomas Monjalon <tho...@monjalon.net>
>> Cc: Stephen Hemminger <step...@networkplumber.org>
>>
>> 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 <stdint.h>
>> #include <stdio.h>
>> #include <stdarg.h>
>> +#include <stdbool.h>
>> #include <sys/queue.h>
>>
>> #include <rte_common.h>
>> @@ -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
>>
>
>
