[v2,1/2] doc/contributing: provide coding details for dynamic logging

Message ID 20230620170728.74117-2-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Improve docs on getting info on running process |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson June 20, 2023, 5:07 p.m. UTC
  While the section on dynamic logging in the contributors guide covered
the details of the logging naming scheme, it failed to cover exactly how
the component developer, i.e. the contributor, could actually use
dynamic logging in their component.

Fix this by splitting the details of the naming scheme into a separate
subsection, and adding to the introduction on logging, a recommendation
(and example) to use RTE_LOG_REGISTER_DEFAULT.

Similarly, when discussing specialization, include a reference to the
RTE_LOG_REGISTER_SUFFIX macro.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/contributing/coding_style.rst | 17 +++++++++++++++++
 lib/cfgfile/rte_cfgfile.c                |  2 ++
 2 files changed, 19 insertions(+)
  

Comments

fengchengwen July 4, 2023, 6:20 a.m. UTC | #1
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2023/6/21 1:07, Bruce Richardson wrote:
> While the section on dynamic logging in the contributors guide covered
> the details of the logging naming scheme, it failed to cover exactly how
> the component developer, i.e. the contributor, could actually use
> dynamic logging in their component.
> 
> Fix this by splitting the details of the naming scheme into a separate
> subsection, and adding to the introduction on logging, a recommendation
> (and example) to use RTE_LOG_REGISTER_DEFAULT.
> 
> Similarly, when discussing specialization, include a reference to the
> RTE_LOG_REGISTER_SUFFIX macro.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

...
  
David Marchand July 4, 2023, 7:46 a.m. UTC | #2
On Tue, Jun 20, 2023 at 7:08 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> While the section on dynamic logging in the contributors guide covered
> the details of the logging naming scheme, it failed to cover exactly how
> the component developer, i.e. the contributor, could actually use
> dynamic logging in their component.
>
> Fix this by splitting the details of the naming scheme into a separate
> subsection, and adding to the introduction on logging, a recommendation
> (and example) to use RTE_LOG_REGISTER_DEFAULT.
>
> Similarly, when discussing specialization, include a reference to the
> RTE_LOG_REGISTER_SUFFIX macro.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  doc/guides/contributing/coding_style.rst | 17 +++++++++++++++++
>  lib/cfgfile/rte_cfgfile.c                |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> index 89db6260cf..307c7deb9a 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst
> @@ -803,6 +803,21 @@ logging of a particular topic, the ``--log-level`` parameter can be provided
>  to EAL, which will change the log level. DPDK code can register topics,
>  which allows the user to adjust the log verbosity for that specific topic.
>
> +To register a library or driver for dynamic logging,

Nit: no need for a line break here, it can be fixed when applying.


