From: Jiri Pirko <jiri@resnulli.us>
To: Eran Ben Elisha <eranbe@mellanox.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Jiri Pirko <jiri@mellanox.com>,
"David S. Miller" <davem@davemloft.net>,
Saeed Mahameed <saeedm@mellanox.com>,
Moshe Shemesh <moshe@mellanox.com>
Subject: Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API
Date: Thu, 24 Jan 2019 10:09:17 +0100 [thread overview]
Message-ID: <20190124090917.GB2357@nanopsycho> (raw)
In-Reply-To: <f1838db3-e32c-dcf2-5aa6-6756ea508e34@mellanox.com>
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
>>>
next prev parent reply other threads:[~2019-01-24 9:18 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-22 15:57 [PATCH net-next 0/7] Devlink health updates Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 1/7] devlink: Add devlink msg API Eran Ben Elisha
2019-01-23 14:31 ` Jiri Pirko
2019-01-24 7:31 ` Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 2/7] net/mlx5e: Move driver to use " Eran Ben Elisha
2019-01-23 14:39 ` Jiri Pirko
2019-01-24 7:39 ` Eran Ben Elisha
2019-01-24 9:08 ` Jiri Pirko
2019-01-24 9:57 ` Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 3/7] devlink: move devlink health reporter " Eran Ben Elisha
2019-01-23 14:42 ` Jiri Pirko
2019-01-24 7:45 ` Eran Ben Elisha
2019-01-24 9:09 ` Jiri Pirko [this message]
2019-01-24 9:57 ` Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 4/7] devlink: Delete depracated health buffers API Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 5/7] devlink: Remove spaces around "=" in the logger print Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 6/7] devlink: Fix use-after-free at reporter destroy Eran Ben Elisha
2019-01-22 15:57 ` [PATCH net-next 7/7] net/mlx5e: Add RTNL lock to TX recover flow Eran Ben Elisha
2019-01-22 16:58 ` [PATCH net-next 0/7] Devlink health updates Jiri Pirko
2019-01-25 6:08 ` David Miller
2019-01-25 9:16 ` Eran Ben Elisha
2019-01-25 9:19 ` Jiri Pirko
2019-01-25 18:56 ` David Miller
2019-01-23 11:44 ` Jiri Pirko
2019-01-23 12:34 ` Eran Ben Elisha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190124090917.GB2357@nanopsycho \
--to=jiri@resnulli.us \
--cc=davem@davemloft.net \
--cc=eranbe@mellanox.com \
--cc=jiri@mellanox.com \
--cc=moshe@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.