From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 3A423DED for ; Tue, 28 Aug 2018 19:03:59 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Aug 2018 10:03:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,300,1531810800"; d="scan'208";a="79043979" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga003.jf.intel.com with ESMTP; 28 Aug 2018 10:03:57 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX152.ger.corp.intel.com (163.33.192.66) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 28 Aug 2018 18:03:56 +0100 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.180]) by irsmsx112.ger.corp.intel.com ([169.254.1.35]) with mapi id 14.03.0319.002; Tue, 28 Aug 2018 18:03:56 +0100 From: "Van Haaren, Harry" To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= , "Power, Ciara" CC: "Archbold, Brian" , "Kenny, Emma" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection socket Thread-Index: AQHUOtoApXnezM7r4ku8gbQ4FIIiQqTVVRAAgAAU3AA= Date: Tue, 28 Aug 2018 17:03:55 +0000 Message-ID: References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <1535026093-101872-3-git-send-email-ciara.power@intel.com> <20180828164002.azcco4o3spvi2yc7@bidouze.vm.6wind.com> In-Reply-To: <20180828164002.azcco4o3spvi2yc7@bidouze.vm.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTEwZmViNDktNTE3NC00Mjg0LTgyODQtNzI0NWY1ZGZjYjI4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWnpyRHAwTWp3OWFKS2J5YUlLRW5tQkdMdHlOc24yQnB1aVRyY1hTTlwvb2R4MEMxSmIzN1hsbmZxOGJxMzd0Q3YifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection socket X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Aug 2018 17:03:59 -0000 > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Tuesday, August 28, 2018 5:40 PM > To: Power, Ciara > Cc: Van Haaren, Harry ; Archbold, Brian > ; Kenny, Emma ; dev@dpdk.= org > Subject: Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection > socket > > +int32_t > > +rte_telemetry_check_port_activity(int port_id) > > +{ > > + int pid; > > + > > + RTE_ETH_FOREACH_DEV(pid) { > > + if (pid =3D=3D port_id) > > + return 1; > > + } > > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid, not active\n", > > + port_id); > > + return 0; > > +} > > + >=20 > This function seems more about ethdev than telemetry. > Maybe add it as part of librte_ethdev. Yep that might be a better place, making it a generic ethdev function. > The "active" semantic is blurry however. With this implementation, this > is checking that the device is currently attached and owned by the > default user. Should telemetry be limited to devices owned by default > user? Good question - perhaps one that we can get input on from others. Would you (app X) want App Y to read telemetry from your ethdev/cryptodev? Perhaps keeping telemetry to an "owned" port is a feature, perhaps a limitation. I'd like input on this one :D > Also, this function does not seem used in this patch, can it be added > when used? Sure. > > +static int32_t > > +rte_telemetry_reg_ethdev_to_metrics(uint16_t port_id) >=20 > "_to" might not be necessary. >=20 > > +{ > > + int ret, num_xstats, start_index, i; > > + struct rte_eth_xstat *eth_xstats; > > + > > + if (!rte_eth_dev_is_valid_port(port_id)) { > > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid\n", port_id); > > + return -EINVAL; > > + } > > + > > + num_xstats =3D rte_eth_xstats_get(port_id, NULL, 0); > > + if (num_xstats < 0) { > > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) failed:" > > + " %d\n", port_id, num_xstats); >=20 > I guess there isn't really a consensus yet, as the checkpatch.sh tool > might be misconfigured, but the cesura is very awkward here. >=20 > Same for other logs. Agreed, improvements possible here. > > + return -EPERM; > > + } > > + > > + eth_xstats =3D malloc(sizeof(struct rte_eth_xstat) * num_xstats); > > + if (eth_xstats =3D=3D NULL) { > > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for" > > + " xstats\n"); > > + return -ENOMEM; > > + } > > + ret =3D rte_eth_xstats_get(port_id, eth_xstats, num_xstats); > > + if (ret < 0 || ret > num_xstats) { > > + free(eth_xstats); > > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) len%i" > > + " failed: %d\n", port_id, num_xstats, ret); > > + return -EPERM; > > + } > > + struct rte_eth_xstat_name *eth_xstats_names; > > + eth_xstats_names =3D malloc(sizeof(struct rte_eth_xstat_name) * > > + num_xstats); > > + if (eth_xstats_names =3D=3D NULL) { >=20 > You are sometimes checking pointers against NULL, sometimes using "!". > You can choose either in your library, but it would be better to be > consistent and use a unified coding style. Heh, I guess that's down to dev-style, and you'll note multiple sign-offs ;= ) Can review for v2. > > + free(eth_xstats); > > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for" > > + " xstats_names\n"); > > + return -ENOMEM; > > + } > > + ret =3D rte_eth_xstats_get_names(port_id, eth_xstats_names, > > + num_xstats); > > + if (ret < 0 || ret > num_xstats) { > > + free(eth_xstats); > > + free(eth_xstats_names); > > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get_names(%u)" > > + " len%i failed: %d\n", port_id, num_xstats, > > + ret); > > + return -EPERM; > > + } > > + const char *xstats_names[num_xstats]; > > + > > + for (i =3D 0; i < num_xstats; i++) > > + xstats_names[i] =3D eth_xstats_names[eth_xstats[i].id].name; > > + > > + start_index =3D rte_metrics_reg_names(xstats_names, num_xstats); > > + > > + if (start_index < 0) { > > + TELEMETRY_LOG_ERR("Error - rte_metrics_reg_names failed -" > > + " metrics may already be registered\n"); > > + free(eth_xstats); > > + free(eth_xstats_names); > > + return -1; > > + } > > + free(eth_xstats_names); > > + free(eth_xstats); >=20 > At this point, you have repeated 3 times those frees(). > It would be cleaner to define proper labels and use goto instead. Agreed. Thanks for review!