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. 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. 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. Does that make sense? > > 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
