| Message ID | 20221117220803.2773887-3-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: support direct read from region | expand |
On Thu, 17 Nov 2022 14:07:57 -0800 Jacob Keller 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. > > Noticed-by: Jakub Kicinski <kuba@kernel.org> I'm afraid that's not a real tag. You can just drop it, I get sufficient credits. > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 96afc7013959..932476956d7e 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -6410,14 +6410,10 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, > *new_offset = start_offset; > > while (curr_offset < end_offset) { > - u32 data_size; > + u32 data_size = min_t(u32, end_offset - curr_offset, > + DEVLINK_REGION_READ_CHUNK_SIZE); nit: don't put multi-line statements on the declaration line if it's not the only variable. > 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;
On 11/18/2022 5:36 PM, Jakub Kicinski wrote: > On Thu, 17 Nov 2022 14:07:57 -0800 Jacob Keller 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. >> >> Noticed-by: Jakub Kicinski <kuba@kernel.org> > > I'm afraid that's not a real tag. You can just drop it, > I get sufficient credits. > Sure. I pulled this forward from some time ago, not sure why I had added it back then. A grep through the log shows its been used a handful of times, but it likely just slipped in without review in the past. Will remove. >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 96afc7013959..932476956d7e 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -6410,14 +6410,10 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, >> *new_offset = start_offset; >> >> while (curr_offset < end_offset) { >> - u32 data_size; >> + u32 data_size = min_t(u32, end_offset - curr_offset, >> + DEVLINK_REGION_READ_CHUNK_SIZE); > > nit: don't put multi-line statements on the declaration line if it's > not the only variable. > Sure, that makes sense. >> 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;
On 11/21/2022 9:51 AM, Jacob Keller wrote: > On 11/18/2022 5:36 PM, Jakub Kicinski wrote: >>> diff --git a/net/core/devlink.c b/net/core/devlink.c >>> index 96afc7013959..932476956d7e 100644 >>> --- a/net/core/devlink.c >>> +++ b/net/core/devlink.c >>> @@ -6410,14 +6410,10 @@ static int >>> devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, >>> *new_offset = start_offset; >>> while (curr_offset < end_offset) { >>> - u32 data_size; >>> + u32 data_size = min_t(u32, end_offset - curr_offset, >>> + DEVLINK_REGION_READ_CHUNK_SIZE); >> >> nit: don't put multi-line statements on the declaration line if it's >> not the only variable. >> > > Sure, that makes sense. > This becomes the only variable in patch 5 of 8. It ends up making the diff look more complicated if I change it back to a combined declare+assign in that patch.
On Mon, 21 Nov 2022 10:35:34 -0800 Jacob Keller wrote: > > Sure, that makes sense. > > This becomes the only variable in patch 5 of 8. It ends up making the > diff look more complicated if I change it back to a combined > declare+assign in that patch. Don't change it back to declare+assign, then? :) In general declare+assign should be used sparingly IMO. My eyes are trained to skip right past the variable declarations, the goal is to make the code clear, not short :S BTW you can probably make DEVLINK_REGION_READ_CHUNK_SIZE a ULL to switch from min_t() to min() ?
On 11/21/2022 11:06 AM, Jakub Kicinski wrote: > On Mon, 21 Nov 2022 10:35:34 -0800 Jacob Keller wrote: >>> Sure, that makes sense. >> >> This becomes the only variable in patch 5 of 8. It ends up making the >> diff look more complicated if I change it back to a combined >> declare+assign in that patch. > > Don't change it back to declare+assign, then? :) > In general declare+assign should be used sparingly IMO. > My eyes are trained to skip right past the variable declarations, > the goal is to make the code clear, not short :S > > BTW you can probably make DEVLINK_REGION_READ_CHUNK_SIZE a ULL to switch > from min_t() to min() ? Fair enough. It looked a bit weird when it was: u32 data_size; data_size = ... But I think thats ok. Thanks, Jake
diff --git a/net/core/devlink.c b/net/core/devlink.c index 96afc7013959..932476956d7e 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6410,14 +6410,10 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, *new_offset = start_offset; while (curr_offset < end_offset) { - u32 data_size; + u32 data_size = min_t(u32, end_offset - curr_offset, + DEVLINK_REGION_READ_CHUNK_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 = &snapshot->data[curr_offset]; err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink, data, data_size,
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. Noticed-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- net/core/devlink.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)