From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <bruce.richardson@intel.com>
CC: <thomas@monjalon.net>, <ferruh.yigit@amd.com>,
<andrew.rybchenko@oktetlabs.ru>, <dev@dpdk.org>, <ciara.power@intel.com>,
<kevin.laatz@intel.com>
References: <20221219090723.29356-1-fengchengwen@huawei.com>
<20221219090723.29356-3-fengchengwen@huawei.com>
<Y6Av0Gl2HgoJf4Vx@bricha3-MOBL.ger.corp.intel.com>
<82dd43d3-661f-64dc-8ce0-a7b6b2a62d79@huawei.com>
<Y7hHUZ3e0ME3ufdr@bricha3-MOBL.ger.corp.intel.com>
<Y7hbUI8VkwFxRtDu@bricha3-MOBL.ger.corp.intel.com>
From: fengchengwen <fengchengwen@huawei.com>
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: <Y7hbUI8VkwFxRtDu@bricha3-MOBL.ger.corp.intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <fengchengwen@huawei.com>
>>>>> ---
>>>>
>>>> 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
> .
>