Re: [PATCH 13/15] devlink: support directly reading from region memory

[Jacob Keller <jacob.e.keller@intel.com>]

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:
>> @@ -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.

> 
>> };
>>
>> 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.

> 
> 
>> +{
>> +	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.

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

Yep.

Thanks,
Jake