Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
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
>> Sent: Friday, November 22, 2019 3:23 AM
>> To: Lance Yang (Arm Technology China) ; ovs-
>> d...@openvswitch.org
>> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu
>> (Arm
>> Technology China) ; Jingzhao Ni (Arm Technology China)
>> ; nd ; Ruifeng Wang (Arm Technology China)
>>
>> 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, ) != 0) {
> 198return s->last_tsc;
> 199 }
> 200
> 201 v = val.tv_sec * UINT64_C(10) + 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
>>> Signed-off-by: Lance Yang
>>> ---
>>> 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(, 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
Re: [ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
Hi Ilya,
Thanks for your comments. Please take time to see the inline comments.
Best regards,
Lance
> -Original Message-
> From: Ilya Maximets
> Sent: Friday, November 22, 2019 3:23 AM
> To: Lance Yang (Arm Technology China) ; ovs-
> d...@openvswitch.org
> Cc: Jieqiang Wang (Arm Technology China) ; Gavin Hu
> (Arm
> Technology China) ; Jingzhao Ni (Arm Technology China)
> ; nd ; Ruifeng Wang (Arm Technology China)
>
> 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, ) != 0) {
198return s->last_tsc;
199 }
200
201 v = val.tv_sec * UINT64_C(10) + 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.
> 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
If this title looks fine, I will modify the title as above in the patch set v2.
> >
> > Reviewed-by: Yanqin Wei
> > Signed-off-by: Lance Yang
> > ---
> > 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(, 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
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 uninitailized 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? 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.' > > Reviewed-by: Yanqin Wei > Signed-off-by: Lance Yang > --- > 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(, 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. > /* DPDK is not available or returned unreliable value. > * Trying to estimate. */ > affinity = ovs_numa_thread_getaffinity_dump(); > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 1/4] dpif-netdev: Fix the pmd_perf_stats
When compiling Open vSwitch for aarch64, the compiler will warn about an uninitailized variable in lib/dpif-netdev-perf.c. It is necessary to initialize it to avoid build failure. Reviewed-by: Yanqin Wei Signed-off-by: Lance Yang --- 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(, 0, sizeof(s)); /* DPDK is not available or returned unreliable value. * Trying to estimate. */ affinity = ovs_numa_thread_getaffinity_dump(); -- 2.7.4 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
