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

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

On 1/31/2020 10:07 AM, Jakub Kicinski wrote:
> On Thu, 30 Jan 2020 14:59:08 -0800, Jacob Keller wrote:
>> Add a new region operation for directly reading from a region, without
>> taking a full snapshot.
>>
>> Extend the DEVLINK_CMD_REGION_READ to allow directly reading from
>> a region, if supported. Instead of reporting a missing snapshot id as
>> invalid, check to see if direct reading is implemented for the region.
>> If so, use the direct read operation to grab the current contents of the
>> region.
>>
>> This new behavior of DEVLINK_CMD_REGION_READ should be backwards
>> compatible. Previously, all kernels rejected such
>> a DEVLINK_CMD_REGION_READ with -EINVAL, and will now either accept the
>> call or report -EOPNOTSUPP for regions which do not implement direct
>> access.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
>> +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)
>> +{
>> +	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;
> 
> Also known as min() ?
> 

Heh. Yep.

>> +		err = region->ops->read(devlink, curr_offset, data_size, data);
>> +		if (err)
>> +			break;
>> +
>> +		err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink,
>> +							    data, data_size,
>> +							    curr_offset);
>> +		if (err)
>> +			break;
>> +
>> +		curr_offset += data_size;
>> +	}
>> +	*new_offset = curr_offset;
>> +
>> +	kfree(data);
>> +
>> +	return err;
>> +}
>> +
>>  static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>>  					     struct netlink_callback *cb)
>>  {
> 
>> +	/* Region may not support direct read access */
>> +	if (direct && !region->ops->read) {
> 
> for missing trigger you added an extack, perhaps makes sense here, too?
> 

Sure.

>> +		err = -EOPNOTSUPP;
>> +		goto out_unlock;
>> +	}
>> +
>>  	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
>>  			  &devlink_nl_family, NLM_F_ACK | NLM_F_MULTI,
>>  			  DEVLINK_CMD_REGION_READ);
> 
> Generally all the devlink parts look quite reasonable to me 👍
>