[PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[Wan Jiabing]

Fix the following coccicheck warning:
./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
Changelog:
v2:
- Use typeof(bytes_left) instead of u8.
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 35579cf4283f..57586a2e6dec 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
 	for (i = 0; i < data_len; i += bytes_read) {
 		u16 bytes_left = data_len - i;
 
-		bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
-					  ICE_MAX_I2C_DATA_SIZE;
+		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
 
 		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
-- 
2.35.1

RE: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[David Laight]

From: Wan Jiabing
> Sent: 21 March 2022 14:00
> 
> Fix the following coccicheck warning:
> ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> Changelog:
> v2:
> - Use typeof(bytes_left) instead of u8.
> ---
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index 35579cf4283f..57586a2e6dec 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
>  	for (i = 0; i < data_len; i += bytes_read) {
>  		u16 bytes_left = data_len - i;

Oh FFS why is that u16?
Don't do arithmetic on anything smaller than 'int'

	David

> 
> -		bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
> -					  ICE_MAX_I2C_DATA_SIZE;
> +		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
> 
>  		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
>  				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> --
> 2.35.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[Alexander Lobakin]

From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 21 Mar 2022 16:02:20 +0000

> From: Wan Jiabing
> > Sent: 21 March 2022 14:00
> >
> > Fix the following coccicheck warning:
> > ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
> >
> > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> > ---
> > Changelog:
> > v2:
> > - Use typeof(bytes_left) instead of u8.
> > ---
> >  drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > index 35579cf4283f..57586a2e6dec 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
> >  	for (i = 0; i < data_len; i += bytes_read) {
> >  		u16 bytes_left = data_len - i;
> 
> Oh FFS why is that u16?
> Don't do arithmetic on anything smaller than 'int'

Any reasoning? I don't say it's good or bad, just want to hear your
arguments (disasms, perf and object code measurements) etc.

> 
> 	David
> 
> >
> > -		bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
> > -					  ICE_MAX_I2C_DATA_SIZE;
> > +		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
> >
> >  		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> >  				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> > --
> > 2.35.1
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Thanks,
Al

RE: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[David Laight]

From: Alexander Lobakin
> Sent: 22 March 2022 17:51
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Mon, 21 Mar 2022 16:02:20 +0000
> 
> > From: Wan Jiabing
> > > Sent: 21 March 2022 14:00
> > >
> > > Fix the following coccicheck warning:
> > > ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
> > >
> > > Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> > > ---
> > > Changelog:
> > > v2:
> > > - Use typeof(bytes_left) instead of u8.
> > > ---
> > >  drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > index 35579cf4283f..57586a2e6dec 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
> > >  	for (i = 0; i < data_len; i += bytes_read) {
> > >  		u16 bytes_left = data_len - i;
> >
> > Oh FFS why is that u16?
> > Don't do arithmetic on anything smaller than 'int'
> 
> Any reasoning? I don't say it's good or bad, just want to hear your
> arguments (disasms, perf and object code measurements) etc.

Look at the object code on anything except x86.
The compiler has to add instruction to mask the value
(which is in a full sized register) down to 16 bits
after every arithmetic operation.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[Jakub Kicinski]

On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
> > > Oh FFS why is that u16?
> > > Don't do arithmetic on anything smaller than 'int'  
> > 
> > Any reasoning? I don't say it's good or bad, just want to hear your
> > arguments (disasms, perf and object code measurements) etc.  
> 
> Look at the object code on anything except x86.
> The compiler has to add instruction to mask the value
> (which is in a full sized register) down to 16 bits
> after every arithmetic operation.

Isn't it also slower on some modern x86 CPUs?
I could have sworn someone mentioned that in the past.

Don't do arithmetic on anything smaller than 'int' (was: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss)

[Paul Menzel]

Dear Linux folks,


Am 22.03.22 um 19:27 schrieb Jakub Kicinski:
> On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
>>>> Oh FFS why is that u16?
>>>> Don't do arithmetic on anything smaller than 'int'
>>>
>>> Any reasoning? I don't say it's good or bad, just want to hear your
>>> arguments (disasms, perf and object code measurements) etc.
>>
>> Look at the object code on anything except x86.
>> The compiler has to add instruction to mask the value
>> (which is in a full sized register) down to 16 bits
>> after every arithmetic operation.
> 
> Isn't it also slower on some modern x86 CPUs?
> I could have sworn someone mentioned that in the past.

I know of Scott’s article *Small Integers: Big Penalty* from 2012 [1].


Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] Don't do arithmetic on anything smaller than 'int' (was: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss)

[Alexander Lobakin]

From: Paul Menzel <pmenzel@molgen.mpg.de>
Date: Tue, 22 Mar 2022 22:02:06 +0100

> Dear Linux folks,
> 
> 
> Am 22.03.22 um 19:27 schrieb Jakub Kicinski:
> > On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
> >>>> Oh FFS why is that u16?
> >>>> Don't do arithmetic on anything smaller than 'int'
> >>>
> >>> Any reasoning? I don't say it's good or bad, just want to hear your
> >>> arguments (disasms, perf and object code measurements) etc.
> >>
> >> Look at the object code on anything except x86.
> >> The compiler has to add instruction to mask the value
> >> (which is in a full sized register) down to 16 bits
> >> after every arithmetic operation.
> >
> > Isn't it also slower on some modern x86 CPUs?
> > I could have sworn someone mentioned that in the past.
> 
> I know of Scott's article *Small Integers: Big Penalty* from 2012 [1].

Thank you all guys, makes sense!

Apart from this article, I tested some stuff on MIPS32 yesterday.
Previously I was sure that it's okay to put u16 on stack to conserve
it and there will be no code difference. I remember even having some
bloat-o-meter data. Well, human memory tends to lie sometimes.
I have a bunch of networking stats on stack which I collect during
a NAPI cycle (receiving 64 packets), it's about 20 counters. I made
them as u16 initially as it is (sizeof(u32) - sizeof(u16)) * 20 = 40
bytes. So I converted them yesterday to u32 and instead of having
+40 bytes of .text, I got -36 in one function and even -88 in
another one!
So it really makes no sense to declare anything on stack smaller
than u32 or int unless it is something to be passed to some HW or
standardized structures, e.g. __be16 etc.

Another interesting observation, on x86_64, is that u32 = u64
assignments take more instructions as well. I converted some
structure field recently from u64 to u32, but forgot that I'm
assigning it in one function from an onstack variable, which was
still unconverted from u64 to u32. When I did the latter, the .text
size became smaller.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: https://notabs.org/coding/smallIntsBigPenalty.htm

Thanks,
Al

RE: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[David Laight]

From: Jakub Kicinski
> Sent: 22 March 2022 18:28
> 
> On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
> > > > Oh FFS why is that u16?
> > > > Don't do arithmetic on anything smaller than 'int'
> > >
> > > Any reasoning? I don't say it's good or bad, just want to hear your
> > > arguments (disasms, perf and object code measurements) etc.
> >
> > Look at the object code on anything except x86.
> > The compiler has to add instruction to mask the value
> > (which is in a full sized register) down to 16 bits
> > after every arithmetic operation.
> 
> Isn't it also slower on some modern x86 CPUs?
> I could have sworn someone mentioned that in the past.

Not in the cpu clock count tables I've read.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss

[G, GurucharanX]

> -----Original Message-----
> From: Wan Jiabing <wanjiabing@vivo.com>
> Sent: Monday, March 21, 2022 7:30 PM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Lobakin, Alexandr <alexandr.lobakin@intel.com>; Wan Jiabing
> <wanjiabing@vivo.com>
> Subject: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss
> 
> Fix the following coccicheck warning:
> ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity
> for min()
> 
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> Changelog:
> v2:
> - Use typeof(bytes_left) instead of u8.
> ---
>  drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)