| Message ID | 20220321135947.378250-1-wanjiabing@vivo.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ice: use min_t() to make code cleaner in ice_gnss | expand |
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)
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
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)
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.
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
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)
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
> -----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)
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),
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(-)