On 10/21/22 17:53, Wilson Peng wrote:
> From: Wilson Peng <pweis...@vmware.com>
>
> The stats 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 counter is incorrect.
>
> From the output below, it is incorrect for the n_bytes counter via OpenFlow15
> on
> CMD ovs-ofctl dump-flows.
>
> In the patch, get_unaligned_u64__() will return network byte order value to
> caller
> on Windows.
>
> ovs-ofctl.exe -O OpenFlow15 dump-flows nsx-managed | findstr 1504762
> cookie=0x1e77082def43e867, duration=1868893.146s, table=4, n_packets=1504762,
> n_bytes=18446744071589406094, …
> cookie=0x2033543ed8e89cc1, duration=1868893.146s, table=4, n_packets=1504762,
> n_bytes=18446744071589406094, …
>
> ovs-ofctl.exe -O OpenFlow10 dump-flows nsx-managed | findstr 1504762
> cookie=0x1e77082def43e867, duration=1868902.796s, table=4, n_packets=1504762,
> n_bytes=2174821774, idle_age=59,
> cookie=0x2033543ed8e89cc1, duration=1868902.796s, table=4, n_packets=1504762,
> n_bytes=2174821774, idle_age=59,
>
> With the fix, new compiled ovs-ofctl10211.exe could dump the correct n_bytes
> counter
> Via OpenFlow15
> ovs-ofctl10211.exe -O OpenFlow15 dump-flows nsx-managed | findstr 1504762
> cookie=0x1e77082def43e867, duration=1868872.272s, table=4, n_packets=1504762,
> n_bytes=2174821774, reset_counts idle_age=29, …
> cookie=0x2033543ed8e89cc1, duration=1868872.272s, table=4, n_packets=1504762,
> n_bytes=2174821774, reset_counts idle_age=29, …
>
> Signed-off-by: Wilson Peng <pweis...@vmware.com>
> ---
> lib/unaligned.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/unaligned.h b/lib/unaligned.h
> index f40e4e10d..cceab6eb8 100644
> --- a/lib/unaligned.h
> +++ b/lib/unaligned.h
> @@ -91,6 +91,8 @@ 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);
>
> static inline uint16_t get_unaligned_u16(const uint16_t *p_)
> {
> @@ -126,6 +128,9 @@ static inline void put_unaligned_u32(uint32_t *p_,
> uint32_t x_)
>
> static inline uint64_t get_unaligned_u64__(const uint64_t *p_)
> {
> +#if defined(WIN32)
> + return get_32aligned_be64(p_);
Hi. Thanks for the updated patch.
But we can't really do that. The pointer in this function is not aligned,
so we can't just use 32aligned variant of the function.
What I meant in my reply to v1 is that if get_32aligned_be64() and
get_unaligned_be64() give different results, one of these functions
has a bug. Since get_32aligned_be64() is producing the correct
result, the get_unaligned_be64() implementation must be incorrect.
> +#else
> const uint8_t *p = (const uint8_t *) p_;
> return ntohll(((uint64_t) p[0] << 56)
> | ((uint64_t) p[1] << 48)
> @@ -135,6 +140,7 @@ static inline uint64_t get_unaligned_u64__(const uint64_t
> *p_)
> | (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.
So, what we need to do is to cast all other p[] elements before
performing a left shit, so we will not have this issue.
Why all? Because the size of the 'int' is not specified. If we
will run on a system with 16bit int, we will have an issue with
p[6] instead of p[4].
And we also need to perform appropriate casts in all other
similar functions, i.e. get_unaligned_be32 and get_unaligned_be16,
to avoid possible issues there, and just to be uniform.
Could you check if that fixes your issue and make that kind of
change with the explanation of what is actually going on?
It will need a following tag:
Fixes: afa3a93165f1 ("Add header for access to potentially unaligned data.")
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
