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,URIBL_BLOCKED,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 973DFC282C3 for ; Thu, 24 Jan 2019 09:17:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 48B9E2184C for ; Thu, 24 Jan 2019 09:17:00 +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="TO88KP81" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726053AbfAXJQ7 (ORCPT ); Thu, 24 Jan 2019 04:16:59 -0500 Received: from mail-wm1-f68.google.com ([209.85.128.68]:51181 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725939AbfAXJQ6 (ORCPT ); Thu, 24 Jan 2019 04:16:58 -0500 Received: by mail-wm1-f68.google.com with SMTP id n190so2299051wmd.0 for ; Thu, 24 Jan 2019 01:16:57 -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=EsWzlMKZc0ZYir39b+iD3dZJk2XdkCN0++NT2EZk/Ts=; b=TO88KP81nvmazo8crXMYyXch4d+oIMwR1mWdiaveYRq5Sm0AOWXr6/3FsFTtN1/RPB A76K/mKFWTPe/Knf2FKYCA2wMl6KU/+I9MLn1HzqYB2xIxbzEBnySAkLtIl2Vs9D8DQ7 2H2R0nBMaRJ/m/Mkp4jMQx1RG5iK8+nnuWdjNyEDZckzwPPPTZwGLQC4TG7nH73QYm25 Y7wIrMFKJTJTNqJ/x6xtsEp2qZG91wtljtBrLM9/9HMLHJ2mvnu6X/ZzpZTntBO6YBmg kU4SntrYnDeKK3qh8KpVp1+K5MVQR5hKGjyW4aWfvykpOKyChnQJ90sNCKtz5k+U6WVs BLsA== 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=EsWzlMKZc0ZYir39b+iD3dZJk2XdkCN0++NT2EZk/Ts=; b=EY9soXcwOJFCQZPSWTRZT9E9NYAHI+OXv8jvFpI8sTrHpC4RcWfHoTz1FjvrZZlJve tHnC854Cuw4YGFLaXQzPBncVoqPgkZDRkqbNmuvdVtbiQJ4a5nY6tzIQ/M/2Ng4ihfUo hGsxkJoGUEVkJsr7cDs9q2Ft7T6YvJaY7p1BY9/zrZ3Icde7UKh7rdHP5RxOTA52hnzu n+R2o/DXm81B1NjI3jzmr2dKkfcOj6zvD+99Q3pRivACZBrkeGwdQJ9o/5a6rHrf/78w yetI56fGRorI/MkmMM/Z6hCfSL0lJN1sYYdN6QGP7BwTgvvR3UhUF3S/ab0IoBPHBVwU d33w== X-Gm-Message-State: AJcUuke7sP/NU9qXxorSKbDXp6I4P3P1dVv6Us6IUsjnR/gDEz1CCno0 p8OAE35ftL0JKNazjsX/lHGUjQ== X-Google-Smtp-Source: ALg8bN7A6d6NLStf7PKry1C07NFFfE9pPd65J/+rPCKW9lbzVinUIdL2PUnl+Fl1fAtML8j3sn6xxQ== X-Received: by 2002:a1c:a913:: with SMTP id s19mr1682455wme.4.1548321416362; Thu, 24 Jan 2019 01:16:56 -0800 (PST) Received: from localhost ([193.47.165.251]) by smtp.gmail.com with ESMTPSA id r77sm76312911wmd.22.2019.01.24.01.16.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 24 Jan 2019 01:16:55 -0800 (PST) Date: Thu, 24 Jan 2019 10:08:06 +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 2/7] net/mlx5e: Move driver to use devlink msg API Message-ID: <20190124090806.GA2357@nanopsycho> References: <1548172644-30862-1-git-send-email-eranbe@mellanox.com> <1548172644-30862-3-git-send-email-eranbe@mellanox.com> <20190123143930.GH2191@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:39:12AM CET, eranbe@mellanox.com wrote: > > >On 1/23/2019 4:39 PM, Jiri Pirko wrote: >> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote: >>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX >>> reporter used devlink health buffers API. Which will soon be depracated. >>> Modify the reporter to use the new devlink msg API. >>> >>> The actual set of the new diagnose function will be done later in the >>> downstream patch, only when devlink will actually expose a new diagnose >>> operation that uses the new API. >>> >>> Signed-off-by: Eran Ben Elisha >>> Reviewed-by: Moshe Shemesh >>> --- >>> .../mellanox/mlx5/core/en/reporter_tx.c | 123 ++++-------------- >>> 1 file changed, 26 insertions(+), 97 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 d9675afbb924..fc92850c214a 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c >>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter, >>> } >>> >>> static int >>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer, >>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx, >>> u32 sqn, u8 state, u8 stopped) >> >> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum. >> >stopped is the reply from netif_xmit_stopped, and it is a bool. If it is bool, you should add it into message as NLA_FLAG, not NLA_U8 >state is the HW state of the SQ. >it is defined in the ifc file: >enum { > MLX5_SQC_STATE_RST = 0x0, > MLX5_SQC_STATE_RDY = 0x1, > MLX5_SQC_STATE_ERR = 0x3, >}; >I could have translated it into strings, but I though it would be fine >to leave it as is. It's fine as u8 > >> >>> { >>> - int err, i; >>> - int nest = 0; >>> - char name[20]; >>> - >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT); >>> - if (err) >>> - goto buffer_error; >>> - nest++; >>> - >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR); >>> - if (err) >>> - goto buffer_error; >>> - nest++; >>> - >>> - sprintf(name, "SQ 0x%x", sqn); >>> - err = devlink_health_buffer_put_object_name(buffer, name); >>> - if (err) >>> - goto buffer_error; >>> - >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE); >>> - if (err) >>> - goto buffer_error; >>> - nest++; >>> - >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT); >>> - if (err) >>> - goto buffer_error; >>> - nest++; >>> - >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR); >>> - if (err) >>> - goto buffer_error; >>> - nest++; >>> - >>> - err = devlink_health_buffer_put_object_name(buffer, "HW state"); >>> - if (err) >>> - goto buffer_error; >>> + int err; >>> >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE); >>> + err = devlink_msg_object_start(msg_ctx, "SQ"); >>> if (err) >>> - goto buffer_error; >>> - nest++; >>> + return err; >>> >>> - err = devlink_health_buffer_put_value_u8(buffer, state); >>> + err = devlink_msg_object_start(msg_ctx, NULL); >>> if (err) >>> - goto buffer_error; >>> - >>> - devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */ >>> - nest--; >>> - >>> - devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */ >>> - nest--; >>> + return err; >>> >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR); >>> + err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn, >>> + sizeof(sqn), NLA_U32); >>> if (err) >>> - goto buffer_error; >>> - nest++; >>> + return err; >>> >>> - err = devlink_health_buffer_put_object_name(buffer, "stopped"); >>> + err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state, >>> + sizeof(state), NLA_U8); >>> if (err) >>> - goto buffer_error; >>> + return err; >>> >>> - err = devlink_health_buffer_nest_start(buffer, >>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE); >>> + err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped, >>> + sizeof(stopped), NLA_U8); >>> if (err) >>> - goto buffer_error; >>> - nest++; >>> + return err; >>> >>> - err = devlink_health_buffer_put_value_u8(buffer, stopped); >>> + err = devlink_msg_object_end(msg_ctx, NULL); >>> if (err) >>> - goto buffer_error; >>> - >>> - for (i = 0; i < nest; i++) >>> - devlink_health_buffer_nest_end(buffer); >>> - >>> - return 0; >>> + return err; >>> >>> -buffer_error: >>> - for (i = 0; i < nest; i++) >>> - devlink_health_buffer_nest_cancel(buffer); >>> + err = devlink_msg_object_end(msg_ctx, "SQ"); >>> return err; >>> } >>> >>> static int mlx5e_tx_reporter_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 mlx5e_priv *priv = devlink_health_reporter_priv(reporter); >>> - unsigned int buff = 0; >>> - int i = 0, err = 0; >>> - >>> - if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN) >>> - return -ENOMEM; >>> + int i, err = 0; >>> >>> mutex_lock(&priv->state_lock); >>> >>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter, >>> return 0; >>> } >>> >>> - while (i < priv->channels.num * priv->channels.params.num_tc) { >>> + for (i = 0; i < priv->channels.num * priv->channels.params.num_tc; >>> + i++) { >>> struct mlx5e_txqsq *sq = priv->txq2sq[i]; >>> u8 state; >>> >>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter, >>> if (err) >>> break; >>> >>> - err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff], >>> - sq->sqn, state, >>> + err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn, >>> + state, >>> netif_xmit_stopped(sq->txq)); >> >> This should be an array. On SQ entry : one member of array. >> >> So if you want to change it, you need to do this in 2 patches. One API, >> one the actual layout. :/ >> >> >> >>> - if (err) { >>> - if (++buff == num_buffers) >>> - break; >>> - } else { >>> - i++; >>> - } >>> + if (err) >>> + break; >>> } >>> >>> mutex_unlock(&priv->state_lock); >>> @@ -330,11 +264,6 @@ 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_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC * >>> - MLX5E_TX_REPORTER_PER_SQ_MAX_LEN, >>> - .diagnose = mlx5e_tx_reporter_diagnose, >> >> So you change the callback, remove it so the dissection is broken. > >This is needed in order to have this patch compiled. You have to figure it out without breakage. This is not the way to do this. > >> >> >> >>> - .dump_size = 0, >>> - .dump = NULL, >> >> >> This has 0 relation to the patch. Should be separate. > >ack. > >> >> >>> }; >>> >>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500 >>> -- >>> 2.17.1 >>>