On 22.11.2019 10:20, Lance Yang (Arm Technology China) wrote:
> Hi Ilya,
>
> Thanks for your comments. Please take time to see the inline comments.
>
> Best regards,
> Lance
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@ovn.org>
>> Sent: Friday, November 22, 2019 3:23 AM
>> To: Lance Yang (Arm Technology China) <lance.y...@arm.com>; ovs-
>> d...@openvswitch.org
>> Cc: Jieqiang Wang (Arm Technology China) <jieqiang.w...@arm.com>; Gavin Hu
>> (Arm
>> Technology China) <gavin...@arm.com>; Jingzhao Ni (Arm Technology China)
>> <jingzhao...@arm.com>; nd <n...@arm.com>; Ruifeng Wang (Arm Technology China)
>> <ruifeng.w...@arm.com>
>> Subject: Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
>>
>> On 20.11.2019 9:13, Lance Yang wrote:
>>> When compiling Open vSwitch for aarch64, the compiler will warn about
>>> an uninitialized variable in lib/dpif-netdev-perf.c. It is necessary
>>> to initialize it to avoid build failure.
>>
>> Hi. Thanks for making OVS build on aarch64.
>>
>> Could you provide the error message?
>
> [Lance]
> For the error message you would like to see, I copied it below:
> --- Begin of the error message ---
> In file included from lib/dpif-netdev-perf.c:21:
> lib/dpif-netdev-perf.c: In function 'pmd_perf_estimate_tsc_frequency':
> lib/dpif-netdev-perf.h:198:16: error: 's.last_tsc' may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> return s->last_tsc;
> ~^~~~~~~~~~
> libtool: compile: gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib
> -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
> -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition
> -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing
> -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument
> -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow
> -Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT
> lib/heap.lo -MD -MP -MF lib/.deps/heap.Tpo -c lib/heap.c -o lib/heap.o
> cc1: all warnings being treated as errors
> --- end of the message---
>
> 191 static inline uint64_t
> 192 rdtsc_syscall(struct pmd_perf_stats *s)
> 193 {
> 194 struct timespec val;
> 195 uint64_t v;
> 196
> 197 if (clock_gettime(CLOCK_MONOTONIC_RAW, &val) != 0) {
> 198 return s->last_tsc;
> 199 }
> 200
> 201 v = val.tv_sec * UINT64_C(1000000000) + val.tv_nsec;
> 202 return s->last_tsc = v;
> 203 }
> 204 #endif
>
> When the clock_gettime function failed, the code "return s->last_tsc" will be
> reached.
Oh, I see. So, this is not a false-positive even if it's not able
to produce any real issue.
Please, add this information to the commit message, i.e. describe in words
in which condition we could use this uninitialized value.
>> In fact this variable is never used (only assigned), so this should be a
>> false-positive. So, I'd
>> like to look at the error message.
>>
>
>> Also, it might be better to rename the patch to something more specific. e.g.
>> 'dpif-netdev-perf: Avoid false-positive on uninitialized perf stats.'
> I typed an "Enter key" in the patch. The original title should be:
>
> Subject: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
> uninitalized issue
s/uninitalized/uninitialized/
And you still need change the area from 'dpif-netdev' to 'dpif-netdev-perf'.
And a period in the end.
Maybe something like this:
'dpif-netdev-perf: Fix using of uninitialized last_tsc.' ?
>
> If this title looks fine, I will modify the title as above in the patch set
> v2.
>
>>>
>>> Reviewed-by: Yanqin Wei <yanqin....@arm.com>
>>> Signed-off-by: Lance Yang <lance.y...@arm.com>
>>> ---
>>> lib/dpif-netdev-perf.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c index
>>> baf90b0..f85bb0c 100644
>>> --- a/lib/dpif-netdev-perf.c
>>> +++ b/lib/dpif-netdev-perf.c
>>> @@ -63,6 +63,7 @@ pmd_perf_estimate_tsc_frequency(void)
>>> struct pmd_perf_stats s;
>>> uint64_t start, stop;
>>>
>>> + memset(&s, 0, sizeof(s));
>>
>> Please, don't parenthesize the argument of a 'sizeof'.
>>
>> Also, it might be better to initialize the variable close to the first
>> usage. It doesn't look
>> good here.
>>
> [Lance]
> I will remove the parenthesize for the sizeof operator and move the struct
> initialization closer to where it is used.
>
>>> /* DPDK is not available or returned unreliable value.
>>> * Trying to estimate. */
>>> affinity = ovs_numa_thread_getaffinity_dump();
>>>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
