From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB4DFC282C3 for ; Thu, 24 Jan 2019 09:18:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9ACCD2184C for ; Thu, 24 Jan 2019 09:18:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="n65c69NE" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726074AbfAXJSJ (ORCPT ); Thu, 24 Jan 2019 04:18:09 -0500 Received: from mail-wm1-f67.google.com ([209.85.128.67]:53580 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725987AbfAXJSJ (ORCPT ); Thu, 24 Jan 2019 04:18:09 -0500 Received: by mail-wm1-f67.google.com with SMTP id d15so2281235wmb.3 for ; Thu, 24 Jan 2019 01:18:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YVzmnW2dPLXikvpHZ4xAwgmlHm8AQYmkvxfaocQ9630=; b=n65c69NEmc4WZSUADvV/TZwe0PEQVMyG3Tk5r1xdgx7Ox5rK74wT/uS/lq1bYue2SD 0SXpyWGEtEsiXGbWgyPuqVoloKHhSx61qqjlvyG+XupyKHwIDp407NF43QXruSjveFfQ v0ulo5B3X/PtnyL9dogGdQJSHeXX1Cp+3VJC/5bxgT+QMfoCzd3tYF/FWb42G9dgIBMx q448kIZZPNK6zsXZKB+E5zL5fG9yT3wxocu+OJy7FBAUUDJdGBnnkTEXA0zZNKC/eDSQ 1rbx77Ep8TROmiL7jwc5Nebn+XERVq4ylcm5VnLWaP3NbceLWNbpIjSNVSK3vv4SXKzQ wlxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=YVzmnW2dPLXikvpHZ4xAwgmlHm8AQYmkvxfaocQ9630=; b=aQmGDZiiOj3lyysoyhAeAqzBkHYs99fYBQbohjGFQLDHgh8yAjA9In57PbmSJYH8Fs 7B3ZHWt1wzAB9odasGNQB1RGRQZlOjXq6kmEioVm+oREPUDD+fu7S2nyfmXwiM2GOBd1 NHPxwS5I4DM0zvJfjYB+D13bpP5oB2OFUJ3Lx9TUCAARPDFBFTmBBLELSkYkbnLp9Kp2 f05M5XQP6914SvpenIO7FjIaacyzdMFR6hXpPoRVfOBQ+f3B6fVytP7I2DykcT36TZDH uApg03gdAFPRIgQ2vffxe+SHfmUfNwvt4MT4rIH1lH467rqL/jfEprcCmDkzezjTog1E JHBA== X-Gm-Message-State: AJcUukdbzVYW3jNRHW8kpmSxlHQIDrXkhh2piR6KlvxQtfhQiqXpcIXE hIbLNiFTWdEzN0ME3w8WWaII+A== X-Google-Smtp-Source: ALg8bN4ncHN0v993WRIhFmdpUVG0yGqDWJF90tJRkPeFxDHIZItCJbVaSWtVZ0jzxy7zaxmuFKLwHQ== X-Received: by 2002:a1c:ab87:: with SMTP id u129mr1787369wme.104.1548321486986; Thu, 24 Jan 2019 01:18:06 -0800 (PST) Received: from localhost ([193.47.165.251]) by smtp.gmail.com with ESMTPSA id p12sm60332189wmi.5.2019.01.24.01.18.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 24 Jan 2019 01:18:06 -0800 (PST) Date: Thu, 24 Jan 2019 10:09:17 +0100 From: Jiri Pirko To: Eran Ben Elisha Cc: "netdev@vger.kernel.org" , Jiri Pirko , "David S. Miller" , Saeed Mahameed , Moshe Shemesh Subject: Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API Message-ID: <20190124090917.GB2357@nanopsycho> References: <1548172644-30862-1-git-send-email-eranbe@mellanox.com> <1548172644-30862-4-git-send-email-eranbe@mellanox.com> <20190123144228.GI2191@nanopsycho> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.5 (2018-04-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 >>> Reviewed-by: Moshe Shemesh >>> --- >>> .../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 >>>