From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 16DB7A0507; Fri, 1 Apr 2022 15:26:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A9B1342911; Fri, 1 Apr 2022 15:26:29 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 589E54067E for ; Fri, 1 Apr 2022 15:26:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1648819587; x=1680355587; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=MPQqI8ziwpUJaLZP0FjWbJsSRpKGzbaSci86IlfyUcg=; b=W9gk0XXg4oYYG/glGSWGpHZ6hwNn14wBrXzcrSyetUfgoqB+0LhBYL3N FIsnQ2mrdbTPahCwCF8Hk1NJsm1QhEu8ywGXSqrp/jgUkmasgbSZK7lHD rF5uCHIpb6LC8rLoS9hb06WsVANv/BbL3PNM9rbYAFpJNvte1L+kekaoo SN7HnkhX6a5e9vpicAZDHwzyO6mAYjWsvrxyMl5pxUIHmlRour8Rv/43n Zseh5HQsAOq8uaFlsAnrZ4vMkJD0sYhnjZogsRIaJsK734JkI7oz7hv5e vuUB9F8hFT6JhG912I3kE7ueZr4FGqnVAf9K0YMa4co5q8BShsUAJAKCw w==; X-IronPort-AV: E=McAfee;i="6200,9189,10303"; a="240073956" X-IronPort-AV: E=Sophos;i="5.90,227,1643702400"; d="scan'208";a="240073956" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Apr 2022 06:26:26 -0700 X-IronPort-AV: E=Sophos;i="5.90,227,1643702400"; d="scan'208";a="547798817" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.20.202]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 01 Apr 2022 06:26:24 -0700 Date: Fri, 1 Apr 2022 14:26:21 +0100 From: Bruce Richardson To: "Morrissey, Sean" Cc: "Walsh, Conor" , Chengwen Feng , "Laatz, Kevin" , "dev@dpdk.org" , "Pai G, Sunil" Subject: Re: [PATCH v3] dmadev: add telemetry support Message-ID: References: <20220331183946.2203233-1-sean.morrissey@intel.com> <20220401102402.2249057-1-sean.morrissey@intel.com> <5cf8942c-dd0e-187f-59b9-f1e1821d285a@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5cf8942c-dd0e-187f-59b9-f1e1821d285a@intel.com> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Apr 01, 2022 at 01:58:49PM +0100, Morrissey, Sean wrote: > > On 01/04/2022 12:00, Walsh, Conor wrote: > > > From: Bruce Richardson > > > Sent: Friday 1 April 2022 11:50 > > > To: Morrissey, Sean > > > Cc: Chengwen Feng ; Laatz, Kevin > > > ; dev@dpdk.org; Pai G, Sunil > > > > > > Subject: Re: [PATCH v3] dmadev: add telemetry support > > > > > > On Fri, Apr 01, 2022 at 10:24:02AM +0000, Sean Morrissey wrote: > > > > Telemetry commands are now registered through the dmadev library > > > > for the gathering of DSA stats. The corresponding callback > > > > functions for listing dmadevs and providing info and stats for a > > > > specific dmadev are implemented in the dmadev library. > > > > > > > > An example usage can be seen below: > > > > > > > > Connecting to /var/run/dpdk/rte/dpdk_telemetry.v2 > > > > {"version": "DPDK 22.03.0-rc2", "pid": 2956551, "max_output_len": 16384} > > > > Connected to application: "dpdk-dma" > > > > --> / > > > > {"/": ["/", "/dmadev/info", "/dmadev/list", "/dmadev/stats", ...]} > > > > --> /dmadev/list > > > > {"/dmadev/list": [0, 1]} > > > > --> /dmadev/info,0 > > > > {"/dmadev/info": {"name": "0000:00:01.0", "nb_vchans": 1, "numa_node": > > > 0, > > > > "max_vchans": 1, "max_desc": 4096, "min_desc": 32, "max_sges": 0, > > > > "capabilities": {"fill": 1, "sva": 0, "silent": 0, ...}}} > > > > --> /dmadev/stats,0,0 > > > > {"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}} > > > > > > > > Signed-off-by: Sean Morrissey > > > > Tested-by: Sunil Pai G > > > Reviewed-by: Bruce Richardson > > Hi Sean, > > > > I'd agree with Bruce's comment below about trying to keep the names the same. > > Looks good to me though and I've tested it with IOAT and dmafwd. > > > > Thanks, > > Reviewed-by: Conor Walsh > > > > > One comment inline below, which I'd like feedback from others on. > > > > --- > > > > V3: > > > > * update docs with correct examples > > > > * code cleanup and added comments > > > > > > > > > > + > > > > +#define ADD_CAPA(c, s) rte_tel_data_add_dict_int(dma_caps, #c, > > > !!(dev_capa & RTE_DMA_CAPA_ ## s)) > > > > + > > > > +static int > > > > +dmadev_handle_dev_info(const char *cmd __rte_unused, > > > > + const char *params, struct rte_tel_data *d) > > > > +{ > > > > + struct rte_dma_info dma_info; > > > > + struct rte_tel_data *dma_caps; > > > > > > > + dma_caps = rte_tel_data_alloc(); > > > > + if (!dma_caps) > > > > + return -ENOMEM; > > > > + > > > > + rte_tel_data_start_dict(dma_caps); > > > > + ADD_CAPA(fill, OPS_FILL); > > > > + ADD_CAPA(sva, SVA); > > > > + ADD_CAPA(silent, SILENT); > > > > + ADD_CAPA(copy, OPS_COPY); > > > > + ADD_CAPA(mem2mem, MEM_TO_MEM); > > > I'm not 100% sure about this approach of having slightly different names > > > compared to the flags, just to have things in lower-case. Looking to have > > > some more input here - I'd tend to have the capabilities in upper case to > > > avoid duplicating parameters, but I'm not massively concerned either way. > > Hi all, > > If that is the preferred approach then I will send another version. I got > the lower case > > names from the capa_names struct in the dma_capability_name() function and > these > > naming conventions are also used in the logs i.e. "Device %d don't support > mem2mem transfer". My apologies, I didn't realise that that function and list of names existed. Can that be used instead of hard-coding the names and values here? Can we iterate through the array of names and check if the relevant bit position is set? > > For this reason, I thought this was the preferred approach to naming the > capabilities, however > > I will keep the names consistent with the flags as suggested. > If there are printable names elsewhere that is what we should keep consistent with. However, we should not duplicate those in this code, but reuse existing defined names. /Bruce