On 19 Jan 2026, at 11:28, Ilya Maximets wrote:
> 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. They were new code, but as suspected, all related to RCU-protected data, so waved them. We are good from a Coverity standpoint. >> 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
