Received-SPF: pass (google.com: domain of linux-kernel+bounces-128294-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <22021664-6630-4663-ac28-c0df4187d8b6@intel.com>
Date: Tue, 2 Apr 2024 17:53:08 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow
 warnings throughout the file
To: Eric Dumazet <edumazet@google.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>
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>
 <CANn89iL+goDgitdic97+um6D-PZDw2xYf=2eYgnNYEYU-Nws0Q@mail.gmail.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <CANn89iL+goDgitdic97+um6D-PZDw2xYf=2eYgnNYEYU-Nws0Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UUxuWkxwTXJpNHlBZDJzYThSUklMc0dPTVN3TDdxTThXbEt6RGc5a1FmOTRr?=
 =?utf-8?B?eEg1Y1BGUDQydjdGZS9ja01HUjVwKzN6MmgvVjg5OUJxNGptenJFRENYOWJG?=
 =?utf-8?B?cDBnMkJDUW1lQiszeUh3NEVjUEhrZEZUV1AvSGFlMlhOdTVWbyt5cEkvOTlS?=
 =?utf-8?B?a2grU1NTbE5mTlMzaGcxaW1vREVvMGRhMC9FRnUxc3lWQ0RLY3M1aG9IeUJO?=
 =?utf-8?B?YklKRGVmZUpGYVlDNHc0UVhUb3k1aEh3UEZJOUxPUThhV2N5L2VVOTdpOXRi?=
 =?utf-8?B?M1krS1I5OFh4NFNzam5jWUxTSll5a2hWa3ZYZVI1dVdzVGowWUJ6aHhaNnUv?=
 =?utf-8?B?N1Q3cm4xRGMzWjZqeWs0ZXFyR2ZaQ0JwdWZMTEJkRTZJYnlFN0pPYVZyOG1B?=
 =?utf-8?B?WGwyWlVSbysvMXZhdTVRYmZwNkZxamNDTjM2UWtzRjllUjdyYmN6NWRKamQ5?=
 =?utf-8?B?L3RrL1MwYk50RVVFSmx1elRFMHNQTlZiR29zTUJISko2bU1IdlJ5OUUyQ0F2?=
 =?utf-8?B?STFUaTZlajhHOURmUnBKWFpTaElDWkx1VWp1bjBwODhxay82WDR2RUNoMHBS?=
 =?utf-8?B?aVBuVVBiMnc0U09GU1NyQmltTVdRREdzbWRGNTlEdVRsaHIxTGdFQUlTOTQ1?=
 =?utf-8?B?QzBIZXdubWVSWTFqN21CcmxwZXdTQTBlY3dyQkdnL1BIZ0k3QysvZklMblBy?=
 =?utf-8?B?c3V2OTZQM2dNKzRmQ1I5dDNUb0RLZzhqb1J1bU9RbTRzbTNOZk85clBObmdj?=
 =?utf-8?B?MDdPcEpabVgra05Mc2Nvb0lkaldndTQyZXdqT3J2QTlHYTRRMTZLVjV6NjFa?=
 =?utf-8?B?eUViUjVsU2c0L28vQUVFbHJaQTQ1TlNnaVlkbEFwWFRiQld2UW0xRjg5R2Ry?=
 =?utf-8?B?T1pVamlkZnlRci8reUJIamVKOXEwUHQwaGFuTVZTQ1pta1dvRUZqM1ZPZ0xN?=
 =?utf-8?B?cVhJNk5KNXRWWWUrV3lhQnAyMnQxSDRqdWludDVoeWNNbEtSbkgwN0hacmlX?=
 =?utf-8?B?UHB0aVIzUFRLbUxyUk1CUjIzSFVyMG5pQ3Z0WFAzUWl6dVNtZmtGZkNZalMx?=
 =?utf-8?B?WFAreS9MZWNac3dBVWd4bDZMZXYrQkp3REw1SXJ5NE9xVWZKL0NsTUpyZHRS?=
 =?utf-8?B?NlJGdDZBYU10NndxUk9SNEREZ2dJYWlndTFheWNSbG0xREY2aDlUVDYzdDF6?=
 =?utf-8?B?N0dKZ3Q1TFZXZVZWbXhGYkxYY3NnTHhMNE5MTW41dE5NbVJKV0V6ZndpMHl6?=
 =?utf-8?B?Uk1mSFl0QmhFdTBtWDJoTE5WN1RFUVJnejl4b0Vnd1IzS0ZsMGlFQkh6Z0VP?=
 =?utf-8?B?VHhnUkhlbU81QVhtOUlsM3RmUzlpQnBvQ3hvUHNJemlleEVnVk5aeUtUK3NW?=
 =?utf-8?B?eHRvK0taNkpYMjFnYW9nUDhJUHMrUEZieG1oL25GNE9hUHd5NHJpN0QySlUy?=
 =?utf-8?B?cDl4dExJb0NFS2VPTkl3cXRkdkNDWTFtREs0SEt4Wmw2M1NGVS9CQXdYazNH?=
 =?utf-8?B?QmNwTHFrcUVaZUxBbWJnenZPc0VMZ1JWaWFBS1FRbCtJdkE1Y0JUd2NGV2I5?=
 =?utf-8?B?bC9QVmlsWWMrTzhmd3NsQ1FVUC9sOW9JNmhVOVp5S0d5L2ZGbm1WaU5hSHdD?=
 =?utf-8?B?bWdIN1cxRHZvek9sMWdGcTNZdFNVWCtiMmQreUVkaXUrUEVuZnUwcjFlVEda?=
 =?utf-8?B?bXlIZVNJRGR2UVZ0bmNGQW9VSjIwbU1VRnl2SDd1NkwydHFSc0lwQVlDOGho?=
 =?utf-8?B?d2NZOXJUVk0xakZzK2dqekFWdS9lelB4ZlhZTk1vSHNFcDNSamJzamtnazZ1?=
 =?utf-8?B?NVJ0UjNoelZSbUtTaEpKcFhHdkN1ckxZTUNQU3ZNL3FOWERPMlF5VHlCVEJ3?=
 =?utf-8?B?UzdOY2J5ak52alVGSzlhYUFUVnNPdDFsQ1hzSGJBbXVoTWJRczA2WnhxNmFz?=
 =?utf-8?B?NEhKdSs2cVZOMitNL1dzSDhRcVBvUjgyN3pQZ0tDWG5FazBNVjBhVDZYMXk0?=
 =?utf-8?B?azdTWFc3WU9acGJJNHVFTXUvWUFpbzIyRXV3WnlwNFZlTXhRclBHYTNqTWV6?=
 =?utf-8?B?am9MaXEyN2xwUzZHSjNUMktQWlU3TDNzNDVzWTJTTkJQeWRYMmdsVXhya1Vu?=
 =?utf-8?B?UzVTZ3FDVFpzSW9HNE9XSWRYT1RpYW44M0V3SzZ1bDBqSk9TRjlDbnhSeUZy?=
 =?utf-8?B?TVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e95074de-3cda-4163-b284-08dc532d2429
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 15:54:12.7992
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fiQFO53nVaqNi7gN3RtbJ70qhluOqT7optOArMtmbRMiWIK6ssGxdJIF3HH0lWTSoms5jO9sJ3oOW92bKLGpjYo57vWhuH/iCPUtabUA8d4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB6052
X-OriginatorOrg: intel.com

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 2 Apr 2024 14:45:07 +0200

> On Tue, Apr 2, 2024 at 1:55 PM 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 declared here
>>>>
>>>> I don't see these building with LLVM=1 W=12 C=1
>>>> 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 = 0;                                               \
>>> +             __res = 0;                                              \
>>>               if (unlikely(get_desc >= start_thrs)) {                 \
>>>                       netif_tx_start_queue(txq);                      \
>>> -                     _res = -1;                                      \
>>> +                     __res = -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 without
> __UNIQUE_ID()

Most of them were added before __UNIQUE_ID() became norm, weren't they?
Lots of them were switched to __UNIQUE_ID() because of issues, weren't they?

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

Only those which flood the console when building with W=12 C=1 to
recheck that my new code is fine.

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

Is it unique in some way?

#define _rb_entry_safe(ptr, type, member, ____ptr) \
	({ typeof(ptr) ____ptr = (ptr); \
	   ____ptr ? rb_entry(____ptr, type, member) : NULL; \
	})
#define rb_entry_safe(ptr, type, member)		\
	_rb_entry_safe(ptr, type, member,		\
		       __UNIQUE_ID(ptr_)

(@____ptr can be renamed if needed, this one would give the smallest
 code diff)

If you think +1 layer is a terrible obfuscating, look at
<linux/fortify-string.h> :D

Thanks,
Olek