On 7/13/26 9:39 PM, Aaron Conole wrote: > Ilya Maximets <[email protected]> writes: > >> On 7/13/26 5:35 PM, Aaron Conole wrote: >>> Ilya Maximets <[email protected]> writes: >>> >>>> On 7/1/26 8:17 PM, Timothy Redaelli wrote: >>>>> Split libopenvswitch into two libraries: >>>>> >>>>> - libopenvswitchutils: containers, utilities, I/O, threading, >>>>> logging, and OVSDB client modules. >>>>> - libopenvswitch: datapath, OpenFlow, netdev, dpif, flow, >>>>> conntrack, and related modules. Links libopenvswitchutils. >>>>> >>>>> Programs that do not need datapath functionality now link only >>>>> libopenvswitchutils, reducing their dependency footprint: >>>>> ovsdb-server, ovsdb-client, ovsdb-tool, ovs-appctl, vtep-ctl, >>>>> test-lib. >>>>> >>>>> When OVS is built with static DPDK (--with-dpdk=static), these >>>>> programs no longer pull in DPDK code at all. Stripped binary >>>>> sizes (x86_64): >>>>> >>>>> Binary Before After Saved >>>>> ovs-appctl 3,939 KB 346 KB -91% >>>>> ovsdb-server 4,255 KB 766 KB -82% >>>>> ovsdb-client 4,234 KB 750 KB -82% >>>>> ovsdb-tool 4,218 KB 729 KB -83% >>>>> vtep-ctl 4,273 KB 847 KB -80% >>>>> test-lib 3,935 KB 342 KB -91% >>>>> >>>>> Binaries that still need libopenvswitch (ovs-vsctl, ovs-dpctl, >>>>> ovs-ofctl, ovs-vswitchd) are unaffected by this change. Further >>>>> reducing their DPDK dependency requires decoupling dp-packet from >>>>> netdev-dpdk, which is separate follow-up work. >>>>> >>>>> To break a circular dependency between the two libraries, >>>>> ip_parse(), ipv6_parse(), and ipv6_string_mapped() are made >>>>> static inline in packets.h. These are trivial wrappers around >>>>> inet_pton()/inet_ntop() with no side effects or state. >>>> >>>> It is concerning that the new utils library is using packets.h >>>> on the source level, while this file is not listed as part of >>>> sources for it. While inlining solves the build problem, the >>>> logical issue of modules not properly isolated remains. >>>> >>>> Previous versions did an excessive splitting of the header >>>> unnecessarily disintegrating it entirely. But keeping it intact >>>> is also not a good approach, as it is wrong from the library >>>> perspective. Maybe we can split it in some better way? >>>> >>>> The main bits that are used by different utility and database >>>> related applications are address parsing / formatting. So, >>>> maybe we can split functions that just deal with eth and ip >>>> addresses (not headers, hust addresses) into a separate module >>>> called netaddr.[ch] and let that be in the utils, while the >>>> rest of the packets.[ch] would still deal with packet headers >>>> and be used by the main openvswitch library and applications >>>> that actually need to deal with packets. >>>> >>>> WDYT? Aaron, David, Eelco? >>> >>> As you note, keeping packets.h and moving things to static inline ends >>> up creating these backdoor dependencies. I originally removed >>> packets.[ch] entirely because a file called 'packets.h' and a file >>> called 'dp-packet.h' feels confusing for developers. The 'packets.h' >>> file is an overly generic name and felt like going forward it would >>> create more opportunity for backdoor dependencies. I dissolved it into >>> a combination of 'dp-packet.h' and 'net-proto.h' for the "on the wire" >>> network protocol related work. You can see in the initial proposal [0] >>> going from: >>> >>> a. Create 'net-proto.h' for network protocol related functions, and >>> move the 'packets.h' contents there. >>> b. Move out some additional details to CT and remove unused >>> function(s). >>> c. Move the rest into dp-packet.h (because the remaining stuff all >>> manipulated dp-packet objects based on protocol details) and >>> delete packets.h >>> >>> Calling it netaddr.[ch] is fine by me (we're just arguing about a bike >>> shed there) but it still needs to facilitate separating the linkage. I >>> think keeping packets.h around doesn't make too much sense, because it >>> is very generic space, pulls in lots of modules from all over and lends >>> itself to getting cluttered up with all sorts of vague utilities that >>> aren't related (eg why are the *_compose in packets.h and not >>> dp-packet.h, or even dp-protocols.h?). In my split, I moved ALL of the >>> network protocol details out. That was the important part of the work. >>> >>> I think it's visible that packets.h became a catchall over the years. >>> What I mean is, it pulled lots of seemingly unrelated stuff. For >>> instance, we have flow_*compose, and cfm_*compose in their own headers, >>> but for some reason, the lacp compose didn't end up in the lacp >>> header (completely inconsistent) even though those things got moved out. >>> Once you do the split like you're discussing, you'll see from the >>> original proposal that packets.[ch] becomes just dp-packet related >>> functions that insert/remove headers, and so packets.[ch] becomes *less* >>> descriptive. >>> >>> After splitting out address related stuff, what's left doesn't make as >>> much sense together (some leftover consts, structs like ip_header, >>> tcp_header, etc) sitting alongside the dp-packet helper functions >>> (push_eth, pop_eth, packet_set_*, etc) with nothing connecting them. >>> Moving all the *protocol* definitions (structs, address stuff, consts) >>> to another header+c file, and then folding the remaining dp-packet >>> related functionality into dp-packet.[ch] seemed to me the cleanest >>> option. I gave some pushback [1], but it looks like that didn't land >>> enough and you asked Timothy to change it [2]. >>> >>> Maybe it makes more sense now about the original split proposal? >> >> My concern was and remains mostly the unnecessary code movements and >> renaming. The original set was 6K lines of changes that was hard to >> follow and would definitely create a lot of trouble for backports. >> And I was also genuinely asking questions, not just demanding the >> changes. >> >> I also do not fully agree that the proposed split was clean or even >> correct. >> >> As it stands, we have two separate modules: >> >> 1. dp-packet: it's a vessel for carrying packets around, and tracking >> the processing metadata like memory backends, offsets and offloads. >> As well as handling collections of those vessels (buckets). >> >> 2. packets: it's a module that contains functions that manipulate >> packet internals and the metadata that is not directly part of the >> packet, but treated like packet fields, e.g., conntrack state, >> tunnel metadata. >> >> Of course, there is some overlap as you can't track offloads or offsets >> without peaking into the packet structure, but that's fairly minimal. >> >> Functions in packets module operate either on parts of the headers or >> on the packet as a whole in case we're dealing with an operation like >> construct or remove a header. Since dp-packet is a vessel of the >> packet data, it's natural that such functions in the packet module >> accept dp-packet as input. Other functions that do not need to touch >> things globally just accept the pointer to the data they need. But >> even though some functions receive dp-packet as input, their primary >> goal is still to change the packet data inside, not to manipulate >> processing metadata. When needed, appropriate dp-packet methods are >> called to do so. > > But under dp-packets we also do this. At least if I look at all of the > dp_packet_get_*_payload functions (like dp_packet_get_tcp_payload which > is very complex peeking logic, and I wouldn't call it minimal).
I think, these functions belong to packets.h. They use offsets provided by the dp-packet module, which is fine, but they aim to get inside the packet headers, so they should not be in dp-packet module. > > Also, dp_packet_calc_hash_ipv4 which really doesn't have anything to do > with dp-packet module (other than as a dependency). At least to me, it > doesn't read mostly making sense. This function as well should be in packets.h. The update_rss functions belong in dp-packet as they work with the processing metadata (RSS), but the dp_packet_calc_hash_ipv4() goes inside the packet headers and is clearly aware of what's in there, so it should be in packets module. dp-packet can call it. > >> So, using this understanding, I don't think most methods like pop_mpls >> belong in dp-packet module, as they do not map to the dp-packet module's >> overall goal of tracking processing metadata like memory and batching. > > Okay, I can agree if that's the goal, but looking at the examples above, > I'm not sure that we've achieved it. And I also think maybe we're > getting lost in some weeds - the primary goal of this work is to get a > common library split out and we can stage these kinds of changes to > start. It will at least require that we agree a new module should be > created to move this stuff out of packets.[ch] (and I think we do, just > making sure). I agree that more modules are needed here. The way the original code was written is that there was ofpbuf and the packets module did everything that is related to anything about packets, headers, etc. Then there was a need to decouple packets from their storage (because of DPDK, IIRC), so the dp-packet was split out of the packets module into a separate one. Though these two were always interconnected and will likely always be. And so, they will have to be part of a single library. What we can do is to continue splitting the packets module into smaller ones. Logical parts would be to separate header manipulations and packet metadata handling. Packet manipulations can be split into address management and the actual header changes. Packet metadata handling can be split into conntrack, tunnel and the rest of the metadata. In the end the packet header manipulations would remain in packets.[ch] and the other parts would be in their own smaller sub-modules. Packets and dp-packet will remain closely related and will keep using functions and structures from each other, so we will not be able to move them into different libraries. But all the utilities and tools that do not actually need to work with packets or packet headers will be able to consume those smaller modules that will not have dependencies on dp-packet and so can be moved into a separate library. And while, e.g. ovs-ofctl, doesn't technically need to link with DPDK and could use non-DPDK variant of dp-packet, I think, it is very dangerous to allow this kind of linking. And if we want to move in this direction, we will have to add a ton of build-time checks to make sure we never link together libraries that use different representation of dp-packet, as it did happen in the past with dynamic libraries in ubuntu, IIRC, and issues like that are very hard to debug. > >> There are some naming challenges in packets module, I agree. The >> 'flow_tnl' functions look out of place at a first glance, but they >> just work on pkt_metadata->tunnel structure that is one of the main >> parts of the packets module. And this is also why net-proto module >> doesn't make a lot of sense, as packet metadata (match-able fields) >> isn't any network protocol for the most part, but it is also not >> a flow information, as it is carried around along with the packet and >> doesn't belong to the flow structure and so shouldn't be in flow.c. >> >> So, there are definitely some naming challenges and there are a few >> functions that are misplaced, and a lot of stuff is coming from the >> era where dp-packet didn't exist and everything was an ofpbuf. But >> the separation and the way things are aggregated together today still >> makes sense for the most part. That's why I'm proposing to just split >> a small fraction of the packets module into a separate submodule instead >> of disintegrating packets.[ch] and scattering logically related bits >> into different place where, IMO, they do not really belong. > > Agreed. Maybe the biggest issue with the original approach is that it > tried to collapse things and thus made the resulting change difficult to > review for the purposes of figuring out where things should split. > >> Does that make sense? > > What makes sense to me is to at least implement a new module that moves > out the independent code to break the packets.[ch] dependency and enable > a libopenvswitchutils. I think we can do that in this series, and then > try to achieve more cleanup in a future series. If you want to call the > new module netaddrs.[ch], or net-proto.[ch], or anything else, it's fine > by me provided it makes some sense. > > Then we can start another series to go through the two modules > packets.[ch] and dp-packet.[ch] and try to achieve a real split that > gets as close to "the platonic ideal of modularity" that we could > achieve. And if we want to keep the name packets.[ch] fine, but we > should aim for the separation to be clear. Yes, it should be a gradual approach and creation of something like netaddr.ch could be a first step that should allow the largest part of the library separation. > >>> >>> 0: >>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=495033&state=%2A&archive=both >>> 1: https://patchwork.ozlabs.org/comment/3662240/ >>> 2: https://patchwork.ozlabs.org/comment/3711397/ > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
