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=-2.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,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 AE50AC2F3A0 for ; Mon, 21 Jan 2019 13:54:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 730302084C for ; Mon, 21 Jan 2019 13:54:21 +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="Eq0p6a4+" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731473AbfAUNyU (ORCPT ); Mon, 21 Jan 2019 08:54:20 -0500 Received: from mail-wr1-f67.google.com ([209.85.221.67]:36150 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730700AbfAUNyT (ORCPT ); Mon, 21 Jan 2019 08:54:19 -0500 Received: by mail-wr1-f67.google.com with SMTP id u4so23454461wrp.3 for ; Mon, 21 Jan 2019 05:54:17 -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=nNB0HCf9B3rdlh2yeSJpThAUxjH/H0FCdIwxlXpY9yA=; b=Eq0p6a4+7O4Tqt/jYY7wOBnK4jT6hT0V4ih9alniBNFzBmQWgT7QPY5F9agrk7eKUw g687jKeq/AtA6BnfzyWEXGapeCsyY3mkstppBII7cuGN3z9MbCHlrCxQWISLxUvPcLK7 OwkkrYosb9Q/wlTdX1MLcEoZQu1yFPqy+CXZLCWm3TtlcX8scOkRBOeXzSDR9mBFZDdA PbEuvZukCUH58ueOaaYZtoH1IJ4R5TPDwXrwxR2ntnP5mCmXPIVSAiByAl+07naiFjMt j8Yl6M3zB5QUxosDdZh3spMEio9P9/B4C3pE7m+ruRZ2UYRz/mA4dh7r1atZIR9j7U2/ trMQ== 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=nNB0HCf9B3rdlh2yeSJpThAUxjH/H0FCdIwxlXpY9yA=; b=E4q8FpszNZIay0zlsUWrwNMiXklFhA3SS44xbhK4iz8z+77YIY2idB3/T/NxFgt1vL aPfIiBM760iSHbKGLBBvqaebsX7EOsfS+NC1kP3jByZaVembtE3qYEqAlbuiI2DShmYh pjw50lxLGGEJLCh7IifsutahzWW81JRi2z50scsnroqD9QVuVO3vZDE3A/O5zHtifZFK 91hcayGjWYA7cGW+2g4PmmSWYFZOreOqqCaZdbsOO+hxN7lHOk+pJDI2KbrOeOlBeYND SaactjEe0YE/qVrx1LArUQuuR8WaucnMeMeTMgB3CEaKjtJSUD0NvVnCCJpsGGjAgTdS RFZg== X-Gm-Message-State: AJcUukcEeI5iRSdROfOI8QUg7Egq+nikbfcIrH/JkMfKCwy1X502L3wT 1T92TD6FmxgZ5KtZ9Va/U+sHQw== X-Google-Smtp-Source: ALg8bN68UrQ3Aeu8sy1VataJNOrfhnM25v01wXxJNF4IQy7LHhJH0zBqiTq7IreY2pvSgPJjyJpfEw== X-Received: by 2002:a5d:628a:: with SMTP id k10mr27527979wru.254.1548078856675; Mon, 21 Jan 2019 05:54:16 -0800 (PST) Received: from localhost ([193.47.165.251]) by smtp.gmail.com with ESMTPSA id s16sm77799609wrt.77.2019.01.21.05.54.15 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 21 Jan 2019 05:54:16 -0800 (PST) Date: Mon, 21 Jan 2019 14:45:32 +0100 From: Jiri Pirko To: Eran Ben Elisha Cc: "netdev@vger.kernel.org" , Jiri Pirko , "David S. Miller" , Ariel Almog , Aya Levin , Moshe Shemesh Subject: Re: [PATCH net-next v2 09/11] net/mlx5e: Add TX reporter support Message-ID: <20190121134532.GG2228@nanopsycho> References: <1547762360-7075-1-git-send-email-eranbe@mellanox.com> <1547762360-7075-10-git-send-email-eranbe@mellanox.com> <20190120110618.GB2730@nanopsycho> <05e949b6-030c-e51f-bdf1-7f159a5cbb55@mellanox.com> <20190121121154.GF2228@nanopsycho> <48d40e8e-4dc9-dcfc-6d25-a5b58d9a2419@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48d40e8e-4dc9-dcfc-6d25-a5b58d9a2419@mellanox.com> 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 Mon, Jan 21, 2019 at 02:06:44PM CET, eranbe@mellanox.com wrote: > > >On 1/21/2019 2:11 PM, Jiri Pirko wrote: >> Mon, Jan 21, 2019 at 12:32:07PM CET, eranbe@mellanox.com wrote: >>> >>> >>> On 1/20/2019 1:06 PM, Jiri Pirko wrote: >>>> Thu, Jan 17, 2019 at 10:59:18PM CET, eranbe@mellanox.com wrote: >> >> [...] >> >> >>>>> +static int >>>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer, >>>>> + u32 sqn, u8 state, u8 stopped) >>>>> +{ >>>>> + 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); >>>> >>>> No. The whole idea of having the message build up with nested attributes >>>> (json-like) was to avoid things like this. No sprintfs please. If you >>>> want to do sprintf, most of the time you are doing something wrong. >>> >>> I wanted that each SQ object will have a unique name. But I can merge >>> the sqn into its attributes instead. >> >> Should be an array. > >Every SQ shall hold it's own properties. I don't see why all SQs shall >be held under one array, this array do not provide any additional >info/order. If you have 8 SQs, you should have 8 objects (with subobjects) in an array. > >In addition, it is better to keep main preliminary objects up to the >size of one netlink SKB, otherwise it will be impossible for the devlink >to prepare this one big object as skb fragments, and also to re-assmble >them in user space as driver intended them to be. > >If all SWs will be under one big array, under one big object, we will >hit this corner case. > >> >> >>> >>>> >>>> >>>>> + 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; >>>>> + >>>>> + err = devlink_health_buffer_nest_start(buffer, >>>>> + DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE); >>>> >>>> How could you put an object name and don't start nesting? It looks >>>> implicit to me. >>> >>> I will add some helper functions that you could review. Just keep in >>> mind the implicit nest start must come with implicit nest end, so it >>> won't fit into every use... >> >> You can do just object_start(), object_finish() or something like that. > >ack. > >Better to have also helpers that start and close their nests on one >shot. like pair_name_value or pair_name_value_array. > >Also, The json is too powerful to close it inside few wrappers, we must >give the basic blocks as well for nontraditional structures. > > >> >> [...] >>