Re: [PATCH net-next 2/2] devlink: Add auto dump flag to health reporter

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 25, 2020 at 07:45:29PM CET, kuba@kernel.org wrote:
>On Wed, 25 Mar 2020 15:26:24 +0200 Eran Ben Elisha wrote:
>> On low memory system, run time dumps can consume too much memory. Add
>> administrator ability to disable auto dumps per reporter as part of the
>> error flow handle routine.
>> 
>> This attribute is not relevant while executing
>> DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET.
>> 
>> By default, auto dump is activated for any reporter that has a dump method,
>> as part of the reporter registration to devlink.
>> 
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  include/uapi/linux/devlink.h |  2 ++
>>  net/core/devlink.c           | 26 ++++++++++++++++++++++----
>>  2 files changed, 24 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index dfdffc42e87d..e7891d1d2ebd 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -429,6 +429,8 @@ enum devlink_attr {
>>  	DEVLINK_ATTR_NETNS_FD,			/* u32 */
>>  	DEVLINK_ATTR_NETNS_PID,			/* u32 */
>>  	DEVLINK_ATTR_NETNS_ID,			/* u32 */
>> +
>> +	DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP,	/* u8 */
>>  	/* add new attributes above here, update the policy in devlink.c */
>>  
>>  	__DEVLINK_ATTR_MAX,
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index ad69379747ef..e14bf3052289 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4837,6 +4837,7 @@ struct devlink_health_reporter {
>>  	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>  	u64 graceful_period;
>>  	bool auto_recover;
>> +	bool auto_dump;
>>  	u8 health_state;
>>  	u64 dump_ts;
>>  	u64 dump_real_ts;
>> @@ -4903,6 +4904,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>  	reporter->devlink = devlink;
>>  	reporter->graceful_period = graceful_period;
>>  	reporter->auto_recover = !!ops->recover;
>> +	reporter->auto_dump = !!ops->dump;
>>  	mutex_init(&reporter->dump_lock);
>>  	refcount_set(&reporter->refcount, 1);
>>  	list_add_tail(&reporter->list, &devlink->reporter_list);
>> @@ -4983,6 +4985,10 @@ devlink_nl_health_reporter_fill(struct sk_buff *msg,
>>  	    nla_put_u64_64bit(msg, DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS_NS,
>>  			      reporter->dump_real_ts, DEVLINK_ATTR_PAD))
>>  		goto reporter_nest_cancel;
>> +	if (reporter->ops->dump &&
>> +	    nla_put_u8(msg, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP,
>> +		       reporter->auto_dump))
>> +		goto reporter_nest_cancel;
>
>Since you're making it a u8 - does it make sense to indicate to user

Please don't be mistaken. u8 carries a bool here.


>space whether the dump is disabled or not supported?

If you want to expose "not supported", I suggest to do it in another
attr. Because this attr is here to do the config from userspace. Would
be off if the same enum would carry "not supported".

But anyway, since you opened this can, the supported/capabilities
should be probably passed by a separate bitfield for all features.


>
>Right now no attribute means either old kernel or dump not possible..
>
>>  	nla_nest_end(msg, reporter_attr);
>>  	genlmsg_end(msg, hdr);
>> @@ -5129,10 +5135,12 @@ int devlink_health_report(struct devlink_health_reporter *reporter,
>>  
>>  	reporter->health_state = DEVLINK_HEALTH_REPORTER_STATE_ERROR;
>>  
>> -	mutex_lock(&reporter->dump_lock);
>> -	/* store current dump of current error, for later analysis */
>> -	devlink_health_do_dump(reporter, priv_ctx, NULL);
>> -	mutex_unlock(&reporter->dump_lock);
>> +	if (reporter->auto_dump) {
>> +		mutex_lock(&reporter->dump_lock);
>> +		/* store current dump of current error, for later analysis */
>> +		devlink_health_do_dump(reporter, priv_ctx, NULL);
>> +		mutex_unlock(&reporter->dump_lock);
>> +	}
>>  
>>  	if (reporter->auto_recover)
>>  		return devlink_health_reporter_recover(reporter,
>> @@ -5306,6 +5314,11 @@ devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
>>  		err = -EOPNOTSUPP;
>>  		goto out;
>>  	}
>> +	if (!reporter->ops->dump &&
>> +	    info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP]) {
>
>... and then this behavior may have to change, I think?
>
>> +		err = -EOPNOTSUPP;
>> +		goto out;
>> +	}
>>  
>>  	if (info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
>>  		reporter->graceful_period =
>> @@ -5315,6 +5328,10 @@ devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
>>  		reporter->auto_recover =
>>  			nla_get_u8(info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER]);
>>  
>> +	if (info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP])
>> +		reporter->auto_dump =
>> +		nla_get_u8(info->attrs[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP]);
>> +
>>  	devlink_health_reporter_put(reporter);
>>  	return 0;
>>  out:
>> @@ -6053,6 +6070,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
>>  	[DEVLINK_ATTR_HEALTH_REPORTER_NAME] = { .type = NLA_NUL_STRING },
>>  	[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD] = { .type = NLA_U64 },
>>  	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_RECOVER] = { .type = NLA_U8 },
>> +	[DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP] = { .type = NLA_U8 },
>
>I'd suggest we keep the attrs in order of definition, because we should
>set .strict_start_type, and then it matters which are before and which
>are after.
>
>Also please set max value of 1.
>
>>  	[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME] = { .type = NLA_NUL_STRING },
>>  	[DEVLINK_ATTR_FLASH_UPDATE_COMPONENT] = { .type = NLA_NUL_STRING },
>>  	[DEVLINK_ATTR_TRAP_NAME] = { .type = NLA_NUL_STRING },
>