diff mbox series

[net-next,3/7] devlink: move devlink health reporter to use devlink msg API

Message ID 1548172644-30862-4-git-send-email-eranbe@mellanox.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Devlink health updates | expand

Commit Message

Eran Ben Elisha Jan. 22, 2019, 3:57 p.m. UTC 
Move devlink reporter diagnose and dump operations to use the new msg API.
Redefine the signature of diagnose and dump operations and move the mlx5e
reporter to use it with the new format.

Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
---
 .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
 include/net/devlink.h                         |  9 +-
 net/core/devlink.c                            | 95 +++++--------------
 3 files changed, 28 insertions(+), 77 deletions(-)

Comments

Jiri Pirko Jan. 23, 2019, 2:42 p.m. UTC | #1 
Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>Move devlink reporter diagnose and dump operations to use the new msg API.
>Redefine the signature of diagnose and dump operations and move the mlx5e
>reporter to use it with the new format.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
> include/net/devlink.h                         |  9 +-
> net/core/devlink.c                            | 95 +++++--------------
> 3 files changed, 28 insertions(+), 77 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index fc92850c214a..7238cda670ba 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> 		.name = "TX",
> 		.recover = mlx5e_tx_reporter_recover,
>+		.diagnose = mlx5e_tx_reporter_diagnose,

Unrelated to this patch.


