Re: [PATCH net-next v2 14/15] devlink: add by-instance dump infra

[Jiri Pirko <jiri@resnulli.us>]

Fri, Jan 06, 2023 at 01:16:05AM CET, jacob.e.keller@intel.com wrote:
>
>
>On 1/5/2023 1:24 AM, Jiri Pirko wrote:
>> Thu, Jan 05, 2023 at 05:05:30AM CET, kuba@kernel.org wrote:
>>> Most dumpit implementations walk the devlink instances.
>>> This requires careful lock taking and reference dropping.
>>> Factor the loop out and provide just a callback to handle
>>> a single instance dump.
>>>
>>> Convert one user as an example, other users converted
>>> in the next change.
>>>
>>> Slightly inspired by ethtool netlink code.
>>>
>>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>> ---
>>> net/devlink/devl_internal.h | 10 +++++++
>>> net/devlink/leftover.c      | 55 ++++++++++++++++---------------------
>>> net/devlink/netlink.c       | 34 +++++++++++++++++++++++
>>> 3 files changed, 68 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>>> index 15149b0a68af..734553beccde 100644
>>> --- a/net/devlink/devl_internal.h
>>> +++ b/net/devlink/devl_internal.h
>>> @@ -122,6 +122,11 @@ struct devlink_nl_dump_state {
>>> 	};
>>> };
>>>
>>> +struct devlink_gen_cmd {
>> 
>> As I wrote in reply to v1, could this be "genl"?
>> 
>
>Except Kuba already said this wasn't about "generic netlink" but
>"generic devlink command" vs "complicated command that can't use the new

Okay, that confuses me. What is supposed to be "generic devlink
command"? I don't see anything "generic" about these.


>iterator for whatever reason", so genl feels misleading.
>
>I guess gen is also kind of misleading but I can't think of anything better.
>
>> 
>>> +	int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
>>> +			struct netlink_callback *cb);
>>> +};
>>> +
>>> /* Iterate over registered devlink instances for devlink dump.
>>>  * devlink_put() needs to be called for each iterated devlink pointer
>>>  * in loop body in order to release the reference.
>> 
>> [...]
>> 
>> 
>>> @@ -9130,7 +9123,7 @@ const struct genl_small_ops devlink_nl_ops[56] = {
>>> 	{
>>> 		.cmd = DEVLINK_CMD_RATE_GET,
>>> 		.doit = devlink_nl_cmd_rate_get_doit,
>>> -		.dumpit = devlink_nl_cmd_rate_get_dumpit,
>>> +		.dumpit = devlink_nl_instance_iter_dump,
>> 
>> devlink_nl_instance_iter_dumpit to match ".dumpit".
>> 
>> 
>>> 		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
>>> 		/* can be retrieved by unprivileged users */
>>> 	},
>> 
>> [...]