* Re: [dpdk-dev] [PATCH v2] log: add API to check if a logtype can log in a given level
2020-03-03 18:18 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
@ 2020-03-04 7:50 ` Hyong Youb Kim (hyonkim)
2020-03-04 8:41 ` Hemant Agrawal (OSS)
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Hyong Youb Kim (hyonkim) @ 2020-03-04 7:50 UTC (permalink / raw)
To: Ferruh Yigit, Hemant Agrawal, Sachin Saxena, Fiona Trahe,
John Griffin, Deepak Kumar Jain, John Daley (johndale),
Alfredo Cardigliano, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Bernard Iremonger
Cc: dev, David Marchand, Thomas Monjalon, Stephen Hemminger
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, March 4, 2020 3:18 AM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> <sachin.saxena@nxp.com>; Fiona Trahe <fiona.trahe@intel.com>; John
> Griffin <john.griffin@intel.com>; Deepak Kumar Jain
> <deepak.k.jain@intel.com>; John Daley (johndale) <johndale@cisco.com>;
> Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>; Alfredo Cardigliano
> <cardigliano@ntop.org>; Matan Azrad <matan@mellanox.com>; Shahaf
> Shuler <shahafs@mellanox.com>; Viacheslav Ovsiienko
> <viacheslavo@mellanox.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; David Marchand
> <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Stephen Hemminger
> <stephen@networkplumber.org>
> Subject: [PATCH v2] log: add API to check if a logtype can log in a given level
>
> 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.yigit@intel.com>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Stephen Hemminger <stephen@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/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);
For enic,
Acked-by: Hyong Youb Kim <hyonkim@cisco.com>
Thanks..
-Hyong
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] log: add API to check if a logtype can log in a given level
2020-03-03 18:18 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2020-03-04 7:50 ` Hyong Youb Kim (hyonkim)
@ 2020-03-04 8:41 ` Hemant Agrawal (OSS)
2020-03-12 13:41 ` David Marchand
2020-03-13 14:51 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
3 siblings, 0 replies; 11+ messages in thread
From: Hemant Agrawal (OSS) @ 2020-03-04 8:41 UTC (permalink / raw)
To: Ferruh Yigit, Sachin Saxena, Fiona Trahe, John Griffin,
Deepak Kumar Jain, John Daley, Hyong Youb Kim,
Alfredo Cardigliano, Matan Azrad, Shahaf Shuler,
Viacheslav Ovsiienko, Bernard Iremonger
Cc: dev, David Marchand, Thomas Monjalon, Stephen Hemminger
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] log: add API to check if a logtype can log in a given level
2020-03-03 18:18 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2020-03-04 7:50 ` Hyong Youb Kim (hyonkim)
2020-03-04 8:41 ` Hemant Agrawal (OSS)
@ 2020-03-12 13:41 ` David Marchand
2020-03-12 14:52 ` Ferruh Yigit
2020-03-13 14:51 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
3 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2020-03-12 13:41 UTC (permalink / raw)
To: Ferruh Yigit, 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
On Tue, Mar 3, 2020 at 7:18 PM Ferruh Yigit <ferruh.yigit@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.yigit@intel.com>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Stephen Hemminger <stephen@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.
> + *
> + * @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?
Then we would not introduce a new API, but this means deferring this
patch to 20.11.
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
> /**
> * 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
>
--
David marchand
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v2] log: add API to check if a logtype can log in a given level
2020-03-12 13:41 ` David Marchand
@ 2020-03-12 14:52 ` Ferruh Yigit
0 siblings, 0 replies; 11+ messages in thread
From: Ferruh Yigit @ 2020-03-12 14:52 UTC (permalink / raw)
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
On 3/12/2020 1:41 PM, David Marchand wrote:
> On Tue, Mar 3, 2020 at 7:18 PM Ferruh Yigit <ferruh.yigit@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.yigit@intel.com>
>> ---
>> Cc: David Marchand <david.marchand@redhat.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Stephen Hemminger <stephen@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
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v3] log: add API to check if a logtype can log in a given level
2020-03-03 18:18 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
` (2 preceding siblings ...)
2020-03-12 13:41 ` David Marchand
@ 2020-03-13 14:51 ` Ferruh Yigit
2020-03-26 14:04 ` Andrzej Ostruszka
2020-03-27 10:23 ` David Marchand
3 siblings, 2 replies; 11+ messages in thread
From: Ferruh Yigit @ 2020-03-13 14:51 UTC (permalink / raw)
To: Hemant Agrawal, Sachin Saxena, Fiona Trahe, John Griffin,
Deepak Kumar Jain, John Daley, Hyong Youb Kim, Matan Azrad,
Shahaf Shuler, Viacheslav Ovsiienko, Bernard Iremonger
Cc: dev, Ferruh Yigit, David Marchand, Thomas Monjalon, Stephen Hemminger
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.yigit@intel.com>
Acked-by: Hyong Youb Kim <hyonkim@cisco.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
Cc: David Marchand <david.marchand@redhat.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Stephen Hemminger <stephen@networkplumber.org>
v2:
* Convert API return type to 'bool'. Removed custom definitions from
'ionic' PMD for this.
v3:
* Update function comment
* Use new function in `rte_vlog()`
---
drivers/bus/fslmc/fslmc_bus.c | 7 +------
drivers/common/qat/qat_logs.c | 7 ++-----
drivers/net/enic/enic_fm_flow.c | 2 +-
drivers/net/mlx5/mlx5_mr.c | 2 +-
lib/librte_eal/common/eal_common_log.c | 22 +++++++++++++++++---
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 ++-----
8 files changed, 43 insertions(+), 21 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)) {
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/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..7647a916e 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)
{
@@ -417,11 +435,9 @@ rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
FILE *f = rte_log_get_stream();
int ret;
- if (level > rte_logs.level)
- return 0;
if (logtype >= rte_logs.dynamic_types_len)
return -1;
- if (level > rte_logs.dynamic_types[logtype].loglevel)
+ if (!rte_log_can_log(logtype, level))
return 0;
/* save loglevel and logtype in a global per-lcore variable */
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 1bb0e6694..7f5cdbb39 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);
+/**
+ * For a given `logtype`, check if a log with `loglevel` can be printed.
+ *
+ * @param logtype
+ * The log type identifier
+ * @param loglevel
+ * 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 loglevel);
+
/**
* 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))
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [dpdk-dev] [PATCH v3] log: add API to check if a logtype can log in a given level
2020-03-13 14:51 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
@ 2020-03-26 14:04 ` Andrzej Ostruszka
2020-03-27 10:23 ` David Marchand
1 sibling, 0 replies; 11+ messages in thread
From: Andrzej Ostruszka @ 2020-03-26 14:04 UTC (permalink / raw)
To: dev
On 3/13/20 3:51 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 <ferruh.yigit@intel.com>
> Acked-by: Hyong Youb Kim <hyonkim@cisco.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
>
> v2:
> * Convert API return type to 'bool'. Removed custom definitions from
> 'ionic' PMD for this.
>
> v3:
> * Update function comment
> * Use new function in `rte_vlog()`
> ---
> drivers/bus/fslmc/fslmc_bus.c | 7 +------
> drivers/common/qat/qat_logs.c | 7 ++-----
> drivers/net/enic/enic_fm_flow.c | 2 +-
> drivers/net/mlx5/mlx5_mr.c | 2 +-
> lib/librte_eal/common/eal_common_log.c | 22 +++++++++++++++++---
> 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 ++-----
> 8 files changed, 43 insertions(+), 21 deletions(-)
Ferruh
I like this and would like use it for the next round of my patches. Is
there any timeline for that?
With regards
Andrzej Ostruszka
Reviewed-by: Andrzej Ostruszka <aostruszka@marvell.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v3] log: add API to check if a logtype can log in a given level
2020-03-13 14:51 ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2020-03-26 14:04 ` Andrzej Ostruszka
@ 2020-03-27 10:23 ` David Marchand
1 sibling, 0 replies; 11+ messages in thread
From: David Marchand @ 2020-03-27 10:23 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Hemant Agrawal, Sachin Saxena, Fiona Trahe, John Griffin,
Deepak Kumar Jain, John Daley, Hyong Youb Kim, Matan Azrad,
Shahaf Shuler, Viacheslav Ovsiienko, Bernard Iremonger, dev,
Thomas Monjalon, Stephen Hemminger, Andrzej Ostruszka
On Fri, Mar 13, 2020 at 3:52 PM Ferruh Yigit <ferruh.yigit@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.yigit@intel.com>
> Acked-by: Hyong Youb Kim <hyonkim@cisco.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Reviewed-by: Andrzej Ostruszka <aostruszka@marvell.com>
lgtm
Acked-by: David Marchand <david.marchand@redhat.com>
Applied, thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 11+ messages in thread