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). 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. > 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). > 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. >> >> 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
