Re: [PATCH] pktgen: avoid unused-const-variable warning

[Peter Seiderer]

Hello Paolo,

On Thu, 27 Feb 2025 12:35:45 +0100, Paolo Abeni <pabeni@xxxxxxxxxx> wrote:

> On 2/26/25 7:17 PM, Peter Seiderer wrote:
> > From: Arnd Bergmann <arnd@xxxxxxxx>
> >> When extra warnings are enable, there are configurations that build
> >> pktgen without CONFIG_XFRM, which leaves a static const variable unused:
> >>
> >> net/core/pktgen.c:213:1: error: unused variable 'F_IPSEC' [-Werror,-Wunused-const-variable]
> >>   213 | PKT_FLAGS
> >>       | ^~~~~~~~~
> >> net/core/pktgen.c:197:2: note: expanded from macro 'PKT_FLAGS'
> >>   197 |         pf(IPSEC)               /* ipsec on for flows */                \
> >>       |         ^~~~~~~~~
> >>
> >> This could be marked as __maybe_unused, or by making the one use visible
> >> to the compiler by slightly rearranging the #ifdef blocks. The second
> >> variant looks slightly nicer here, so use that.
> >>
> >> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> >> ---
> >>  net/core/pktgen.c | 9 ++-------
> >>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> >> index 55064713223e..402e01a2ce19 100644
> >> --- a/net/core/pktgen.c
> >> +++ b/net/core/pktgen.c
> >> @@ -158,9 +158,7 @@
> >>  #include <net/udp.h>
> >>  #include <net/ip6_checksum.h>
> >>  #include <net/addrconf.h>
> >> -#ifdef CONFIG_XFRM
> >>  #include <net/xfrm.h>
> >> -#endif
> >
> > This ifdef/endif can be kept (as the xfrm stuff is still not used)...
>
> FTR, I think dropping unneeded #ifdef is preferable in c files: only
> such file build time is affected, and the code is more readable.

The ifdef/endif emphasizes no xfrm usage (even by mistake) in case CONFIG_XFRM
is not defined, but in the end a matter of taste ;-)

>
> >
> >>  #include <net/netns/generic.h>
> >>  #include <asm/byteorder.h>
> >>  #include <linux/rcupdate.h>
> >> @@ -2363,13 +2361,13 @@ static inline int f_pick(struct pktgen_dev *pkt_dev)
> >>  }
> >>
> >>
> >> -#ifdef CONFIG_XFRM
> >>  /* If there was already an IPSEC SA, we keep it as is, else
> >>   * we go look for it ...
> >>  */
> >>  #define DUMMY_MARK 0
> >
> > A now unused define...
> >
> >>  static void get_ipsec_sa(struct pktgen_dev *pkt_dev, int flow)
> >>  {
> >> +#ifdef CONFIG_XFRM
> >>  	struct xfrm_state *x = pkt_dev->flows[flow].x;
> >>  	struct pktgen_net *pn = net_generic(dev_net(pkt_dev->odev), pg_net_id);
> >
> > Maybe better this way here?
> >
> > 	const u32 dummy_mark = 0;
>
> I think the unused define is preferable; I think pre-processor defines
> are cheaper than static const.

In which regards cheaper (out of interest)?

Both (with and without static) produce the same code see e.g.

	https://godbolt.org/z/Tsr1jM45r
	https://godbolt.org/z/6sr1o8da3

Regards,
Peter

>
> Thanks,
>
> Paolo
>