Received-SPF: pass (google.com: domain of linux-kernel+bounces-127965-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240329170000.3241460-1-aleksander.lobakin@intel.com>
 <20240329170000.3241460-3-aleksander.lobakin@intel.com> <20240329131857.730c6528@kernel.org>
 <20240329135344.1a310f31@kernel.org> <912d1f9f-a88d-4751-8d91-b82a75f82a32@intel.com>
In-Reply-To: <912d1f9f-a88d-4751-8d91-b82a75f82a32@intel.com>
From: Eric Dumazet <edumazet@google.com>
Date: Tue, 2 Apr 2024 14:45:07 +0200
Message-ID: <CANn89iL+goDgitdic97+um6D-PZDw2xYf=2eYgnNYEYU-Nws0Q@mail.gmail.com>
Subject: Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow
 warnings throughout the file
To: Alexander Lobakin <aleksander.lobakin@intel.com>
Cc: Jakub Kicinski <kuba@kernel.org>, "David S. Miller" <davem@davemloft.net>, 
	Paolo Abeni <pabeni@redhat.com>, Dmitry Safonov <0x7f454c46@gmail.com>, 
	Heiner Kallweit <hkallweit1@gmail.com>, nex.sw.ncis.osdt.itp.upstreaming@intel.com, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 2, 2024 at 1:55=E2=80=AFPM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Fri, 29 Mar 2024 13:53:44 -0700
>
> > On Fri, 29 Mar 2024 13:18:57 -0700 Jakub Kicinski wrote:
> >>> Sparse:
> >>>
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: warning: symbol =
'_res' shadows an earlier one
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: originally decla=
red here
> >>
> >> I don't see these building with LLVM=3D1 W=3D12 C=3D1
> >> and I really don't like complicating the code because the compiler
> >> is stupid. Can't you solve this with some renames? Add another
>
> It's not the compiler, its warnings are valid actually. Shadowing makes
> it very easy to confuse variables and make bugs...
>
> >> underscore or something?
> >
> > I'm stupid I tried on the test branch which already had your fix..
>
> :D Sometimes it happens.
>
> >
> > This is enough:
> >
> > diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> > index 1ec408585373..2270fbb99cf7 100644
> > --- a/include/net/netdev_queues.h
> > +++ b/include/net/netdev_queues.h
> > @@ -89,7 +89,7 @@ struct netdev_stat_ops {
> >
> >  #define netif_txq_try_stop(txq, get_desc, start_thrs)                 =
       \
> >       ({                                                              \
> > -             int _res;                                               \
> > +             int __res;                                              \
> >                                                                       \
> >               netif_tx_stop_queue(txq);                               \
> >               /* Producer index and stop bit must be visible          \
> > @@ -101,12 +101,12 @@ struct netdev_stat_ops {
> >               /* We need to check again in a case another             \
> >                * CPU has just made room available.                    \
> >                */                                                     \
> > -             _res =3D 0;                                              =
 \
> > +             __res =3D 0;                                             =
 \
> >               if (unlikely(get_desc >=3D start_thrs)) {                =
 \
> >                       netif_tx_start_queue(txq);                      \
> > -                     _res =3D -1;                                     =
 \
> > +                     __res =3D -1;                                    =
 \
> >               }                                                       \
> > -             _res;                                                   \
> > +             __res;                                                  \
> >       })                                                              \
> >
> >  /**
>
> But what if there's a function which calls one of these functions and
> already has _res or __res or something? I know renaming is enough for
> the warnings I mentioned, but without __UNIQUE_ID() anything can happen
> anytime, so I wanted to fix that once and for all :z
>
> I already saw some macros which have a layer of indirection for
> __UNIQUE_ID(), but previously they didn't and then there were fixes
> which added underscores, renamed variables etc etc...
>

We have hundreds of macros in include/ directory which use local names with=
out
__UNIQUE_ID()

What is the plan ? Hundreds of patches obfuscating them more than they are =
?

Can you show us how rb_entry_safe() (random choice) would be rewritten ?