On 16 Jan 2026, at 17:12, Eelco Chaudron wrote:
> On 16 Jan 2026, at 12:13, Ilya Maximets wrote: > >> On 1/15/26 10:01 PM, Eelco Chaudron wrote: >>> >>> >>> On 15 Jan 2026, at 18:44, Ilya Maximets wrote: >>>> On 1/15/26 10:19 AM, Eelco Chaudron wrote: >>> [...] >>> >>>> Thanks, Eelco for the update! I read through the set again and it >>>> reads much easier after the renames. There are still a few small >>>> style issues throughout the set, but they are all minor like an >>>> extra empty line at the EOF of dpif-offload.c in patch 7, a few >>>> more places with a missing double space or underscore or the thread >>>> safety annotations not on a separate line. There is also a missed >>>> rename of dpif_offload_flow_get_n_offloaded() into something like >>>> dpif_ofload_flow_count(), since the underlying callback was renamed. >>>> And the provider_collection_add() should return a positive errno >>>> after all, since it's passed directly into ovs_strerror(). >>>> >>>> As discussed off-list, instead of me writing all of these nits down >>>> in the emails and then you fixing them, it's simpler if I just >>>> handle the merge and fix the nits on commit. >>>> >>>> Also, found a couple more issues that we should address: >>>> >>>> 1. In the first patch, the dpif_offload_dump_next() function is >>>> describing a case that should be impossible, but still tries >>>> to handle it. We shouldn't do that. And instead we need to >>>> just take the collection reference in dpif_offload_dump_start() >>>> to make sure the collection doesn't go away during the dump. >>>> And then we can fully rely on the LIST_FOR_EACH_CONTINUE. >>>> >>>> 2. The per-port priority configuration should be per-interface >>>> instead as datapath doesn't deal with ports, it only works >>>> with interfaces. And some ports, like bonds can have multiple >>>> interfaces. It may not be a big deal as in a normal case >>>> I don't think we would need different offload for interfaces >>>> in the same port, but it's not really correct to configure >>>> ports, when it is applied on the interface. >>>> >>>> And there are two things that I flagged in v5, but we decided to >>>> handle separately: >>>> >>>> 3. Mass-rename of offload provider structures and functions to >>>> make them coherent and shorter than they are. Most of them >>>> are not exported and don't need extensive prefixes. And these >>>> prefixes need unification after moving stuff between netdev* >>>> and dpif* modules. >>>> >>>> 4. Move of the port manager instances from provider-specific >>>> structures to the common struct dpif_offload. This should >>>> eliminate some duplication and make the module boundaries >>>> more clear. This should also eliminate the need for the >>>> port dump API or at least significantly simplify it, e.g. >>>> by just iterating over port cmap with a cmap_cursor. >>>> >>>> >>>> So, the situation is the following: We have branching planned for >>>> tomorrow. Full CI run takes about 24 hours. So, we don't really >>>> have time for v7. We could postpone branching, but it also seems >>>> unreasonable as we only really need changes in about 4 patches >>>> out of 40, and none of the issues listed above are critical or >>>> breaking any functionality. They are mostly internal code movements. >>>> It would be much easier to get the set merged and then work on the >>>> 4 items above next week and backport to the new branch. >>>> >>>> As mentioned earlier, to save some time, I can handle the merging >>>> and fix all the small nits on commit instead of wasting a lot of >>>> time on writing them down on the mailing list and then Eelco >>>> fixing them. >>>> >>>> Eelco, Aaron, does that sound like a good plan? Or did I miss >>>> anything in my summary? >>> >>> Thanks, Ilya, for summarizing our offline discussion! This seems like >>> the best way forward, and I will start working on those patches once >>> I get back on Monday. >> >> Thanks, Eelco, Eli, and Aaron! As agreed, I fixed all the small nits >> that I saw throughout the set, added ACKs from Aaron, and applied the >> set to main. Also backported patch 34 down to 3.3. >> >> Let's try to get the patches for the remaining 4 items above soon after >> branching. > > Thanks for taking care of this Ilya!! I’ll start working on the followup > patches on Monday. In addition, Coverity marked 9 issues as resolved, but we introduced 6 new ones. I did notice similar ones in my pre-merge builds; they are all related to accessing RCU-protected structures without a lock, which is the purpose of RCUs :) I’ll take a closer look this week and mark them as false positives. If any of them need a patch, I’ll send it. Cheers, Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
