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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Mar 26, 2020 at 01:01:35AM CET, kuba@kernel.org wrote:
>On Wed, 25 Mar 2020 21:38:35 +0200 Eran Ben Elisha wrote:
>> On 3/25/2020 9:08 PM, Jiri Pirko wrote:
>> > 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.
>
>Are you okay with limiting the value in the policy?

Well, not-0 means true. Do you think it is wise to limit to 0/1?

[...]