> +using the standardized naming scheme described below,
> +use ``RTE_LOG_REGISTER_DEFAULT`` macro to define a log-type variable inside your component's main C file.
> +Thereafter, it is usual to define a macro or macros inside your component to make logging more convenient.
> +
> +For example, the ``rte_cfgfile`` library defines:
> +
> +.. literalinclude:: ../../../lib/cfgfile/rte_cfgfile.c
> +    :language: c
> +    :start-after: Setting up dynamic logging 8<
> +    :end-before: >8 End of setting up dynamic logging
> +
> +Dynamic Logging Naming Scheme
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
>  In general, the naming scheme is as follows: ``type.section.name``
>
>   * Type is the type of component, where ``lib``, ``pmd``, ``bus`` and ``user``
> @@ -837,6 +852,8 @@ A specialization looks like this:
>   * Initialization output: ``type.section.name.init``
>   * PF/VF mailbox output: ``type.section.name.mbox``
>
> +These specializations are created using the ``RTE_LOG_REGISTER_SUFFIX`` macro.
> +
>  A real world example is the i40e poll mode driver which exposes two
>  specializations, one for initialization ``pmd.net.i40e.init`` and the other for
>  the remaining driver logs ``pmd.net.i40e.driver``.
> diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
> index 9fa7d010ef..eefba6e408 100644
> --- a/lib/cfgfile/rte_cfgfile.c
> +++ b/lib/cfgfile/rte_cfgfile.c
> @@ -27,11 +27,13 @@ struct rte_cfgfile {
>         struct rte_cfgfile_section *sections;
>  };
>
> +/* Setting up dynamic logging 8< */
>  RTE_LOG_REGISTER_DEFAULT(cfgfile_logtype, INFO);
>
>  #define CFG_LOG(level, fmt, args...)                                   \
>         rte_log(RTE_LOG_ ## level, cfgfile_logtype, "%s(): " fmt "\n",  \
>                 __func__, ## args)
> +/* >8 End of setting up dynamic logging */
>
>  /** when we resize a file structure, how many extra entries
>   * for new sections do we add in */
> --
> 2.39.2
>

Thanks, this doc patch is a good addition in its current form.

Reviewed-by: David Marchand <david.marchand@redhat.com>


Stephen used a little trick with a #define RTE_LOGTYPE_$type
${type}_logtype so that RTE_LOG() is directly usable.
This makes conversion from static to dynamic logtypes rather easy.
Example: https://patchwork.dpdk.org/project/dpdk/patch/20230329234049.11071-5-stephen@networkplumber.org/
It could be something we advertise in this doc later.

Maybe some consistency cleanup (and guidance) would be good too, as
many components have close-yet-somewhat-different log formats (some
log the function name, others write the line number etc...).
  

Patch

diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index 89db6260cf..307c7deb9a 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -803,6 +803,21 @@  logging of a particular topic, the ``--log-level`` parameter can be provided
 to EAL, which will change the log level. DPDK code can register topics,
 which allows the user to adjust the log verbosity for that specific topic.
 
+To register a library or driver for dynamic logging,
+using the standardized naming scheme described below,
+use ``RTE_LOG_REGISTER_DEFAULT`` macro to define a log-type variable inside your component's main C file.
+Thereafter, it is usual to define a macro or macros inside your component to make logging more convenient.
+
+For example, the ``rte_cfgfile`` library defines:
+
+.. literalinclude:: ../../../lib/cfgfile/rte_cfgfile.c
+    :language: c
+    :start-after: Setting up dynamic logging 8<
+    :end-before: >8 End of setting up dynamic logging
+
+Dynamic Logging Naming Scheme
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 In general, the naming scheme is as follows: ``type.section.name``
 
  * Type is the type of component, where ``lib``, ``pmd``, ``bus`` and ``user``
@@ -837,6 +852,8 @@  A specialization looks like this:
  * Initialization output: ``type.section.name.init``
  * PF/VF mailbox output: ``type.section.name.mbox``
 
+These specializations are created using the ``RTE_LOG_REGISTER_SUFFIX`` macro.
+
 A real world example is the i40e poll mode driver which exposes two
 specializations, one for initialization ``pmd.net.i40e.init`` and the other for
 the remaining driver logs ``pmd.net.i40e.driver``.
diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
index 9fa7d010ef..eefba6e408 100644
--- a/lib/cfgfile/rte_cfgfile.c
+++ b/lib/cfgfile/rte_cfgfile.c
@@ -27,11 +27,13 @@  struct rte_cfgfile {
 	struct rte_cfgfile_section *sections;
 };
 
+/* Setting up dynamic logging 8< */
 RTE_LOG_REGISTER_DEFAULT(cfgfile_logtype, INFO);
 
 #define CFG_LOG(level, fmt, args...)					\
 	rte_log(RTE_LOG_ ## level, cfgfile_logtype, "%s(): " fmt "\n",	\
 		__func__, ## args)
+/* >8 End of setting up dynamic logging */
 
 /** when we resize a file structure, how many extra entries
  * for new sections do we add in */