| Message ID | 20221123203834.738606-6-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:30PM CET, jacob.e.keller@intel.com wrote: >The devlink_nl_region_read_snapshot_fill is used to copy the contents of >a snapshot into a message for reporting to userspace via the >DEVLINK_CMG_REGION_READ netlink message. > >A future change is going to add support for directly reading from >a region. Almost all of the logic for this new capability is identical. > >To help reduce code duplication and make this logic more generic, >refactor the function to take a cb and cb_priv pointer for doing the >actual copy. > >Add a devlink_region_snapshot_fill implementation that will simply copy >the relevant chunk of the region. This does require allocating some >storage for the chunk as opposed to simply passing the correct address >forward to the devlink_nl_cmg_region_read_chunk_fill function. > >A future change to implement support for directly reading from a region >without a snapshot will provide a separate implementation that calls the >newly added devlink region operation. > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >--- >Changes since v1: >* Use kmalloc instead of kzalloc >* Don't combine data_size declaration and assignment >* Fix the always_unused placement for devlink_region_snapshot_fill > > net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 35 insertions(+), 9 deletions(-) > >diff --git a/net/core/devlink.c b/net/core/devlink.c >index bd7af0600405..729e2162a4db 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -6460,25 +6460,36 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg, > > #define DEVLINK_REGION_READ_CHUNK_SIZE 256 > >-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, >- struct devlink_snapshot *snapshot, >- u64 start_offset, >- u64 end_offset, >- u64 *new_offset) >+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size, >+ u64 curr_offset, >+ struct netlink_ext_ack *extack); >+ >+static int >+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb, >+ void *cb_priv, u64 start_offset, u64 end_offset, >+ u64 *new_offset, struct netlink_ext_ack *extack) > { > u64 curr_offset = start_offset; > int err = 0; >+ u8 *data; >+ >+ /* Allocate and re-use a single buffer */ >+ data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL); Hmm, I tried to figure out how to do this without extra alloc and memcpy, didn't find any nice solution :/ Reviewed-by: Jiri Pirko <jiri@nvidia.com> Btw, do you plan to extend this for write as well. It might be valuable for debugging purposes to have that. I recall we discussed it in past. >+ if (!data) >+ return -ENOMEM; > > *new_offset = start_offset; > > while (curr_offset < end_offset) { > u32 data_size; >- u8 *data; > > data_size = min_t(u32, end_offset - curr_offset, > DEVLINK_REGION_READ_CHUNK_SIZE); > >- data = &snapshot->data[curr_offset]; >+ err = cb(cb_priv, data, data_size, curr_offset, extack); >+ if (err) >+ break; >+ > err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset); > if (err) > break; >@@ -6487,9 +6498,23 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, > } > *new_offset = curr_offset; > >+ kfree(data); >+ > return err; > } > >+static int >+devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size, >+ u64 curr_offset, >+ struct netlink_ext_ack __always_unused *extack) >+{ >+ struct devlink_snapshot *snapshot = cb_priv; >+ >+ memcpy(chunk, &snapshot->data[curr_offset], chunk_size); >+ >+ return 0; >+} >+ > static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > struct netlink_callback *cb) > { >@@ -6608,8 +6633,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > goto nla_put_failure; > } > >- err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset, >- end_offset, &ret_offset); >+ err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill, >+ snapshot, start_offset, end_offset, >+ &ret_offset, cb->extack); > > if (err && err != -EMSGSIZE) > goto nla_put_failure; >-- >2.38.1.420.g319605f8f00e >
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Thursday, November 24, 2022 1:12 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@nvidia.com>; Jakub Kicinski > <kuba@kernel.org> > Subject: Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill > to use a callback function > > Wed, Nov 23, 2022 at 09:38:30PM CET, jacob.e.keller@intel.com wrote: > >The devlink_nl_region_read_snapshot_fill is used to copy the contents of > >a snapshot into a message for reporting to userspace via the > >DEVLINK_CMG_REGION_READ netlink message. > > > >A future change is going to add support for directly reading from > >a region. Almost all of the logic for this new capability is identical. > > > >To help reduce code duplication and make this logic more generic, > >refactor the function to take a cb and cb_priv pointer for doing the > >actual copy. > > > >Add a devlink_region_snapshot_fill implementation that will simply copy > >the relevant chunk of the region. This does require allocating some > >storage for the chunk as opposed to simply passing the correct address > >forward to the devlink_nl_cmg_region_read_chunk_fill function. > > > >A future change to implement support for directly reading from a region > >without a snapshot will provide a separate implementation that calls the > >newly added devlink region operation. > > > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > >--- > >Changes since v1: > >* Use kmalloc instead of kzalloc > >* Don't combine data_size declaration and assignment > >* Fix the always_unused placement for devlink_region_snapshot_fill > > > > net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++--------- > > 1 file changed, 35 insertions(+), 9 deletions(-) > > > >diff --git a/net/core/devlink.c b/net/core/devlink.c > >index bd7af0600405..729e2162a4db 100644 > >--- a/net/core/devlink.c > >+++ b/net/core/devlink.c > >@@ -6460,25 +6460,36 @@ static int > devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg, > > > > #define DEVLINK_REGION_READ_CHUNK_SIZE 256 > > > >-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, > >- struct devlink_snapshot > *snapshot, > >- u64 start_offset, > >- u64 end_offset, > >- u64 *new_offset) > >+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size, > >+ u64 curr_offset, > >+ struct netlink_ext_ack *extack); > >+ > >+static int > >+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb, > >+ void *cb_priv, u64 start_offset, u64 end_offset, > >+ u64 *new_offset, struct netlink_ext_ack *extack) > > { > > u64 curr_offset = start_offset; > > int err = 0; > >+ u8 *data; > >+ > >+ /* Allocate and re-use a single buffer */ > >+ data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL); > > Hmm, I tried to figure out how to do this without extra alloc and > memcpy, didn't find any nice solution :/ > I also came up blank as well :( I can take another look when sending v3 with the other fixups. > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > Btw, do you plan to extend this for write as well. It might be valuable > for debugging purposes to have that. I recall we discussed it in past. > We could extend to support writing. I haven't had a strong need yet, and I worry about the potential for abuse of such an interface. We've already seen examples of people (ab)using such interfaces for unclear purposes... Thanks, Jake
On Mon, 28 Nov 2022 18:27:42 +0000 Keller, Jacob E wrote: > > Hmm, I tried to figure out how to do this without extra alloc and > > memcpy, didn't find any nice solution :/ > > I also came up blank as well :( I can take another look when sending v3 with the other fixups. You can certainly rearrange things to nla_reserve() the space in the message and pass to the driver a pointer to a buffer already in the skb. But I don't think it's worth the code complexity.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Monday, November 28, 2022 11:00 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; Jiri Pirko > <jiri@nvidia.com> > Subject: Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill > to use a callback function > > On Mon, 28 Nov 2022 18:27:42 +0000 Keller, Jacob E wrote: > > > Hmm, I tried to figure out how to do this without extra alloc and > > > memcpy, didn't find any nice solution :/ > > > > I also came up blank as well :( I can take another look when sending v3 with the > other fixups. > > You can certainly rearrange things to nla_reserve() the space in the > message and pass to the driver a pointer to a buffer already in the > skb. But I don't think it's worth the code complexity. Fair enough. I'll leave that as-is for v3. Thanks, Jake
diff --git a/net/core/devlink.c b/net/core/devlink.c index bd7af0600405..729e2162a4db 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6460,25 +6460,36 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg, #define DEVLINK_REGION_READ_CHUNK_SIZE 256 -static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, - struct devlink_snapshot *snapshot, - u64 start_offset, - u64 end_offset, - u64 *new_offset) +typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size, + u64 curr_offset, + struct netlink_ext_ack *extack); + +static int +devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb, + void *cb_priv, u64 start_offset, u64 end_offset, + u64 *new_offset, struct netlink_ext_ack *extack) { u64 curr_offset = start_offset; int err = 0; + u8 *data; + + /* Allocate and re-use a single buffer */ + data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL); + if (!data) + return -ENOMEM; *new_offset = start_offset; while (curr_offset < end_offset) { u32 data_size; - u8 *data; data_size = min_t(u32, end_offset - curr_offset, DEVLINK_REGION_READ_CHUNK_SIZE); - data = &snapshot->data[curr_offset]; + err = cb(cb_priv, data, data_size, curr_offset, extack); + if (err) + break; + err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset); if (err) break; @@ -6487,9 +6498,23 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb, } *new_offset = curr_offset; + kfree(data); + return err; } +static int +devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size, + u64 curr_offset, + struct netlink_ext_ack __always_unused *extack) +{ + struct devlink_snapshot *snapshot = cb_priv; + + memcpy(chunk, &snapshot->data[curr_offset], chunk_size); + + return 0; +} + static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, struct netlink_callback *cb) { @@ -6608,8 +6633,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, goto nla_put_failure; } - err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset, - end_offset, &ret_offset); + err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill, + snapshot, start_offset, end_offset, + &ret_offset, cb->extack); if (err && err != -EMSGSIZE) goto nla_put_failure;
The devlink_nl_region_read_snapshot_fill is used to copy the contents of a snapshot into a message for reporting to userspace via the DEVLINK_CMG_REGION_READ netlink message. A future change is going to add support for directly reading from a region. Almost all of the logic for this new capability is identical. To help reduce code duplication and make this logic more generic, refactor the function to take a cb and cb_priv pointer for doing the actual copy. Add a devlink_region_snapshot_fill implementation that will simply copy the relevant chunk of the region. This does require allocating some storage for the chunk as opposed to simply passing the correct address forward to the devlink_nl_cmg_region_read_chunk_fill function. A future change to implement support for directly reading from a region without a snapshot will provide a separate implementation that calls the newly added devlink region operation. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- Changes since v1: * Use kmalloc instead of kzalloc * Don't combine data_size declaration and assignment * Fix the always_unused placement for devlink_region_snapshot_fill net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-)