> };
> 
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index fe323e9b14e1..d66de8b80cc2 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -442,17 +442,12 @@ struct devlink_health_reporter;
> 
> struct devlink_health_reporter_ops {
> 	char *name;
>-	unsigned int dump_size;
>-	unsigned int diagnose_size;
> 	int (*recover)(struct devlink_health_reporter *reporter,
> 		       void *priv_ctx);
> 	int (*dump)(struct devlink_health_reporter *reporter,
>-		    struct devlink_health_buffer **buffers_array,
>-		    unsigned int buffer_size, unsigned int num_buffers,
>-		    void *priv_ctx);
>+		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>-			struct devlink_health_buffer **buffers_array,
>-			unsigned int buffer_size, unsigned int num_buffers);
>+			struct devlink_msg_ctx *msg_ctx);
> };
> 
> struct devlink_ops {
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 57ca096849b3..347b638e6f32 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
> 
> struct devlink_health_reporter {
> 	struct list_head list;
>-	struct devlink_health_buffer **dump_buffers_array;
>+	struct devlink_msg_ctx *dump_msg_ctx;
> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>-	struct devlink_health_buffer **diagnose_buffers_array;
>-	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */

Howcome you don't need the mutex anymore?


> 	void *priv;
> 	const struct devlink_health_reporter_ops *ops;
> 	struct devlink *devlink;
>@@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
> 		goto unlock;
> 	}
> 
>-	if (WARN_ON(ops->dump && !ops->dump_size) ||
>-	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>-	    WARN_ON(auto_recover && !ops->recover) ||
>+	if (WARN_ON(auto_recover && !ops->recover) ||
> 	    WARN_ON(graceful_period && !ops->recover)) {
> 		reporter = ERR_PTR(-EINVAL);
> 		goto unlock;
>@@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
> 		goto unlock;
> 	}
> 
>-	if (ops->dump) {
>-		reporter->dump_buffers_array =
>-			devlink_health_buffers_create(ops->dump_size);
>-		if (!reporter->dump_buffers_array) {
>-			kfree(reporter);
>-			reporter = ERR_PTR(-ENOMEM);
>-			goto unlock;
>-		}
>-	}
>-
>-	if (ops->diagnose) {
>-		reporter->diagnose_buffers_array =
>-			devlink_health_buffers_create(ops->diagnose_size);
>-		if (!reporter->diagnose_buffers_array) {
>-			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>-						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>-			kfree(reporter);
>-			reporter = ERR_PTR(-ENOMEM);
>-			goto unlock;
>-		}
>-	}
>-
> 	list_add_tail(&reporter->list, &devlink->reporter_list);
> 	mutex_init(&reporter->dump_lock);
>-	mutex_init(&reporter->diagnose_lock);
> 
> 	reporter->priv = priv;
> 	reporter->ops = ops;
>@@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
> {
> 	mutex_lock(&reporter->devlink->lock);
> 	list_del(&reporter->list);
>-	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>-	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>+	if (reporter->dump_msg_ctx)
>+		devlink_msg_ctx_free(reporter->dump_msg_ctx);
> 	kfree(reporter);
> 	mutex_unlock(&reporter->devlink->lock);
> }
>@@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
> 	if (reporter->dump_avail)
> 		return 0;
> 
>-	devlink_health_buffers_reset(reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>-	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_BUFFER_SIZE,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>-				     priv_ctx);
>+	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>+	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>+		err = PTR_ERR(reporter->dump_msg_ctx);
>+		reporter->dump_msg_ctx = NULL;
>+		return err;
>+	}
>+
>+	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>+				  priv_ctx);
> 	if (!err) {
> 		reporter->dump_avail = true;
> 		reporter->dump_ts = jiffies;
>@@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> {
> 	struct devlink *devlink = info->user_ptr[0];
> 	struct devlink_health_reporter *reporter;
>-	u64 num_of_buffers;
>+	struct devlink_msg_ctx *msg_ctx;
> 	int err;
> 
> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> 	if (!reporter->ops->diagnose)
> 		return -EOPNOTSUPP;
> 
>-	num_of_buffers =
>-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>+	msg_ctx = devlink_msg_ctx_alloc();
>+	if (IS_ERR_OR_NULL(msg_ctx))
>+		return PTR_ERR(msg_ctx);
> 
>-	mutex_lock(&reporter->diagnose_lock);
>-	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>-				     num_of_buffers);
>-
>-	err = reporter->ops->diagnose(reporter,
>-				      reporter->diagnose_buffers_array,
>-				      DEVLINK_HEALTH_BUFFER_SIZE,
>-				      num_of_buffers);
>+	err = reporter->ops->diagnose(reporter, msg_ctx);

So this is not needed to be in reporter now? Why it was needed before?



> 	if (err)
> 		goto out;
> 
>-	err = devlink_health_buffer_snd(info,
>-					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>-					0, reporter->diagnose_buffers_array,
>-					num_of_buffers);
>-	if (err)
>-		goto out;
>-
>-	mutex_unlock(&reporter->diagnose_lock);
>-	return 0;
>+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>+			      0, msg_ctx);
> 
> out:
>-	mutex_unlock(&reporter->diagnose_lock);
>+	devlink_msg_ctx_free(msg_ctx);
> 	return err;
> }
> 
>@@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
> {
> 	reporter->dump_avail = false;
> 	reporter->dump_ts = 0;
>-	devlink_health_buffers_reset(reporter->dump_buffers_array,
>-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>+	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>+	reporter->dump_msg_ctx = NULL;
> }
> 
> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>@@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> {
> 	struct devlink *devlink = info->user_ptr[0];
> 	struct devlink_health_reporter *reporter;
>-	u64 num_of_buffers;
> 	int err;
> 
> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> 	if (!reporter->ops->dump)
> 		return -EOPNOTSUPP;
> 
>-	num_of_buffers =
>-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>-
> 	mutex_lock(&reporter->dump_lock);
> 	err = devlink_health_do_dump(reporter, NULL);
> 	if (err)
> 		goto out;
> 
>-	err = devlink_health_buffer_snd(info,
>-					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>-					0, reporter->dump_buffers_array,
>-					num_of_buffers);
>+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>+			      0, reporter->dump_msg_ctx);
> 
> out:
> 	mutex_unlock(&reporter->dump_lock);
>-- 
>2.17.1
>
Eran Ben Elisha Jan. 24, 2019, 7:45 a.m. UTC | #2 
On 1/23/2019 4:42 PM, Jiri Pirko wrote:
> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>> Move devlink reporter diagnose and dump operations to use the new msg API.
>> Redefine the signature of diagnose and dump operations and move the mlx5e
>> reporter to use it with the new format.
>>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>> ---
>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>> include/net/devlink.h                         |  9 +-
>> net/core/devlink.c                            | 95 +++++--------------
>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> index fc92850c214a..7238cda670ba 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>> 		.name = "TX",
>> 		.recover = mlx5e_tx_reporter_recover,
>> +		.diagnose = mlx5e_tx_reporter_diagnose,
> 
> Unrelated to this patch.

ack.

> 
> 
>> };
>>
>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index fe323e9b14e1..d66de8b80cc2 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>
>> struct devlink_health_reporter_ops {
>> 	char *name;
>> -	unsigned int dump_size;
>> -	unsigned int diagnose_size;
>> 	int (*recover)(struct devlink_health_reporter *reporter,
>> 		       void *priv_ctx);
>> 	int (*dump)(struct devlink_health_reporter *reporter,
>> -		    struct devlink_health_buffer **buffers_array,
>> -		    unsigned int buffer_size, unsigned int num_buffers,
>> -		    void *priv_ctx);
>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>> -			struct devlink_health_buffer **buffers_array,
>> -			unsigned int buffer_size, unsigned int num_buffers);
>> +			struct devlink_msg_ctx *msg_ctx);
>> };
>>
>> struct devlink_ops {
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 57ca096849b3..347b638e6f32 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>
>> struct devlink_health_reporter {
>> 	struct list_head list;
>> -	struct devlink_health_buffer **dump_buffers_array;
>> +	struct devlink_msg_ctx *dump_msg_ctx;
>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>> -	struct devlink_health_buffer **diagnose_buffers_array;
>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
> 
> Howcome you don't need the mutex anymore?

Now, as data is allocated on the fly while diagnose_doit(), no need to 
store the diagnose  over the reporter anymore. So no need for any mutex 
locking in order to prepare and send it.

> 
> 
>> 	void *priv;
>> 	const struct devlink_health_reporter_ops *ops;
>> 	struct devlink *devlink;
>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>> 		goto unlock;
>> 	}
>>
>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>> -	    WARN_ON(auto_recover && !ops->recover) ||
>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>> 	    WARN_ON(graceful_period && !ops->recover)) {
>> 		reporter = ERR_PTR(-EINVAL);
>> 		goto unlock;
>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>> 		goto unlock;
>> 	}
>>
>> -	if (ops->dump) {
>> -		reporter->dump_buffers_array =
>> -			devlink_health_buffers_create(ops->dump_size);
>> -		if (!reporter->dump_buffers_array) {
>> -			kfree(reporter);
>> -			reporter = ERR_PTR(-ENOMEM);
>> -			goto unlock;
>> -		}
>> -	}
>> -
>> -	if (ops->diagnose) {
>> -		reporter->diagnose_buffers_array =
>> -			devlink_health_buffers_create(ops->diagnose_size);
>> -		if (!reporter->diagnose_buffers_array) {
>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>> -			kfree(reporter);
>> -			reporter = ERR_PTR(-ENOMEM);
>> -			goto unlock;
>> -		}
>> -	}
>> -
>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>> 	mutex_init(&reporter->dump_lock);
>> -	mutex_init(&reporter->diagnose_lock);
>>
>> 	reporter->priv = priv;
>> 	reporter->ops = ops;
>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>> {
>> 	mutex_lock(&reporter->devlink->lock);
>> 	list_del(&reporter->list);
>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>> +	if (reporter->dump_msg_ctx)
>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>> 	kfree(reporter);
>> 	mutex_unlock(&reporter->devlink->lock);
>> }
>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>> 	if (reporter->dump_avail)
>> 		return 0;
>>
>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>> -				     priv_ctx);
>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>> +		reporter->dump_msg_ctx = NULL;
>> +		return err;
>> +	}
>> +
>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>> +				  priv_ctx);
>> 	if (!err) {
>> 		reporter->dump_avail = true;
>> 		reporter->dump_ts = jiffies;
>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>> {
>> 	struct devlink *devlink = info->user_ptr[0];
>> 	struct devlink_health_reporter *reporter;
>> -	u64 num_of_buffers;
>> +	struct devlink_msg_ctx *msg_ctx;
>> 	int err;
>>
>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>> 	if (!reporter->ops->diagnose)
>> 		return -EOPNOTSUPP;
>>
>> -	num_of_buffers =
>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>> +	msg_ctx = devlink_msg_ctx_alloc();
>> +	if (IS_ERR_OR_NULL(msg_ctx))
>> +		return PTR_ERR(msg_ctx);
>>
>> -	mutex_lock(&reporter->diagnose_lock);
>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>> -				     num_of_buffers);
>> -
>> -	err = reporter->ops->diagnose(reporter,
>> -				      reporter->diagnose_buffers_array,
>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>> -				      num_of_buffers);
>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
> 
> So this is not needed to be in reporter now? Why it was needed before?

see reply above.

> 
> 
> 
>> 	if (err)
>> 		goto out;
>>
>> -	err = devlink_health_buffer_snd(info,
>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>> -					0, reporter->diagnose_buffers_array,
>> -					num_of_buffers);
>> -	if (err)
>> -		goto out;
>> -
>> -	mutex_unlock(&reporter->diagnose_lock);
>> -	return 0;
>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>> +			      0, msg_ctx);
>>
>> out:
>> -	mutex_unlock(&reporter->diagnose_lock);
>> +	devlink_msg_ctx_free(msg_ctx);
>> 	return err;
>> }
>>
>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>> {
>> 	reporter->dump_avail = false;
>> 	reporter->dump_ts = 0;
>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>> +	reporter->dump_msg_ctx = NULL;
>> }
>>
>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> {
>> 	struct devlink *devlink = info->user_ptr[0];
>> 	struct devlink_health_reporter *reporter;
>> -	u64 num_of_buffers;
>> 	int err;
>>
>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>> 	if (!reporter->ops->dump)
>> 		return -EOPNOTSUPP;
>>
>> -	num_of_buffers =
>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>> -
>> 	mutex_lock(&reporter->dump_lock);
>> 	err = devlink_health_do_dump(reporter, NULL);
>> 	if (err)
>> 		goto out;
>>
>> -	err = devlink_health_buffer_snd(info,
>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>> -					0, reporter->dump_buffers_array,
>> -					num_of_buffers);
>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>> +			      0, reporter->dump_msg_ctx);
>>
>> out:
>> 	mutex_unlock(&reporter->dump_lock);
>> -- 
>> 2.17.1
>>
Jiri Pirko Jan. 24, 2019, 9:09 a.m. UTC | #3 
Thu, Jan 24, 2019 at 08:45:05AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>>> Move devlink reporter diagnose and dump operations to use the new msg API.
>>> Redefine the signature of diagnose and dump operations and move the mlx5e
>>> reporter to use it with the new format.
>>>
>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>> ---
>>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>>> include/net/devlink.h                         |  9 +-
>>> net/core/devlink.c                            | 95 +++++--------------
>>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> index fc92850c214a..7238cda670ba 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>> 		.name = "TX",
>>> 		.recover = mlx5e_tx_reporter_recover,
>>> +		.diagnose = mlx5e_tx_reporter_diagnose,
>> 
>> Unrelated to this patch.
>
>ack.
>
>> 
>> 
>>> };
>>>
>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index fe323e9b14e1..d66de8b80cc2 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>>
>>> struct devlink_health_reporter_ops {
>>> 	char *name;
>>> -	unsigned int dump_size;
>>> -	unsigned int diagnose_size;
>>> 	int (*recover)(struct devlink_health_reporter *reporter,
>>> 		       void *priv_ctx);
>>> 	int (*dump)(struct devlink_health_reporter *reporter,
>>> -		    struct devlink_health_buffer **buffers_array,
>>> -		    unsigned int buffer_size, unsigned int num_buffers,
>>> -		    void *priv_ctx);
>>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>>> -			struct devlink_health_buffer **buffers_array,
>>> -			unsigned int buffer_size, unsigned int num_buffers);
>>> +			struct devlink_msg_ctx *msg_ctx);
>>> };
>>>
>>> struct devlink_ops {
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 57ca096849b3..347b638e6f32 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>>
>>> struct devlink_health_reporter {
>>> 	struct list_head list;
>>> -	struct devlink_health_buffer **dump_buffers_array;
>>> +	struct devlink_msg_ctx *dump_msg_ctx;
>>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>> -	struct devlink_health_buffer **diagnose_buffers_array;
>>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
>> 
>> Howcome you don't need the mutex anymore?
>
>Now, as data is allocated on the fly while diagnose_doit(), no need to 
>store the diagnose  over the reporter anymore. So no need for any mutex 
>locking in order to prepare and send it.

Why the data is allocated on the fly? Shouldn't it be a separate patch
as it looks like a separate change?


>
>> 
>> 
>>> 	void *priv;
>>> 	const struct devlink_health_reporter_ops *ops;
>>> 	struct devlink *devlink;
>>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>> 		goto unlock;
>>> 	}
>>>
>>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>>> -	    WARN_ON(auto_recover && !ops->recover) ||
>>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>>> 	    WARN_ON(graceful_period && !ops->recover)) {
>>> 		reporter = ERR_PTR(-EINVAL);
>>> 		goto unlock;
>>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>>> 		goto unlock;
>>> 	}
>>>
>>> -	if (ops->dump) {
>>> -		reporter->dump_buffers_array =
>>> -			devlink_health_buffers_create(ops->dump_size);
>>> -		if (!reporter->dump_buffers_array) {
>>> -			kfree(reporter);
>>> -			reporter = ERR_PTR(-ENOMEM);
>>> -			goto unlock;
>>> -		}
>>> -	}
>>> -
>>> -	if (ops->diagnose) {
>>> -		reporter->diagnose_buffers_array =
>>> -			devlink_health_buffers_create(ops->diagnose_size);
>>> -		if (!reporter->diagnose_buffers_array) {
>>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>>> -			kfree(reporter);
>>> -			reporter = ERR_PTR(-ENOMEM);
>>> -			goto unlock;
>>> -		}
>>> -	}
>>> -
>>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>>> 	mutex_init(&reporter->dump_lock);
>>> -	mutex_init(&reporter->diagnose_lock);
>>>
>>> 	reporter->priv = priv;
>>> 	reporter->ops = ops;
>>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>>> {
>>> 	mutex_lock(&reporter->devlink->lock);
>>> 	list_del(&reporter->list);
>>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>>> +	if (reporter->dump_msg_ctx)
>>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>> 	kfree(reporter);
>>> 	mutex_unlock(&reporter->devlink->lock);
>>> }
>>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>>> 	if (reporter->dump_avail)
>>> 		return 0;
>>>
>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>>> -				     priv_ctx);
>>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>>> +		reporter->dump_msg_ctx = NULL;
>>> +		return err;
>>> +	}
>>> +
>>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>>> +				  priv_ctx);
>>> 	if (!err) {
>>> 		reporter->dump_avail = true;
>>> 		reporter->dump_ts = jiffies;
>>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>> {
>>> 	struct devlink *devlink = info->user_ptr[0];
>>> 	struct devlink_health_reporter *reporter;
>>> -	u64 num_of_buffers;
>>> +	struct devlink_msg_ctx *msg_ctx;
>>> 	int err;
>>>
>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>> 	if (!reporter->ops->diagnose)
>>> 		return -EOPNOTSUPP;
>>>
>>> -	num_of_buffers =
>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>>> +	msg_ctx = devlink_msg_ctx_alloc();
>>> +	if (IS_ERR_OR_NULL(msg_ctx))
>>> +		return PTR_ERR(msg_ctx);
>>>
>>> -	mutex_lock(&reporter->diagnose_lock);
>>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>>> -				     num_of_buffers);
>>> -
>>> -	err = reporter->ops->diagnose(reporter,
>>> -				      reporter->diagnose_buffers_array,
>>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>>> -				      num_of_buffers);
>>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
>> 
>> So this is not needed to be in reporter now? Why it was needed before?
>
>see reply above.
>
>> 
>> 
>> 
>>> 	if (err)
>>> 		goto out;
>>>
>>> -	err = devlink_health_buffer_snd(info,
>>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>> -					0, reporter->diagnose_buffers_array,
>>> -					num_of_buffers);
>>> -	if (err)
>>> -		goto out;
>>> -
>>> -	mutex_unlock(&reporter->diagnose_lock);
>>> -	return 0;
>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>> +			      0, msg_ctx);
>>>
>>> out:
>>> -	mutex_unlock(&reporter->diagnose_lock);
>>> +	devlink_msg_ctx_free(msg_ctx);
>>> 	return err;
>>> }
>>>
>>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>>> {
>>> 	reporter->dump_avail = false;
>>> 	reporter->dump_ts = 0;
>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>> +	reporter->dump_msg_ctx = NULL;
>>> }
>>>
>>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> {
>>> 	struct devlink *devlink = info->user_ptr[0];
>>> 	struct devlink_health_reporter *reporter;
>>> -	u64 num_of_buffers;
>>> 	int err;
>>>
>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>> 	if (!reporter->ops->dump)
>>> 		return -EOPNOTSUPP;
>>>
>>> -	num_of_buffers =
>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>>> -
>>> 	mutex_lock(&reporter->dump_lock);
>>> 	err = devlink_health_do_dump(reporter, NULL);
>>> 	if (err)
>>> 		goto out;
>>>
>>> -	err = devlink_health_buffer_snd(info,
>>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>> -					0, reporter->dump_buffers_array,
>>> -					num_of_buffers);
>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>> +			      0, reporter->dump_msg_ctx);
>>>
>>> out:
>>> 	mutex_unlock(&reporter->dump_lock);
>>> -- 
>>> 2.17.1
>>>
Eran Ben Elisha Jan. 24, 2019, 9:57 a.m. UTC | #4 
On 1/24/2019 11:09 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:45:05AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>>>> Move devlink reporter diagnose and dump operations to use the new msg API.
>>>> Redefine the signature of diagnose and dump operations and move the mlx5e
>>>> reporter to use it with the new format.
>>>>
>>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>>> ---
>>>> .../mellanox/mlx5/core/en/reporter_tx.c       |  1 +
>>>> include/net/devlink.h                         |  9 +-
>>>> net/core/devlink.c                            | 95 +++++--------------
>>>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> index fc92850c214a..7238cda670ba 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>>> 		.name = "TX",
>>>> 		.recover = mlx5e_tx_reporter_recover,
>>>> +		.diagnose = mlx5e_tx_reporter_diagnose,
>>>
>>> Unrelated to this patch.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>> index fe323e9b14e1..d66de8b80cc2 100644
>>>> --- a/include/net/devlink.h
>>>> +++ b/include/net/devlink.h
>>>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>>>
>>>> struct devlink_health_reporter_ops {
>>>> 	char *name;
>>>> -	unsigned int dump_size;
>>>> -	unsigned int diagnose_size;
>>>> 	int (*recover)(struct devlink_health_reporter *reporter,
>>>> 		       void *priv_ctx);
>>>> 	int (*dump)(struct devlink_health_reporter *reporter,
>>>> -		    struct devlink_health_buffer **buffers_array,
>>>> -		    unsigned int buffer_size, unsigned int num_buffers,
>>>> -		    void *priv_ctx);
>>>> +		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>>>> 	int (*diagnose)(struct devlink_health_reporter *reporter,
>>>> -			struct devlink_health_buffer **buffers_array,
>>>> -			unsigned int buffer_size, unsigned int num_buffers);
>>>> +			struct devlink_msg_ctx *msg_ctx);
>>>> };
>>>>
>>>> struct devlink_ops {
>>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>>> index 57ca096849b3..347b638e6f32 100644
>>>> --- a/net/core/devlink.c
>>>> +++ b/net/core/devlink.c
>>>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>>>
>>>> struct devlink_health_reporter {
>>>> 	struct list_head list;
>>>> -	struct devlink_health_buffer **dump_buffers_array;
>>>> +	struct devlink_msg_ctx *dump_msg_ctx;
>>>> 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>>> -	struct devlink_health_buffer **diagnose_buffers_array;
>>>> -	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
>>>
>>> Howcome you don't need the mutex anymore?
>>
>> Now, as data is allocated on the fly while diagnose_doit(), no need to
>> store the diagnose  over the reporter anymore. So no need for any mutex
>> locking in order to prepare and send it.
> 
> Why the data is allocated on the fly? Shouldn't it be a separate patch
> as it looks like a separate change?
> 
This start to be really complicated (but possible).

(#1) I think we shall take your approach and do a revert and put it all 
again on top.

> 
>>
>>>
>>>
>>>> 	void *priv;
>>>> 	const struct devlink_health_reporter_ops *ops;
>>>> 	struct devlink *devlink;
>>>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> 		goto unlock;
>>>> 	}
>>>>
>>>> -	if (WARN_ON(ops->dump && !ops->dump_size) ||
>>>> -	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>>>> -	    WARN_ON(auto_recover && !ops->recover) ||
>>>> +	if (WARN_ON(auto_recover && !ops->recover) ||
>>>> 	    WARN_ON(graceful_period && !ops->recover)) {
>>>> 		reporter = ERR_PTR(-EINVAL);
>>>> 		goto unlock;
>>>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> 		goto unlock;
>>>> 	}
>>>>
>>>> -	if (ops->dump) {
>>>> -		reporter->dump_buffers_array =
>>>> -			devlink_health_buffers_create(ops->dump_size);
>>>> -		if (!reporter->dump_buffers_array) {
>>>> -			kfree(reporter);
>>>> -			reporter = ERR_PTR(-ENOMEM);
>>>> -			goto unlock;
>>>> -		}
>>>> -	}
>>>> -
>>>> -	if (ops->diagnose) {
>>>> -		reporter->diagnose_buffers_array =
>>>> -			devlink_health_buffers_create(ops->diagnose_size);
>>>> -		if (!reporter->diagnose_buffers_array) {
>>>> -			devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> -						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>>>> -			kfree(reporter);
>>>> -			reporter = ERR_PTR(-ENOMEM);
>>>> -			goto unlock;
>>>> -		}
>>>> -	}
>>>> -
>>>> 	list_add_tail(&reporter->list, &devlink->reporter_list);
>>>> 	mutex_init(&reporter->dump_lock);
>>>> -	mutex_init(&reporter->diagnose_lock);
>>>>
>>>> 	reporter->priv = priv;
>>>> 	reporter->ops = ops;
>>>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>>>> {
>>>> 	mutex_lock(&reporter->devlink->lock);
>>>> 	list_del(&reporter->list);
>>>> -	devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> -	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>>>> -				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>>>> +	if (reporter->dump_msg_ctx)
>>>> +		devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> 	kfree(reporter);
>>>> 	mutex_unlock(&reporter->devlink->lock);
>>>> }
>>>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>>>> 	if (reporter->dump_avail)
>>>> 		return 0;
>>>>
>>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> -	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_BUFFER_SIZE,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>>>> -				     priv_ctx);
>>>> +	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>>>> +	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>>>> +		err = PTR_ERR(reporter->dump_msg_ctx);
>>>> +		reporter->dump_msg_ctx = NULL;
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>>>> +				  priv_ctx);
>>>> 	if (!err) {
>>>> 		reporter->dump_avail = true;
>>>> 		reporter->dump_ts = jiffies;
>>>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> {
>>>> 	struct devlink *devlink = info->user_ptr[0];
>>>> 	struct devlink_health_reporter *reporter;
>>>> -	u64 num_of_buffers;
>>>> +	struct devlink_msg_ctx *msg_ctx;
>>>> 	int err;
>>>>
>>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> 	if (!reporter->ops->diagnose)
>>>> 		return -EOPNOTSUPP;
>>>>
>>>> -	num_of_buffers =
>>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>>>> +	msg_ctx = devlink_msg_ctx_alloc();
>>>> +	if (IS_ERR_OR_NULL(msg_ctx))
>>>> +		return PTR_ERR(msg_ctx);
>>>>
>>>> -	mutex_lock(&reporter->diagnose_lock);
>>>> -	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>>>> -				     num_of_buffers);
>>>> -
>>>> -	err = reporter->ops->diagnose(reporter,
>>>> -				      reporter->diagnose_buffers_array,
>>>> -				      DEVLINK_HEALTH_BUFFER_SIZE,
>>>> -				      num_of_buffers);
>>>> +	err = reporter->ops->diagnose(reporter, msg_ctx);
>>>
>>> So this is not needed to be in reporter now? Why it was needed before?
>>
>> see reply above.
>>
>>>
>>>
>>>
>>>> 	if (err)
>>>> 		goto out;
>>>>
>>>> -	err = devlink_health_buffer_snd(info,
>>>> -					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> -					0, reporter->diagnose_buffers_array,
>>>> -					num_of_buffers);
>>>> -	if (err)
>>>> -		goto out;
>>>> -
>>>> -	mutex_unlock(&reporter->diagnose_lock);
>>>> -	return 0;
>>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> +			      0, msg_ctx);
>>>>
>>>> out:
>>>> -	mutex_unlock(&reporter->diagnose_lock);
>>>> +	devlink_msg_ctx_free(msg_ctx);
>>>> 	return err;
>>>> }
>>>>
>>>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>>>> {
>>>> 	reporter->dump_avail = false;
>>>> 	reporter->dump_ts = 0;
>>>> -	devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> -				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> +	devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> +	reporter->dump_msg_ctx = NULL;
>>>> }
>>>>
>>>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> {
>>>> 	struct devlink *devlink = info->user_ptr[0];
>>>> 	struct devlink_health_reporter *reporter;
>>>> -	u64 num_of_buffers;
>>>> 	int err;
>>>>
>>>> 	reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> 	if (!reporter->ops->dump)
>>>> 		return -EOPNOTSUPP;
>>>>
>>>> -	num_of_buffers =
>>>> -		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>>>> -
>>>> 	mutex_lock(&reporter->dump_lock);
>>>> 	err = devlink_health_do_dump(reporter, NULL);
>>>> 	if (err)
>>>> 		goto out;
>>>>
>>>> -	err = devlink_health_buffer_snd(info,
>>>> -					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> -					0, reporter->dump_buffers_array,
>>>> -					num_of_buffers);
>>>> +	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> +			      0, reporter->dump_msg_ctx);
>>>>
>>>> out:
>>>> 	mutex_unlock(&reporter->dump_lock);
>>>> -- 
>>>> 2.17.1
>>>>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
index fc92850c214a..7238cda670ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
@@ -264,6 +264,7 @@  static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
 static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
 		.name = "TX",
 		.recover = mlx5e_tx_reporter_recover,
