Re: [PATCH 11/11] drivers: use per line logging in helpers

[David Marchand <david.marchand@redhat.com>]

On Sat, Sep 7, 2024 at 4:56 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Use RTE_LOG_LINE in existing macros that append a \n.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

[snip]

> diff --git a/drivers/net/octeon_ep/otx_ep_common.h b/drivers/net/octeon_ep/otx_ep_common.h
> index 7d5dd91a77..ea9788757e 100644
> --- a/drivers/net/octeon_ep/otx_ep_common.h
> +++ b/drivers/net/octeon_ep/otx_ep_common.h
> @@ -68,18 +68,18 @@
>  #define OTX_CUST_DATA_LEN 0
>
>  #define otx_ep_info(fmt, args...)                              \
> -       rte_log(RTE_LOG_INFO, otx_net_ep_logtype,               \
> -               "%s():%u " fmt "\n",                            \
> +       RTE_LOG_LINE(INFO, OTX_NET_EP,          \
> +               "%s():%u " fmt,                         \
>                 __func__, __LINE__, ##args)
>
>  #define otx_ep_err(fmt, args...)                               \
> -       rte_log(RTE_LOG_ERR, otx_net_ep_logtype,                \
> -               "%s():%u " fmt "\n",                            \
> +       RTE_LOG_LINE(ERR, OTX_NET_EP,           \
> +               "%s():%u " fmt,                         \
>                 __func__, __LINE__, ##args)
>
>  #define otx_ep_dbg(fmt, args...)                               \
> -       rte_log(RTE_LOG_DEBUG, otx_net_ep_logtype,              \
> -               "%s():%u " fmt "\n",                            \
> +       RTE_LOG_LINE(DEBUG, OTX_NET_EP,         \
> +               "%s():%u " fmt,                         \
>                 __func__, __LINE__, ##args)
>
>  /* IO Access */

I am facing a strange warning with gcc 13 on Fedora 39 when compiler
optimisations (O3) are on.
Looking at the CI reports, some other versions of gcc seem affected.

On the other hand, O0, O1, O2 are fine, at least with my gcc.

Here is the warning:

ccache cc -Idrivers/libtmp_rte_net_octeon_ep.a.p -Idrivers
-I../drivers -Idrivers/net/octeon_ep -I../drivers/net/octeon_ep
-Ilib/ethdev -I../lib/ethdev -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/log
-I../lib/log -Ilib/metrics -I../lib/metrics -Ilib/telemetry
-I../lib/telemetry -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf
-Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter
-I../lib/meter -Idrivers/bus/pci -I../drivers/bus/pci
-I../drivers/bus/pci/linux -Ilib/pci -I../lib/pci -Idrivers/bus/vdev
-I../drivers/bus/vdev -fdiagnostics-color=always
-D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11
-O3 -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
-fPIC -march=native -mrtm -DALLOW_EXPERIMENTAL_API
-DALLOW_INTERNAL_API -Wno-format-truncation -DCC_AVX2_SUPPORT
-Wno-strict-aliasing -flax-vector-conversions
-DRTE_LOG_DEFAULT_LOGTYPE=pmd.net.octeon_ep -MD -MQ
drivers/libtmp_rte_net_octeon_ep.a.p/net_octeon_ep_otx_ep_ethdev.c.o
-MF drivers/libtmp_rte_net_octeon_ep.a.p/net_octeon_ep_otx_ep_ethdev.c.o.d
-o drivers/libtmp_rte_net_octeon_ep.a.p/net_octeon_ep_otx_ep_ethdev.c.o
-c ../drivers/net/octeon_ep/otx_ep_ethdev.c
../drivers/net/octeon_ep/otx_ep_ethdev.c: In function ‘otx_ep_dev_mtu_set’:
../drivers/net/octeon_ep/otx_ep_ethdev.c:200:12: error:
‘devinfo.max_mtu’ may be used uninitialized
[-Werror=maybe-uninitialized]
  200 |         if (mtu > devinfo.max_mtu) {
      |            ^
../drivers/net/octeon_ep/otx_ep_ethdev.c:186:33: note:
‘devinfo.max_mtu’ was declared here
  186 |         struct rte_eth_dev_info devinfo;
      |                                 ^~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.

This warning only appears with the last patch of the series.
Looking at the code, it seems incorrect, as devinfo.max_mtu is
supposed to be initialised in otx_ep_dev_info_get() if this function
returns 0.


A minimum reproducer is (it can be tested on origin/main):

$ git diff
diff --git a/drivers/net/octeon_ep/otx_ep_ethdev.c
b/drivers/net/octeon_ep/otx_ep_ethdev.c
index c4a5a67c79..0a6dbcea0d 100644
--- a/drivers/net/octeon_ep/otx_ep_ethdev.c
+++ b/drivers/net/octeon_ep/otx_ep_ethdev.c
@@ -135,7 +135,7 @@ otx_ep_dev_info_get(struct rte_eth_dev *eth_dev,

        max_rx_pktlen = otx_ep_mbox_get_max_pkt_len(eth_dev);
        if (!max_rx_pktlen) {
-               otx_ep_err("Failed to get Max Rx packet length");
+               rte_log(RTE_LOG_ERR, otx_net_ep_logtype, "%u
%u\n%.0s", 1, 1, "");
                return -EINVAL;
        }

(FYI, the %.0s trick comes from expanding RTE_FMT())

After various tries, it seems hitting this error requires passing (at
least) 2 arguments in addition to %.0s in the format string.
Another observation is that removing __rte_cold from rte_log()
declaration makes the warning go away.

I am a bit puzzled, it smells like a compiler (optimisation) bug to me.

A workaround is to memset(&devinfo, 0) or call rte_eth_dev_info_get()
in otx_ep_dev_mtu_set.
But on the other hand we may have a deeper issue with RTE_FMT()...

Opinions?


-- 
David Marchand