| Message ID | 20221123203834.738606-2-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | support direct read from region | expand |
Wed, Nov 23, 2022 at 09:38:26PM CET, jacob.e.keller@intel.com wrote: >The calculation for the data_size in the devlink_nl_read_snapshot_fill >function uses an if statement that is better expressed using the min_t >macro. > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
From: Jacob Keller > Sent: 23 November 2022 20:38 > > The calculation for the data_size in the devlink_nl_read_snapshot_fill > function uses an if statement that is better expressed using the min_t > macro. There ought to be a 'duck shoot' arranged for all uses of min_t(). I was testing a patch (I might submit next week) that relaxes the checks in min() so that it doesn't error a lot of valid cases. In particular a positive integer constant can always be cast to (int) and the compare will DTRT. I found things like min_t(u32, u32_length, u64_limit) where you really don't want to mask the limit down. There are also the min_t(u8, ...) and min_t(u16, ...). ... > + data_size = min_t(u32, end_offset - curr_offset, > + DEVLINK_REGION_READ_CHUNK_SIZE); Here I think both xxx_offset are u32 - so the CHUNK_SIZE constant probably needs a U suffix. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 11/24/2022 1:53 PM, David Laight wrote: > From: Jacob Keller >> Sent: 23 November 2022 20:38 >> >> The calculation for the data_size in the devlink_nl_read_snapshot_fill >> function uses an if statement that is better expressed using the min_t >> macro. > > There ought to be a 'duck shoot' arranged for all uses of min_t(). > I was testing a patch (I might submit next week) that relaxes the > checks in min() so that it doesn't error a lot of valid cases. > In particular a positive integer constant can always be cast to (int) > and the compare will DTRT. > > I found things like min_t(u32, u32_length, u64_limit) where > you really don't want to mask the limit down. > There are also the min_t(u8, ...) and min_t(u16, ...). > Wouldn't that example just want to be min_t(u64, ...)? > > ... >> + data_size = min_t(u32, end_offset - curr_offset, >> + DEVLINK_REGION_READ_CHUNK_SIZE); > > Here I think both xxx_offset are u32 - so the CHUNK_SIZE > constant probably needs a U suffix. Right. My understanding was that min_t would cast everything to a u32 when doing such comparison, and we know that DEVLINK_REGION_READ_CHUNK_SIZE is < U32_MAX so this is ok? Or am I misunderstanding? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
From: Jacob Keller > Sent: 28 November 2022 18:31 > > On 11/24/2022 1:53 PM, David Laight wrote: > > From: Jacob Keller > >> Sent: 23 November 2022 20:38 > >> > >> The calculation for the data_size in the devlink_nl_read_snapshot_fill > >> function uses an if statement that is better expressed using the min_t > >> macro. > > > > There ought to be a 'duck shoot' arranged for all uses of min_t(). > > I was testing a patch (I might submit next week) that relaxes the > > checks in min() so that it doesn't error a lot of valid cases. > > In particular a positive integer constant can always be cast to (int) > > and the compare will DTRT. > > > > I found things like min_t(u32, u32_length, u64_limit) where > > you really don't want to mask the limit down. > > There are also the min_t(u8, ...) and min_t(u16, ...). > > > > Wouldn't that example just want to be min_t(u64, ...)? That is what is would need to be. But the compiler can work it out and get it right. > > ... > >> + data_size = min_t(u32, end_offset - curr_offset, > >> + DEVLINK_REGION_READ_CHUNK_SIZE); > > > > Here I think both xxx_offset are u32 - so the CHUNK_SIZE > > constant probably needs a U suffix. > > Right. My understanding was that min_t would cast everything to a u32 > when doing such comparison, and we know that > DEVLINK_REGION_READ_CHUNK_SIZE is < U32_MAX so this is ok? > > Or am I misunderstanding? The code isn't wrong, except that errors from min() are really an indication that the types mismatch, not that you should add loads of casts. You wouldn't think: x = (int)a + (int)b; was anything normal, but that is what min_t() does. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/net/core/devlink.c b/net/core/devlink.c index d93bc95cd7cb..a490b8850179 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6485,10 +6485,8 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, u32 data_size; u8 *data; - if (end_offset - curr_offset < DEVLINK_REGION_READ_CHUNK_SIZE) - data_size = end_offset - curr_offset; - else - data_size = DEVLINK_REGION_READ_CHUNK_SIZE; + data_size = min_t(u32, end_offset - curr_offset, + DEVLINK_REGION_READ_CHUNK_SIZE); data = &snapshot->data[curr_offset]; err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink,
The calculation for the data_size in the devlink_nl_read_snapshot_fill function uses an if statement that is better expressed using the min_t macro. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- Changes since v1: * Moved to 1/9 of the series * No longer combined declaration of data_size with its assignment net/core/devlink.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)