On 27 Oct 2025, at 18:05, Eelco Chaudron wrote:
> On 27 Oct 2025, at 16:17, Eli Britstein wrote: > <SNIP> >>>>> Hi Eli, >>>>> >>>>> This is the patch that changes how the flow mark is handled, as >>>>> mentioned in the reply to the cover letter of the v3 series. >>>>> >>>>> Could you let me know if this patch aligns with what you had in mind? >>>>> If so, please confirm, and I’ll roll this patch in with the previous >>>>> one and send out a non-RFC version of the series. >>>> [Eli Britstein] >>>> Not my meaning. >>>> In this series, there is still the mapping in dpif-netdev.c, yes, for >>>> "mufid" and >>> not mark, but still. Why do we need this at this layer at all? >>>> My meaning was to provide the flow pointer to >>> dpif_offload_datapath_flow_put() (it is already provided in >>> .cb_data.callback_aux_flow). >>>> Dpif-offload-dpdk will hold the map, and perform all the >>> associate/disassociate/etc functions. >>>> dpif_offload_netdev_hw_miss_packet_postprocess() can return the flow hint >>> so the find function is also implemented in dpif-offload-dpdk layer (and >>> later >>> enhanced with skipping actions). >>>> WDYT? >>> >>> The reason I still keep this mapping in dpif-netdev is that it maintains a >>> one-to- >>> many relationship between the mufid and the corresponding dp_netdev_flow >>> objects. Flow offload tracking is therefore inherently done in dpif-netdev, >>> and it >>> feels odd to delegate this responsibility entirely to the individual offload >>> providers. >>> >>> Even if we were to shift the tracking into the offload provider, it would >>> still need >>> to track which PMDs have the mufid offloaded and which dp_netdev_flow >>> objects correspond to it. When removing a flow, it can only be deleted once >>> the >>> last PMD has removed its instance of the flow. Handling all of this within >>> the >>> offload provider layer feels misplaced. Moreover, managing the >>> dp_netdev_flow reference counting from that layer would introduce additional >>> complexity and risk around lifecycle management. >>> >>> Thoughts? >> [Eli Britstein] >> >> To comment about the 1:N mapping, it is another issue that I didn't want to >> open now, but as it's brought up, and may affect the arch, I'll elaborate: >> In case there are multiple (N) PMDs, it is not guaranteed to have N u-flows. >> There might be just one. Then, a packet hitting the HW flow might reach to >> another PMD, in which there is no u-flow (same mega-ufid). >> In case of just mark partial offload, it is harmless. This PMD won't find >> any u-flow associated, but the processing is done correctly as the packet is >> not modified by the HW so it is just processed without any offload, creating >> its own u-flow. >> With partial offload that allows HW modification (and processing after >> skipping some actions) this is wrong. The PMD cannot just process as if >> there wasn't any offload since the packet is already modified by the HW. >> Instead, there should be some awareness of it. >> In our downstream, we did it by searching the mark->flow of other PMDs and >> cloning the foreign PMD's u-flow to the current one. >> >> The concept of flow<->mark(/mufid) mapping is related to offload. The >> purpose of this RFC (/patch-set) is to have a clean architecture for >> offloads. Keeping dpif-netdev doing has a taste of missing that purpose. >> I understand your points, including the risk in it. Still, I think we should >> figure it out. >> I don't think it is impossible. What do you think about trying to make the >> API for it, and try to detect the potential bugs in code review and testing? >> >> For the 1:N issue above, I think the dpif-offload can provide the foreign >> u-flow, marking it as foreign but still have the clone logic in dpif-netdev. >> >> WDYT? > > Let me think about this a bit more, and see if I can cook up something that > does not put too much backpressure on the flow offload. I spent some time thinking through your feedback and reworking the design to better align with the direction you suggested. I’ve now abstracted the logic so that the dpif-offload providers are responsible for all the bookkeeping. This change does mean that the providers now need to track offloads on a per-PMD basis, particularly to handle flow references correctly. The current patch doesn’t yet address the 1:N mapping issue you mentioned earlier, since it doesn’t currently affect existing implementations. My goal at this stage is to get this patch series into a non-RFC state, so we can move it forward upstream. We can then work on feature gaps, such as the 1:N case, in parallel during the review. You can find the working branch here: https://github.com/chaudron/ovs/tree/dev/dpif_offload_provider_v4_post The relevant commit, https://github.com/chaudron/ovs/commit/a9180aca87015861cb2c50be22255d27fccb5b6b, implements the shift toward having providers manage references directly. Could you take a look and let me know if this approach aligns with your expectations? If so, I’ll prepare and send out a non-RFC version for proper review. >>>> Another note is that the modify flow bug re-appears in v4. Can you take another stab at this bug using the branch mentioned above? If it still fails, I’ll need specific steps to reproduce it, as your previous steps worked fine here. Cheers, Eelco <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
