On 1/19/26 11:20 AM, Eelco Chaudron wrote: > > > 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.
IIRC, coverity is not very smart when it comes to code movement. So, I would guess, most of these are just pre-existing reports moved to a different file. > 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
