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 D590F423AC; Wed, 11 Jan 2023 13:38:26 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6D53840A7D; Wed, 11 Jan 2023 13:38:26 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id E266E40691 for ; Wed, 11 Jan 2023 13:38:24 +0100 (CET) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4NsRyQ0qHzzJrMn; Wed, 11 Jan 2023 20:37:02 +0800 (CST) Received: from [10.67.100.224] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Wed, 11 Jan 2023 20:38:19 +0800 Subject: Re: [PATCH v5 2/5] telemetry: fix repeated display when callback don't set dict To: Bruce Richardson CC: , , , , , References: <20221219090723.29356-1-fengchengwen@huawei.com> <20221219090723.29356-3-fengchengwen@huawei.com> <82dd43d3-661f-64dc-8ce0-a7b6b2a62d79@huawei.com> From: fengchengwen Message-ID: <33c09194-565d-bfc6-a8e3-a5c647d883c5@huawei.com> Date: Wed, 11 Jan 2023 20:38:19 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected 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 Hi Bruce, On 2023/1/7 1:33, Bruce Richardson wrote: > On Fri, Jan 06, 2023 at 04:07:45PM +0000, Bruce Richardson wrote: >> On Mon, Dec 26, 2022 at 12:53:57PM +0800, fengchengwen wrote: >>> On 2022/12/19 17:33, Bruce Richardson wrote: >>>> On Mon, Dec 19, 2022 at 09:07:20AM +0000, Chengwen Feng wrote: >>>>> When telemetry callback didn't set dict and return a non-negative >>>>> number, the telemetry will repeat to display the last result. >>>>> >>>>> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality") >>>>> Cc: stable@dpdk.org >>>>> >>>>> Signed-off-by: Chengwen Feng >>>>> --- >>>> >>>> Hi Chengwen, >>>> >>>> I'm a little curious about this bug. Can you describe some steps to >>>> reproduce it as I'm curious as to exactly what is happening. The fix seems >>>> a little strange to me so I'd like to investigate a little more to see if >>>> other approaches might work. >>> >>> Hi Bruce, >>> >>> Sorry for late reply. >>> >>> The steps: >>> 1. applay "[PATCH v5 1/5] dmadev: support stats reset telemetry command" >>> 2. compile >>> 3. start dpdk-dma: dpdk-dma -a DMA.BDF -a NIC.BDF -- -c hw >>> 4. start telemetry, and execute /dmadev/stats,0, and then /dmadev/stats_reset,0 >>> the output of /dmadev/stats_reset,0 will be the same of previous cmd "/dmadev/stats,0" >>> e.g. my environment: >>> --> /dmadev/stats,0 >>> { >>> "/dmadev/stats": { >>> "submitted": 23, >>> "completed": 23, >>> "errors": 0 >>> } >>> } >>> --> /dmadev/stats_reset,0 >>> { >>> "/dmadev/stats_reset": { >>> "submitted": 23, >>> "completed": 23, >>> "errors": 0 >>> } >>> } >>> >>> The rootcause is that the /dmadev/stats_reset don't set the outer parameter "struct rte_tel_data *info" >>> and return zero. >>> >> Thanks for the fuller explanation, I'll hopefully test it out myself. >> >> However, in the meantime, looking at the telemetry library code, would the >> following change work rather than explicitly always setting the telemetry >> data to a dictionary by default? Zeroing the data by default sets it to a >> null return which is what you probably want as default rather than an empty >> dictionary. (And it's also a smaller diff) >> >> /Bruce >> >> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c >> index 8fbb4f3060..7b905355cd 100644 >> --- a/lib/telemetry/telemetry.c >> +++ b/lib/telemetry/telemetry.c >> @@ -333,7 +333,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s) >> static void >> perform_command(telemetry_cb fn, const char *cmd, const char *param, int s) >> { >> - struct rte_tel_data data; >> + struct rte_tel_data data = {0}; >> >> int ret = fn(cmd, param, &data); >> if (ret < 0) { >> > > I've handily reproduced the issue using the instructions you gave above, > thanks for those. > > Based on that, and looking a little deeper: > > * I think it is an error with the reset function not to initialize and > complete the return value. I believe that for each telemetry callback we > should require that the callback fill in a valid value on success. For > reset, some options could be just a string "OK", or an array just > containing "[0]", or similar. good idea, the v2 follow it. > > * Given that point above, I do agree though that the "data" parameter > should be properly initialized on entry to the callbacks. However, I feel > that the correct init should be the empty/null value, as is given in the > patch above. already fix in v2 Thanks. > > Regards, > /Bruce > . >