On 1/24/2019 11:09 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:45:05AM CET, era...@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:20PM CET, era...@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 <era...@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <mo...@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
>>>>
