On 11/01/2019 18:02, Ben Pfaff wrote:
> On Fri, Jan 11, 2019 at 05:35:45PM +0000, Pieter Jansen van Vuuren wrote:
>> On 11/01/2019 16:49, Ben Pfaff wrote:
>>> On Fri, Jan 11, 2019 at 11:51:53AM +0000, Pieter Jansen van Vuuren wrote:
>>>> +/* These functions specifically help shifting words in network
>>>> + * byte order, given that they are specified in host order. */
>>>> +static inline uint32_t
>>>> +shift_ovs_be32_left(uint32_t word, int shift)
>>>> +{
>>>> + uint32_t word_shifted = (OVS_FORCE uint32_t)htonl(word) << shift;
>>>> +
>>>> + return ntohl((OVS_FORCE ovs_be32)word_shifted);
>>>> +}
>>>> +
>>>> +static inline uint32_t
>>>> +shift_ovs_be32_right(uint32_t word, int shift)
>>>> +{
>>>> + uint32_t word_shifted = (OVS_FORCE uint32_t)htonl(word) >> shift;
>>>> +
>>>> + return ntohl((OVS_FORCE ovs_be32)word_shifted);
>>>> +}
>>>
>>> I don't understand how these new operations make sense. Can you
>>> explain?
>>>
>>
>> Sure,
>>
>> Background:
>> The original issue is that pedit performs set action operation per word, and
>> the way we are describing the translation from OvS to TC is by using byte
>> offsets(flower_pedit_map). However some fields like Traffic Class in IPv6
>> span
>> 2 pedit-words - one nibble of the field is in one word and the other nibble
>> is
>> in another word. In order to accommodate this a shift may be used.
>>
>> We need to do this shift in network order thus we need the conversion from
>> host
>> to network, but the types for htonl/ntohl either takes as parameter or
>> returns
>> ovs_be32. Doing the shift operation on ovs_be32 results in the compiler
>> degrades to integer:
>>
>> error: restricted ovs_be32 degrades to integer
>>
>> Passing that back to ntohl would result in:
>>
>> error: incorrect type in argument 1 (different base types)
>> expected restricted ovs_be32 [usertype] x
>> got unsigned int
>>
>> therefore we introduced these new operations. Hope this helps. I'm open to
>> alternate approaches to this problem.
>
> The part I don't understand is why doing a shift of a word in network
> byte order makes any sense. It will have different semantics depending
> on whether the host is little-endian or big-endian. Why is that
> desirable?
>
Oh, yes that makes sense. We would not want different semantics for little/big
endian machines.
One confusing aspect of the code is that for historical reasons the pedit deals
with values that are in network byte order using uint32_t variables. So I think
in this case the comment is incorrect and the usage of htonl/ntohl is inverted.
What we actually want is a shift of a value stored in network byte order, and
we want that shift done in host byte order (to avoid the problem you describe).
In terms of run-time the result would be the same as the current patch. But it
would be documented correctly. As a follow-up documentation of the byte order
used by pedit would likely be a good idea.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
