diff mbox series

[net] devlink: Fix reload stats structure

Message ID 1605879637-6114-1-git-send-email-moshe@mellanox.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] devlink: Fix reload stats structure | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 486 this patch: 486
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 673 this patch: 673
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Moshe Shemesh Nov. 20, 2020, 1:40 p.m. UTC 
Fix reload stats structure exposed to the user. Change stats structure
hierarchy to have the reload action as a parent of the stat entry and
then stat entry includes value per limit. This will also help to avoid
string concatenation on iproute2 output.

Reload stats structure before this fix:
"stats": {
    "reload": {
        "driver_reinit": 2,
        "fw_activate": 1,
        "fw_activate_no_reset": 0
     }
}

After this fix:
"stats": {
    "reload": {
        "driver_reinit": {
            "unspecified": 2
        },
        "fw_activate": {
            "unspecified": 1,
            "no_reset": 0
        }
}

Fixes: a254c264267e ("devlink: Add reload stats")
Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 include/uapi/linux/devlink.h |  2 ++
 net/core/devlink.c           | 48 +++++++++++++++++++++++-------------
 2 files changed, 33 insertions(+), 17 deletions(-)

Comments

Jakub Kicinski Nov. 21, 2020, 10:53 p.m. UTC | #1 
On Fri, 20 Nov 2020 15:40:37 +0200 Moshe Shemesh wrote:
> Fix reload stats structure exposed to the user. Change stats structure
> hierarchy to have the reload action as a parent of the stat entry and
> then stat entry includes value per limit. This will also help to avoid
> string concatenation on iproute2 output.
> 
> Reload stats structure before this fix:
> "stats": {
>     "reload": {
>         "driver_reinit": 2,
>         "fw_activate": 1,
>         "fw_activate_no_reset": 0
>      }
> }
> 
> After this fix:
> "stats": {
>     "reload": {
>         "driver_reinit": {
>             "unspecified": 2
>         },
>         "fw_activate": {
>             "unspecified": 1,
>             "no_reset": 0
>         }
> }
> 
> Fixes: a254c264267e ("devlink: Add reload stats")
> Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

At least try to fold the core networking code at 80 characters *please*.

You folded the comments at 86 chars, neither 100 nor 80.
Moshe Shemesh Nov. 23, 2020, 5:19 a.m. UTC | #2 
On 11/22/2020 12:53 AM, Jakub Kicinski wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, 20 Nov 2020 15:40:37 +0200 Moshe Shemesh wrote:
>> Fix reload stats structure exposed to the user. Change stats structure
>> hierarchy to have the reload action as a parent of the stat entry and
>> then stat entry includes value per limit. This will also help to avoid
>> string concatenation on iproute2 output.
>>
>> Reload stats structure before this fix:
>> "stats": {
>>      "reload": {
>>          "driver_reinit": 2,
>>          "fw_activate": 1,
>>          "fw_activate_no_reset": 0
>>       }
>> }
>>
>> After this fix:
>> "stats": {
>>      "reload": {
>>          "driver_reinit": {
>>              "unspecified": 2
>>          },
>>          "fw_activate": {
>>              "unspecified": 1,
>>              "no_reset": 0
>>          }
>> }
>>
>> Fixes: a254c264267e ("devlink: Add reload stats")
>> Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> At least try to fold the core networking code at 80 characters *please*.
>
> You folded the comments at 86 chars, neither 100 nor 80.


Oh, I missed that comment folding while replacing it in this patch. I 
will fix, thanks.
diff mbox series

Patch

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 0113bc4db9f5..5203f54a2be1 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -526,6 +526,8 @@  enum devlink_attr {
 	DEVLINK_ATTR_RELOAD_STATS_LIMIT,	/* u8 */
 	DEVLINK_ATTR_RELOAD_STATS_VALUE,	/* u32 */
 	DEVLINK_ATTR_REMOTE_RELOAD_STATS,	/* nested */
+	DEVLINK_ATTR_RELOAD_ACTION_INFO,        /* nested */
+	DEVLINK_ATTR_RELOAD_ACTION_STATS,       /* nested */
 
 	/* add new attributes above here, update the policy in devlink.c */
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ab4b1368904f..34d38abd74ee 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -517,8 +517,7 @@  devlink_reload_limit_is_supported(struct devlink *devlink, enum devlink_reload_l
 	return test_bit(limit, &devlink->ops->reload_limits);
 }
 
-static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_action action,
-				   enum devlink_reload_limit limit, u32 value)
+static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_limit limit, u32 value)
 {
 	struct nlattr *reload_stats_entry;
 
@@ -526,8 +525,7 @@  static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
 	if (!reload_stats_entry)
 		return -EMSGSIZE;
 
-	if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, action) ||
-	    nla_put_u8(msg, DEVLINK_ATTR_RELOAD_STATS_LIMIT, limit) ||
+	if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_STATS_LIMIT, limit) ||
 	    nla_put_u32(msg, DEVLINK_ATTR_RELOAD_STATS_VALUE, value))
 		goto nla_put_failure;
 	nla_nest_end(msg, reload_stats_entry);
@@ -540,7 +538,7 @@  static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
 
 static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
 {
-	struct nlattr *reload_stats_attr;
+	struct nlattr *reload_stats_attr, *action_info_attr, *action_stats_attr;
 	int i, j, stat_idx;
 	u32 value;
 
@@ -552,17 +550,27 @@  static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink
 	if (!reload_stats_attr)
 		return -EMSGSIZE;
 
-	for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
-		/* Remote stats are shown even if not locally supported. Stats
-		 * of actions with unspecified limit are shown though drivers
-		 * don't need to register unspecified limit.
-		 */
-		if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
-		    !devlink_reload_limit_is_supported(devlink, j))
+	for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) {
+		if ((!is_remote && !devlink_reload_action_is_supported(devlink, i)) ||
+		    i == DEVLINK_RELOAD_ACTION_UNSPEC)
 			continue;
-		for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) {
-			if ((!is_remote && !devlink_reload_action_is_supported(devlink, i)) ||
-			    i == DEVLINK_RELOAD_ACTION_UNSPEC ||
+		action_info_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTION_INFO);
+		if (!action_info_attr)
+			goto nla_put_failure;
+
+		if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, i))
+			goto action_info_nest_cancel;
+		action_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTION_STATS);
+		if (!action_stats_attr)
+			goto action_info_nest_cancel;
+
+		for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
+			/* Remote stats are shown even if not locally supported. Stats
+			 * of actions with unspecified limit are shown though drivers
+			 * don't need to register unspecified limit.
+			 */
+			if ((!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
+			     !devlink_reload_limit_is_supported(devlink, j)) ||
 			    devlink_reload_combination_is_invalid(i, j))
 				continue;
 
@@ -571,13 +579,19 @@  static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink
 				value = devlink->stats.reload_stats[stat_idx];
 			else
 				value = devlink->stats.remote_reload_stats[stat_idx];
-			if (devlink_reload_stat_put(msg, i, j, value))
-				goto nla_put_failure;
+			if (devlink_reload_stat_put(msg, j, value))
+				goto action_stats_nest_cancel;
 		}
+		nla_nest_end(msg, action_stats_attr);
+		nla_nest_end(msg, action_info_attr);
 	}
 	nla_nest_end(msg, reload_stats_attr);
 	return 0;
 
+action_stats_nest_cancel:
+	nla_nest_cancel(msg, action_stats_attr);
+action_info_nest_cancel:
+	nla_nest_cancel(msg, action_info_attr);
 nla_put_failure:
 	nla_nest_cancel(msg, reload_stats_attr);
 	return -EMSGSIZE;