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: sta...@dpdk.org
>>>>>
>>>>> Signed-off-by: Chengwen Feng <fengcheng...@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
> .
>
