[PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[Stephen Hemminger]

Introduce a new dynamic logtype for mbuf related messages.
Since this is used in multiple files put one macro in mbuf_log.h

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/mbuf/mbuf_log.h          | 10 ++++++++++
 lib/mbuf/rte_mbuf.c          | 20 ++++++++++++--------
 lib/mbuf/rte_mbuf_dyn.c      | 14 +++++++-------
 lib/mbuf/rte_mbuf_pool_ops.c |  5 +++--
 4 files changed, 32 insertions(+), 17 deletions(-)
 create mode 100644 lib/mbuf/mbuf_log.h

diff --git a/lib/mbuf/mbuf_log.h b/lib/mbuf/mbuf_log.h
new file mode 100644
index 000000000000..fe97f338c9b7
--- /dev/null
+++ b/lib/mbuf/mbuf_log.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2014 Intel Corporation.
+ * Copyright 2014 6WIND S.A.
+ */
+
+extern int mbuf_logtype;
+
+#define MBUF_LOG(level, fmt, args...)			\
+	rte_log(RTE_LOG_ ## level, mbuf_logtype,	\
+		"%s(): " fmt "\n", __func__, ##args)
diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
index cfd8062f1e6a..64847afbf931 100644
--- a/lib/mbuf/rte_mbuf.c
+++ b/lib/mbuf/rte_mbuf.c
@@ -20,6 +20,8 @@
 #include <rte_errno.h>
 #include <rte_memcpy.h>
 
+#include "mbuf_log.h"
+
 /*
  * pktmbuf pool constructor, given as a callback function to
  * rte_mempool_create(), or called directly if using
@@ -227,8 +229,8 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 	int ret;
 
 	if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
-		RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
-			priv_size);
+		MBUF_LOG(ERR, "mbuf priv_size=%u is not aligned",
+			 priv_size);
 		rte_errno = EINVAL;
 		return NULL;
 	}
@@ -247,7 +249,7 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
 		mp_ops_name = rte_mbuf_best_mempool_ops();
 	ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
 	if (ret != 0) {
-		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
+		MBUF_LOG(ERR, "error setting mempool handler");
 		rte_mempool_free(mp);
 		rte_errno = -ret;
 		return NULL;
@@ -293,7 +295,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
 	int ret;
 
 	if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
-		RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
+		MBUF_LOG(ERR, "mbuf priv_size=%u is not aligned",
 			priv_size);
 		rte_errno = EINVAL;
 		return NULL;
@@ -303,12 +305,12 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
 		const struct rte_pktmbuf_extmem *extm = ext_mem + i;
 
 		if (!extm->elt_size || !extm->buf_len || !extm->buf_ptr) {
-			RTE_LOG(ERR, MBUF, "invalid extmem descriptor\n");
+			MBUF_LOG(ERR, "invalid extmem descriptor");
 			rte_errno = EINVAL;
 			return NULL;
 		}
 		if (data_room_size > extm->elt_size) {
-			RTE_LOG(ERR, MBUF, "ext elt_size=%u is too small\n",
+			MBUF_LOG(ERR, "ext elt_size=%u is too small",
 				priv_size);
 			rte_errno = EINVAL;
 			return NULL;
@@ -317,7 +319,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
 	}
 	/* Check whether enough external memory provided. */
 	if (n_elts < n) {
-		RTE_LOG(ERR, MBUF, "not enough extmem\n");
+		MBUF_LOG(ERR, "not enough extmem");
 		rte_errno = ENOMEM;
 		return NULL;
 	}
@@ -338,7 +340,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
 	mp_ops_name = rte_mbuf_best_mempool_ops();
 	ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
 	if (ret != 0) {
-		RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
+		MBUF_LOG(ERR, "error setting mempool handler");
 		rte_mempool_free(mp);
 		rte_errno = -ret;
 		return NULL;
@@ -936,3 +938,5 @@ rte_get_tx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
 
 	return 0;
 }
+
+RTE_LOG_REGISTER(mbuf_logtype, "lib/mbuf", INFO);
diff --git a/lib/mbuf/rte_mbuf_dyn.c b/lib/mbuf/rte_mbuf_dyn.c
index 35839e938cc5..d8da912c8df0 100644
--- a/lib/mbuf/rte_mbuf_dyn.c
+++ b/lib/mbuf/rte_mbuf_dyn.c
@@ -17,6 +17,8 @@
 #include <rte_mbuf.h>
 #include <rte_mbuf_dyn.h>
 
+#include "mbuf_log.h"
+
 #define RTE_MBUF_DYN_MZNAME "rte_mbuf_dyn"
 
 struct mbuf_dynfield_elt {
@@ -116,7 +118,7 @@ init_shared_mem(void)
 		mz = rte_memzone_lookup(RTE_MBUF_DYN_MZNAME);
 	}
 	if (mz == NULL) {
-		RTE_LOG(ERR, MBUF, "Failed to get mbuf dyn shared memory\n");
+		MBUF_LOG(ERR, "Failed to get mbuf dyn shared memory");
 		return -1;
 	}
 
@@ -315,7 +317,7 @@ __rte_mbuf_dynfield_register_offset(const struct rte_mbuf_dynfield *params,
 		shm->free_space[i] = 0;
 	process_score();
 
-	RTE_LOG(DEBUG, MBUF, "Registered dynamic field %s (sz=%zu, al=%zu, fl=0x%x) -> %zd\n",
+	MBUF_LOG(DEBUG,"Registered dynamic field %s (sz=%zu, al=%zu, fl=0x%x) -> %zd",
 		params->name, params->size, params->align, params->flags,
 		offset);
 
@@ -489,7 +491,7 @@ __rte_mbuf_dynflag_register_bitnum(const struct rte_mbuf_dynflag *params,
 
 	shm->free_flags &= ~(1ULL << bitnum);
 
-	RTE_LOG(DEBUG, MBUF, "Registered dynamic flag %s (fl=0x%x) -> %u\n",
+	MBUF_LOG(DEBUG, "Registered dynamic flag %s (fl=0x%x) -> %u",
 		params->name, params->flags, bitnum);
 
 	return bitnum;
@@ -590,8 +592,7 @@ rte_mbuf_dyn_timestamp_register(int *field_offset, uint64_t *flag,
 
 	offset = rte_mbuf_dynfield_register(&field_desc);
 	if (offset < 0) {
-		RTE_LOG(ERR, MBUF,
-			"Failed to register mbuf field for timestamp\n");
+		MBUF_LOG(ERR, "Failed to register mbuf field for timestamp");
 		return -1;
 	}
 	if (field_offset != NULL)
@@ -600,8 +601,7 @@ rte_mbuf_dyn_timestamp_register(int *field_offset, uint64_t *flag,
 	strlcpy(flag_desc.name, flag_name, sizeof(flag_desc.name));
 	offset = rte_mbuf_dynflag_register(&flag_desc);
 	if (offset < 0) {
-		RTE_LOG(ERR, MBUF,
-			"Failed to register mbuf flag for %s timestamp\n",
+		MBUF_LOG(ERR, "Failed to register mbuf flag for %s timestamp",
 			direction);
 		return -1;
 	}
diff --git a/lib/mbuf/rte_mbuf_pool_ops.c b/lib/mbuf/rte_mbuf_pool_ops.c
index 4c91f4ce8569..8ec2710dc86d 100644
--- a/lib/mbuf/rte_mbuf_pool_ops.c
+++ b/lib/mbuf/rte_mbuf_pool_ops.c
@@ -8,6 +8,8 @@
 #include <rte_errno.h>
 #include <rte_mbuf_pool_ops.h>
 
+#include "mbuf_log.h"
+
 int
 rte_mbuf_set_platform_mempool_ops(const char *ops_name)
 {
@@ -31,8 +33,7 @@ rte_mbuf_set_platform_mempool_ops(const char *ops_name)
 		return 0;
 	}
 
-	RTE_LOG(ERR, MBUF,
-		"%s is already registered as platform mbuf pool ops\n",
+	MBUF_LOG(ERR, "%s is already registered as platform mbuf pool ops",
 		(char *)mz->addr);
 	return -EEXIST;
 }
-- 
2.39.0

RE: [PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 6 February 2023 18.40
> 
> Introduce a new dynamic logtype for mbuf related messages.
> Since this is used in multiple files put one macro in mbuf_log.h
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[David Marchand]

On Mon, Feb 6, 2023 at 6:39 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Introduce a new dynamic logtype for mbuf related messages.
> Since this is used in multiple files put one macro in mbuf_log.h
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/mbuf/mbuf_log.h          | 10 ++++++++++
>  lib/mbuf/rte_mbuf.c          | 20 ++++++++++++--------
>  lib/mbuf/rte_mbuf_dyn.c      | 14 +++++++-------
>  lib/mbuf/rte_mbuf_pool_ops.c |  5 +++--
>  4 files changed, 32 insertions(+), 17 deletions(-)
>  create mode 100644 lib/mbuf/mbuf_log.h
>
> diff --git a/lib/mbuf/mbuf_log.h b/lib/mbuf/mbuf_log.h
> new file mode 100644
> index 000000000000..fe97f338c9b7
> --- /dev/null
> +++ b/lib/mbuf/mbuf_log.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation.
> + * Copyright 2014 6WIND S.A.
> + */
> +
> +extern int mbuf_logtype;
> +
> +#define MBUF_LOG(level, fmt, args...)                  \
> +       rte_log(RTE_LOG_ ## level, mbuf_logtype,        \
> +               "%s(): " fmt "\n", __func__, ##args)
> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> index cfd8062f1e6a..64847afbf931 100644
> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -20,6 +20,8 @@
>  #include <rte_errno.h>
>  #include <rte_memcpy.h>
>
> +#include "mbuf_log.h"
> +
>  /*
>   * pktmbuf pool constructor, given as a callback function to
>   * rte_mempool_create(), or called directly if using
> @@ -227,8 +229,8 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
>         int ret;
>
>         if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
> -               RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
> -                       priv_size);
> +               MBUF_LOG(ERR, "mbuf priv_size=%u is not aligned",
> +                        priv_size);
>                 rte_errno = EINVAL;
>                 return NULL;
>         }
> @@ -247,7 +249,7 @@ rte_pktmbuf_pool_create_by_ops(const char *name, unsigned int n,
>                 mp_ops_name = rte_mbuf_best_mempool_ops();
>         ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
>         if (ret != 0) {
> -               RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> +               MBUF_LOG(ERR, "error setting mempool handler");
>                 rte_mempool_free(mp);
>                 rte_errno = -ret;
>                 return NULL;
> @@ -293,7 +295,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
>         int ret;
>
>         if (RTE_ALIGN(priv_size, RTE_MBUF_PRIV_ALIGN) != priv_size) {
> -               RTE_LOG(ERR, MBUF, "mbuf priv_size=%u is not aligned\n",
> +               MBUF_LOG(ERR, "mbuf priv_size=%u is not aligned",
>                         priv_size);
>                 rte_errno = EINVAL;
>                 return NULL;
> @@ -303,12 +305,12 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
>                 const struct rte_pktmbuf_extmem *extm = ext_mem + i;
>
>                 if (!extm->elt_size || !extm->buf_len || !extm->buf_ptr) {
> -                       RTE_LOG(ERR, MBUF, "invalid extmem descriptor\n");
> +                       MBUF_LOG(ERR, "invalid extmem descriptor");
>                         rte_errno = EINVAL;
>                         return NULL;
>                 }
>                 if (data_room_size > extm->elt_size) {
> -                       RTE_LOG(ERR, MBUF, "ext elt_size=%u is too small\n",
> +                       MBUF_LOG(ERR, "ext elt_size=%u is too small",
>                                 priv_size);
>                         rte_errno = EINVAL;
>                         return NULL;
> @@ -317,7 +319,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
>         }
>         /* Check whether enough external memory provided. */
>         if (n_elts < n) {
> -               RTE_LOG(ERR, MBUF, "not enough extmem\n");
> +               MBUF_LOG(ERR, "not enough extmem");
>                 rte_errno = ENOMEM;
>                 return NULL;
>         }
> @@ -338,7 +340,7 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
>         mp_ops_name = rte_mbuf_best_mempool_ops();
>         ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL);
>         if (ret != 0) {
> -               RTE_LOG(ERR, MBUF, "error setting mempool handler\n");
> +               MBUF_LOG(ERR, "error setting mempool handler");
>                 rte_mempool_free(mp);
>                 rte_errno = -ret;
>                 return NULL;
> @@ -936,3 +938,5 @@ rte_get_tx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
>
>         return 0;
>  }
> +
> +RTE_LOG_REGISTER(mbuf_logtype, "lib/mbuf", INFO);

?!

Why do we need a new naming convention?
Please use RTE_LOG_REGISTER_DEFAULT.

And we should invalidate/deprecate the use of RTE_LOGTYPE_MBUF + avoid
it is registered statically.
lib/eal/common/eal_common_log.c:        {RTE_LOGTYPE_MBUF,       "lib.mbuf"},
lib/eal/include/rte_log.h:#define RTE_LOGTYPE_MBUF      16 /**< Log
related to mbuf. */

These comments apply to the EFD patch too.


-- 
David Marchand

Re: [PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[Stephen Hemminger]

On Tue, 7 Feb 2023 09:34:13 +0100
David Marchand <david.marchand@redhat.com> wrote:

> Why do we need a new naming convention?
> Please use RTE_LOG_REGISTER_DEFAULT.

Should be use RTE_LOG_REGISTER_SUFFIX( ) with "mbuf"

Re: [PATCH] mbuf: replace RTE_LOGTYPE_MBUF with dynamic type

[David Marchand]

On Tue, Feb 7, 2023 at 5:19 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 7 Feb 2023 09:34:13 +0100
> David Marchand <david.marchand@redhat.com> wrote:
>
> > Why do we need a new naming convention?
> > Please use RTE_LOG_REGISTER_DEFAULT.
>
> Should be use RTE_LOG_REGISTER_SUFFIX( ) with "mbuf"

RTE_LOG_REGISTER_DEFAULT() hooks the logtype handle you pass to an
automatic RTE_LOG_DEFAULT_LOGTYPE name based on location in the tree
(generated by the build framework).

#define RTE_LOG_REGISTER_IMPL(type, name, level)                            \
int type;                                                                   \
RTE_INIT(__##type)                                                          \
{                                                                           \
        type = rte_log_register_type_and_pick_level(name, RTE_LOG_##level); \
        if (type < 0)                                                       \
                type = RTE_LOGTYPE_EAL;                                     \
}

#define RTE_LOG_REGISTER_DEFAULT(type, level) \
        RTE_LOG_REGISTER_IMPL(type, RTE_STR(RTE_LOG_DEFAULT_LOGTYPE), level)


When compiling:

[4/5] ccache cc -Ilib/librte_mbuf.a.p -Ilib -I../lib -Ilib/mbuf
-I../lib/mbuf -I. -I.. -Iconfig -I../config -Ilib/eal/include
-I../lib/eal/include -Ilib/eal/linux/include
-I../lib/eal/linux/include -Ilib/eal/x86/include
-I../lib/eal/x86/include -Ilib/eal/common -I../lib/eal/common
-Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs -Ilib/metrics
-I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/mempool
-I../lib/mempool -Ilib/ring -I../lib/ring -fdiagnostics-color=always
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -O2 -g
-include rte_config.h -Wcast-qual -Wdeprecated -Wformat
-Wformat-nonliteral -Wformat-security -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wold-style-definition
-Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef
-Wwrite-strings -Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -Wno-zero-length-bounds -D_GNU_SOURCE
-Wno-error=array-bounds -DRTE_LIBRTE_BBDEV_DEBUG
-DRTE_LIBRTE_EVENTDEV_DEBUG -DRTE_LIBRTE_IEEE1588 -fPIC -march=corei7
-DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation
-DRTE_LOG_DEFAULT_LOGTYPE=lib.mbuf -MD -MQ
lib/librte_mbuf.a.p/mbuf_rte_mbuf.c.o -MF
lib/librte_mbuf.a.p/mbuf_rte_mbuf.c.o.d -o
lib/librte_mbuf.a.p/mbuf_rte_mbuf.c.o -c ../lib/mbuf/rte_mbuf.c


RTE_LOG_REGISTER_SUFFIX() may be used to define "sub" logtypes, like
if you needed lib.mbuf.foo.


-- 
David Marchand