+		.diagnose = mlx5e_tx_reporter_diagnose,
 };
 
 #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
diff --git a/include/net/devlink.h b/include/net/devlink.h
index fe323e9b14e1..d66de8b80cc2 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -442,17 +442,12 @@  struct devlink_health_reporter;
 
 struct devlink_health_reporter_ops {
 	char *name;
-	unsigned int dump_size;
-	unsigned int diagnose_size;
 	int (*recover)(struct devlink_health_reporter *reporter,
 		       void *priv_ctx);
 	int (*dump)(struct devlink_health_reporter *reporter,
-		    struct devlink_health_buffer **buffers_array,
-		    unsigned int buffer_size, unsigned int num_buffers,
-		    void *priv_ctx);
+		    struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
 	int (*diagnose)(struct devlink_health_reporter *reporter,
-			struct devlink_health_buffer **buffers_array,
-			unsigned int buffer_size, unsigned int num_buffers);
+			struct devlink_msg_ctx *msg_ctx);
 };
 
 struct devlink_ops {
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 57ca096849b3..347b638e6f32 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4555,10 +4555,8 @@  static int devlink_msg_snd(struct genl_info *info,
 
 struct devlink_health_reporter {
 	struct list_head list;
-	struct devlink_health_buffer **dump_buffers_array;
+	struct devlink_msg_ctx *dump_msg_ctx;
 	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
-	struct devlink_health_buffer **diagnose_buffers_array;
-	struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
 	void *priv;
 	const struct devlink_health_reporter_ops *ops;
 	struct devlink *devlink;
@@ -4619,9 +4617,7 @@  devlink_health_reporter_create(struct devlink *devlink,
 		goto unlock;
 	}
 
-	if (WARN_ON(ops->dump && !ops->dump_size) ||
-	    WARN_ON(ops->diagnose && !ops->diagnose_size) ||
-	    WARN_ON(auto_recover && !ops->recover) ||
+	if (WARN_ON(auto_recover && !ops->recover) ||
 	    WARN_ON(graceful_period && !ops->recover)) {
 		reporter = ERR_PTR(-EINVAL);
 		goto unlock;
@@ -4633,31 +4629,8 @@  devlink_health_reporter_create(struct devlink *devlink,
 		goto unlock;
 	}
 
-	if (ops->dump) {
-		reporter->dump_buffers_array =
-			devlink_health_buffers_create(ops->dump_size);
-		if (!reporter->dump_buffers_array) {
-			kfree(reporter);
-			reporter = ERR_PTR(-ENOMEM);
-			goto unlock;
-		}
-	}
-
-	if (ops->diagnose) {
-		reporter->diagnose_buffers_array =
-			devlink_health_buffers_create(ops->diagnose_size);
-		if (!reporter->diagnose_buffers_array) {
-			devlink_health_buffers_destroy(reporter->dump_buffers_array,
-						       DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
-			kfree(reporter);
-			reporter = ERR_PTR(-ENOMEM);
-			goto unlock;
-		}
-	}
-
 	list_add_tail(&reporter->list, &devlink->reporter_list);
 	mutex_init(&reporter->dump_lock);
-	mutex_init(&reporter->diagnose_lock);
 
 	reporter->priv = priv;
 	reporter->ops = ops;
@@ -4680,10 +4653,8 @@  devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
 {
 	mutex_lock(&reporter->devlink->lock);
 	list_del(&reporter->list);
-	devlink_health_buffers_destroy(reporter->dump_buffers_array,
-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
-	devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
-				       DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
+	if (reporter->dump_msg_ctx)
+		devlink_msg_ctx_free(reporter->dump_msg_ctx);
 	kfree(reporter);
 	mutex_unlock(&reporter->devlink->lock);
 }
@@ -4720,12 +4691,15 @@  static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
 	if (reporter->dump_avail)
 		return 0;
 
-	devlink_health_buffers_reset(reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
-	err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_BUFFER_SIZE,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
-				     priv_ctx);
+	reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
+	if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
+		err = PTR_ERR(reporter->dump_msg_ctx);
+		reporter->dump_msg_ctx = NULL;
+		return err;
+	}
+
+	err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
+				  priv_ctx);
 	if (!err) {
 		reporter->dump_avail = true;
 		reporter->dump_ts = jiffies;
@@ -4960,7 +4934,7 @@  static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_health_reporter *reporter;
-	u64 num_of_buffers;
+	struct devlink_msg_ctx *msg_ctx;
 	int err;
 
 	reporter = devlink_health_reporter_get_from_info(devlink, info);
@@ -4970,32 +4944,19 @@  static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
 	if (!reporter->ops->diagnose)
 		return -EOPNOTSUPP;
 
-	num_of_buffers =
-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
+	msg_ctx = devlink_msg_ctx_alloc();
+	if (IS_ERR_OR_NULL(msg_ctx))
+		return PTR_ERR(msg_ctx);
 
-	mutex_lock(&reporter->diagnose_lock);
-	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
-				     num_of_buffers);
-
-	err = reporter->ops->diagnose(reporter,
-				      reporter->diagnose_buffers_array,
-				      DEVLINK_HEALTH_BUFFER_SIZE,
-				      num_of_buffers);
+	err = reporter->ops->diagnose(reporter, msg_ctx);
 	if (err)
 		goto out;
 
-	err = devlink_health_buffer_snd(info,
-					DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
-					0, reporter->diagnose_buffers_array,
-					num_of_buffers);
-	if (err)
-		goto out;
-
-	mutex_unlock(&reporter->diagnose_lock);
-	return 0;
+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
+			      0, msg_ctx);
 
 out:
-	mutex_unlock(&reporter->diagnose_lock);
+	devlink_msg_ctx_free(msg_ctx);
 	return err;
 }
 
@@ -5004,8 +4965,8 @@  devlink_health_dump_clear(struct devlink_health_reporter *reporter)
 {
 	reporter->dump_avail = false;
 	reporter->dump_ts = 0;
-	devlink_health_buffers_reset(reporter->dump_buffers_array,
-				     DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
+	devlink_msg_ctx_free(reporter->dump_msg_ctx);
+	reporter->dump_msg_ctx = NULL;
 }
 
 static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
@@ -5013,7 +4974,6 @@  static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_health_reporter *reporter;
-	u64 num_of_buffers;
 	int err;
 
 	reporter = devlink_health_reporter_get_from_info(devlink, info);
@@ -5023,18 +4983,13 @@  static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
 	if (!reporter->ops->dump)
 		return -EOPNOTSUPP;
 
-	num_of_buffers =
-		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
-
 	mutex_lock(&reporter->dump_lock);
 	err = devlink_health_do_dump(reporter, NULL);
 	if (err)
 		goto out;
 
-	err = devlink_health_buffer_snd(info,
-					DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
-					0, reporter->dump_buffers_array,
-					num_of_buffers);
+	err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
+			      0, reporter->dump_msg_ctx);
 
 out:
 	mutex_unlock(&reporter->dump_lock);