On 2/3/2020 5:44 AM, Jiri Pirko wrote:

Thu, Jan 30, 2020 at 11:59:08PM CET, jacob.e.keller@intel.com wrote:

quoted

@@ -508,6 +508,10 @@ struct devlink_region_ops {

			struct netlink_ext_ack *extack,
			u8 **data,
			devlink_snapshot_data_dest_t **destructor);
+	int (*read)(struct devlink *devlink,
+		    u64 curr_offset,
+		    u32 data_size,
+		    u8 *data);

Too much wrapping.

Yep, will clean it up.

quoted

};

struct devlink_fmsg;

diff --git a/net/core/devlink.c b/net/core/devlink.c
index b2b855d12a11..5831b7b78915 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c

@@ -4005,6 +4005,56 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,

	return err;
}

+static int devlink_nl_region_read_direct_fill(struct sk_buff *skb,
+					      struct devlink *devlink,
+					      struct devlink_region *region,
+					      struct nlattr **attrs,
+					      u64 start_offset,
+					      u64 end_offset,
+					      bool dump,
+					      u64 *new_offset)

Again.

Yep.

quoted

+{
+	u64 curr_offset = start_offset;
+	int err = 0;
+	u8 *data;
+
+	/* Allocate and re-use a single buffer */
+	data = kzalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	*new_offset = start_offset;
+
+	if (end_offset > region->size || dump)
+		end_offset = region->size;
+
+	while (curr_offset < end_offset) {
+		u32 data_size;
+
+		if (end_offset - curr_offset < DEVLINK_REGION_READ_CHUNK_SIZE)
+			data_size = end_offset - curr_offset;
+		else
+			data_size = DEVLINK_REGION_READ_CHUNK_SIZE;
+
+		err = region->ops->read(devlink, curr_offset, data_size, data);

There is a lot of code duplication is this function. Perhap there could
be a cb and cb_priv here to distinguish shapshot and direct read?

So, I was looking into how to do this, and I have a couple of patches to
simplify this function:

first I removed the region parameter and replaced it with "snapshot",
which enabled removing the dump and calculating the end_offset in the
caller function properly...

The main problem I found in sharing code is that snapshots didn't need
to allocate a data buffer while the raw-read does.

I'll see about doing a sort of cb/cb_priv setup... I'm not 100%
convinced yet though.

	direct = !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];

Makes sense.

quoted

+
+	/* Region may not support direct read access */
+	if (direct && !region->ops->read) {

extack msg please.

Yep.

Thanks,
Jake