1. Patchew
  2. linux
  3. View series

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

Wan Jiabing posted 1 patch 4 years, 3 months ago 
drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH v2] ice: use min_t() to make code cleaner in ice_gnss
Posted by Wan Jiabing 4 years, 3 months ago 
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
Posted by David Laight 4 years, 3 months ago 
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
Posted by Alexander Lobakin 4 years, 3 months ago 
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
Posted by David Laight 4 years, 3 months ago 
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
Posted by Jakub Kicinski 4 years, 3 months ago 
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)
Posted by Paul Menzel 4 years, 3 months ago 
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)
Posted by Alexander Lobakin 4 years, 3 months ago 
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
Posted by David Laight 4 years, 3 months ago 
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
Posted by G, GurucharanX 4 years, 2 months ago 

> -----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)

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.