On 10/25/22 09:37, Wilson Peng wrote: > From: Wilson Peng <[email protected]> > > The stats(byte_count) is got via function call > ofputil_decode_flow_stats_reply() > and for OpenFlow15 it will also call oxs_pull_entry__(). Currently we found on > Windows the byte_count counter is incorrect. It will get the byte_count on > OpenFlow15 > handling via ntohll(get_unaligned_be64(payload)) > > Quote the comments below from Ilya Maximets(thanks for the given soluton and > explanation) > > static inline uint64_t get_unaligned_u64__(const uint64_t *p_) > ... > return ntohll(((uint64_t) p[0] << 56) > | ((uint64_t) p[1] << 48) > | ((uint64_t) p[2] << 40) > | ((uint64_t) p[3] << 32) > | (p[4] << 24) > | (p[5] << 16) > | (p[6] << 8) > | p[7]); > And indeed the expression above has an issue with data types. > > The problem is the (p[4] << 24) part. The p[4] itself has a type > 'uint8_t' which is unsigned 8bit value. It is not enough to hold > the result of a left shift, so compiler automatically promotes it > to the 'int' by default. But it is *signed* 32bit value. > > In your original report p[4] was equal to 0x81. After the left > shift it became 0x81000000. Looks correct, but the type is 'int'. > The next operation that we do is '|' with the previous shifted > bytes that were explicitly converted to uint64_t before the left > shift. So we have uint64_t | int. In this case compiler needs > to extend the 'int' to 'unit64_t' before performing the operation. > And since the 'int' is signed and the sign bit happens to be set > in the 0x81000000, the sign extension is performed in order to > preserve the value. The result is 0xffffffff81000000. And that > is breaking everything else. > > From the new test below, it is incorrect for the n_bytes counter via > OpenFlow15 > On CMD: ovs-ofctl dump-flows. > > With the patch, get_unaligned_u64__() will return correct value to caller > on Windows. > > In the output (Got via original CMD without fix) below n_bytes 2177130813 will > be incorrectly changed to 18446744071591715133 when processing OpenFlow15 > which > is equal to 0xFFFFFFFF81C4613D and here the p[4] on Windows is 0x81. > > With the fix, new compiled ovs-ofctl1025.exe could dump the correct n_bytes > counter > Via OpenFlow15. > > ovs-ofctl.exe -O OpenFlow15 dump-flows nsx-managed | findstr 1516011 > cookie=0x1e77082def43e867, duration=2167263.984s, table=4, n_packets=1516011, > n_bytes=18446744071591715133, > cookie=0x2033543ed8e89cc1, duration=2167263.984s, table=4, n_packets=1516011, > n_bytes=18446744071591715133, > > ovs-ofctl.exe -O OpenFlow10 dump-flows nsx-managed | findstr 1516011 > cookie=0x1e77082def43e867, duration=2167271.690s, table=4, n_packets=1516011, > n_bytes=2177130813, > cookie=0x2033543ed8e89cc1, duration=2167271.690s, table=4, n_packets=1516011, > n_bytes=2177130813, > > ovs-ofctl.exe dump-flows nsx-managed | findstr 1516011 > cookie=0x1e77082def43e867, duration=2167282.103s, table=4, n_packets=1516011, > n_bytes=2177130813, > cookie=0x2033543ed8e89cc1, duration=2167282.103s, table=4, n_packets=1516011, > n_bytes=2177130813, > > With the fix, new compiled ovs-ofctl1025.exe could dump the correct n_bytes > counter > Via OpenFlow15. > > ovs-ofctl1025.exe -O OpenFlow15 dump-flows nsx-managed | findstr 1516011 > cookie=0x1e77082def43e867, duration=2167239.242s, table=4, n_packets=1516011, > n_bytes=2177130813, > cookie=0x2033543ed8e89cc1, duration=2167239.242s, table=4, n_packets=1516011, > n_bytes=2177130813, > > Fixes: afa3a93165f1 ("Add header for access to potentially unaligned data.") > Signed-off-by: Wilson Peng <[email protected]> > --- > lib/unaligned.h | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/lib/unaligned.h b/lib/unaligned.h > index f40e4e10d..022e8c976 100644 > --- a/lib/unaligned.h > +++ b/lib/unaligned.h > @@ -91,11 +91,13 @@ GCC_UNALIGNED_ACCESSORS(ovs_be32, be32); > GCC_UNALIGNED_ACCESSORS(ovs_be64, be64); > #else > /* Generic implementations. */ > +static inline ovs_be64 > +get_32aligned_be64(const ovs_32aligned_be64 *x);
This prototype is not needed anymore, so I removed it from the patch. With that, applied and backported down to 2.13. Thanks! Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
