<div dir="ltr"><div>Hi all,</div><div><br></div><div>[Robin] > I had not understood that it was *only* the lookups that were network order</div><div>[Morten] >When I saw the byte order flag the first time, it was not clear to me 
either that it only affected lookups - I too thought it covered the 
entire API of the library. This needs to be emphasized in the 
description of the flag. And the flag's name should contain LOOKUP</div><div>[Morten] > And/or rename RTE_FIB_F_NETWORK_ORDER to RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar.</div><div><br></div><div>There is a clear comment for this flag that it has effects on lookup. Repeating the statement with an exclamation mark seems too much. Moreover, at first this flag was named "RTE_FIB_FLAG_LOOKUP_BE" and it was suggested for renaming here:<br></div><div><a href="https://inbox.dpdk.org/dev/D4SWPKOPRD5Z.87YIET3Y4AW@redhat.com/">https://inbox.dpdk.org/dev/D4SWPKOPRD5Z.87YIET3Y4AW@redhat.com/</a></div><div><br></div><div>[Morten] >Control plane API should use CPU byte order ... adding it (support for network byte order) to the RIB library would be nice too.</div><div>I'm not sure if I understood you correctly here, RIB is a control plane library.</div><div><br></div><div>[Robin] > an IPv4 address is *not* an integer. It should be treated as an opaque value.</div><div>I don't agree here. IPv4 is 32 bits of information. CPUs usually can treat 32 bits of information as an integer, which is really useful.</div><div><br></div><div><div>[Morten] > Treating IPv4 addresses as byte arrays would allow simple memcmp() for range comparison</div><div>How is it possible for a general case? For example, I need to test IP addresses against range 1.1.1.7 - 10.20.30.37.</div><div><br></div><div>[Robin] >Also for consistency with IPv6, I really think that *all* addresses should be dealt in their network form.</div><div>There is no such a problem as byte order mismatch for IPv6 addresses since they can not be treated by modern CPUs as an native integer type.</div><div><br></div><div>[Robin] >But it (RTE_IPV4) will always generate addresses in *host order*. Which means they cannot be used in IPv4 headers without passing them through htonl().</div><div>RTE_IPV4 is not limited by setting IPv4 headers values.</div><div><br></div><div>[Robin] >Maybe we could revert that patch and defer a complete change of the rib/fib APIs to only expose network order addresses? <br></div><div><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">I</span><span style="white-space:pre-wrap"> don</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">'t</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">agree</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">with</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">that</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">.</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">Don</span><span style="white-space:pre-wrap">'t </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">limit</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">yourself</span><span style="white-space:pre-wrap"> to </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">just</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">manipulating</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">network</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">headers</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">.</span></div><div><br></div><div>[Robin] >Thinking about it some more. Having a flag for such a drastic change in behaviour does not seem right.<span class="gmail-im"><br></span></div></div><div>This flag is optional. I don't see any problems with that.</div><div><br></div><div>In general, here we just have different perspectives on the problem. I can see and understand your point.</div><div>My considerations are:</div><div>- The vast majority of the longest prefix match algorithms works with addresses in host byte order (binary trees, multibit tries, DXR, except only hash based lookup)</div><div>- If you do byteswap two or more times - <span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">If</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">you</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">run</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">byteswap</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">two</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">or</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">more</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">times</span><span style="white-space:pre-wrap">, </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">you</span><span style="white-space:pre-wrap"> are </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">probably</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">doing</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">something</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">wrong</span><span style="white-space:pre-wrap"> in </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">terms</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">of</span><span style="white-space:pre-wrap"> computations</span></div><div><br></div><div>So, feel free to submit patches adding this feature to the control plane API, but let's consider:</div><div>- default behaviour should remain the same. Why? At least because for my usecases I'd like to have "data representation" (byte swap) outside of the library. Not to mention ABI/API breakage</div><div>-  IPv4 should stay as uint32_t. <span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">C</span><span style="white-space:pre-wrap"> doesn</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">'t</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">know</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">such</span><span style="white-space:pre-wrap"> a </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">thing</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">as</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">byte</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">order</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">,</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">it</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">knows</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">about</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">size</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">and</span><span style="white-space:pre-wrap"> </span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">signedness</span><span class="gmail-EzKURWReUAB5oZgtQNkl" style="white-space:pre-wrap">.</span> rte_be32_t is just a hint for us - humans :)<br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">пт, 15 нояб. 2024 г. в 17:00, Stephen Hemminger <<a href="mailto:stephen@networkplumber.org">stephen@networkplumber.org</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, 15 Nov 2024 15:28:33 +0100<br>
"Robin Jarry" <<a href="mailto:rjarry@redhat.com" target="_blank">rjarry@redhat.com</a>> wrote:<br>
<br>
> Morten Brørup, Nov 15, 2024 at 14:52:<br>
> > Robin, you've totally won me over on this endian discussion. :-)<br>
> > Especially the IPv6 comparison make it clear why IPv4 should also be <br>
> > network byte order.<br>
> ><br>
> > API/ABI stability is a pain... we're stuck with host endian IPv4 <br>
> > addresses; e.g. for the RTE_IPV4() macro, which I now agree produces <br>
> > the wrong endian value (on little endian CPUs).  <br>
> <br>
> At least for 24.11 it is too late. But maybe we could make it right for <br>
> the next LTS?<br>
> <br>
> >> Vladimir, could we at least consider adding a real network order mode <br>
> >> for the rib and fib libraries? So that we can have consistent APIs <br>
> >> between IPv4 and IPv6?  <br>
> ><br>
> > And/or rename RTE_FIB_F_NETWORK_ORDER to <br>
> > RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar. This is important if real <br>
> > network order mode is added (now or later)!  <br>
> <br>
> Maybe we could revert that patch and defer a complete change of the <br>
> rib/fib APIs to only expose network order addresses? It would be an ABI <br>
> breakage but if properly announced in advance, it should be possible.<br>
> <br>
> Thinking about it some more. Having a flag for such a drastic change in <br>
> behaviour does not seem right.<br>
<br>
It was a mistake for DPDK to define its own data structures for IP addresses.<br>
Would have been much better to stick with the legacy what BSD, Linux (and Windows)<br>
uses in API. 'struct in_addr' and 'struct in6_addr'<br>
<br>
Reinvention did not help users.<br>
</blockquote></div><div><br clear="all"></div><br><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div>Vladimir<br></div></div>