On Mon, Jan 5, 2026 at 1:22 AM Ales Musil <[email protected]> wrote: > > > > On Mon, Dec 29, 2025 at 7:29 PM Numan Siddique <[email protected]> wrote: >> >> On Tue, Dec 16, 2025 at 9:31 PM Mark Michelson via dev >> <[email protected]> wrote: >> > >> > The lflow_table_add_lflow() function takes 13 arguments. Many helper >> > macros exist to facilitate filling in those 13 arguments without having >> > to spell them all out every time. Unfortunately, the amount of different >> > macros has gotten a bit out of hand. Some macros, like >> > ovn_lflow_add_with_hint__() have misleading names. Some macros, like >> > ovn_lflow_add_drop_with_lport_hint_and_desc() have horribly unwieldy >> > names. Trying to add a new macro to the list is a minefield since you >> > need to cherry-pick exactly which arguments the new macro should require >> > and which ones the macro should fill in automatically. >> > >> > This series seeks to simplify things by getting rid of all helper macros >> > except for ovn_lflow_add(). We accomplish this by using the VFUNC() >> > macro to create different versions of ovn_lflow_add() that take >> > different numbers of arguments. All invocations of ovn_lflow_add() >> > require a minimum of 7 arguments: >> > * lflow table >> > * ovn_datapath >> > * stage >> > * priority >> > * match >> > * actions >> > * lflow_ref >> > >> > Additional fields can be specified by adding an 8th, 9th, or 10th >> > argument using a series of helper macros to add specific field types. >> > The idea behind this is that any argument beyond the 7th can be any of >> > the available fields. This gives the flexibility to add lflows in any >> > number of combinations without having to define hyper-specific macros >> > for lflow addition. >> > >> > Differences between v1 and v2 of this series: >> > * In this version, the struct argument version of lflow_table_add_lflow >> > takes a pointer to struct lflow_table_add_args instead of a bare >> > struct. This should save some unnecessary copying of arguments. >> > * In patch 2, we define ovn_lflow_add_default_drop() in terms of >> > ovn_lflow_add(). This means we do not need to repeat struct >> > initialization, which means we do not need special START and END >> > macros for struct initialization. >> > >> > Mark Michelson (9): >> > lflow-mgr: Use struct argument for lflow addition. >> > lflows: Create new ovn_lflow_add_default_drop(). >> > lflows: Remove ovn_lflow_add_with_hint(). >> > lflows: Remove ovn_lflow_add_with_lport_and_hint(). >> > lflows: Remove ovn_lflow_metered() and ovn_lflow_add_with_hint__(). >> > lflows: Remove ovn_lflow_add_with_dp_group(). >> > lflows: Remove ovn_lflow_add_drop_with_desc(). >> > lflows: Remove ovn_lflow_add_drop_with_lport_hint_and_desc(). >> > lflows: Make non-struct version of lflow_table_add_lflow() private. >> > >> > northd/lflow-mgr.c | 47 +- >> > northd/lflow-mgr.h | 117 ++- >> > northd/northd.c | 1812 +++++++++++++++++++++++--------------------- >> > 3 files changed, 1045 insertions(+), 931 deletions(-) >> >> Thanks for the improvements. >> >> I had one comment in patch 1, to swap the functions >> lflow_table_add_lflow() and lflow_table_add_lflow__(). >> But after going through the entire series and the last patch, it made >> sense why you did that way. >> >> LGTM. >> >> For the entire series: >> Acked-by: Numan Siddique <[email protected]> >> >> Numan >> >> > >> > -- >> > 2.51.1 >> > >> > _______________________________________________ >> > dev mailing list >> > [email protected] >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > Hi Mark, > > I think you might have missed my feedback on the RFC so putting it here too:
Yes, apologies Ales, I completely missed your feedback on the RFC. My responses are inline. > > > 1) We should also get rid of ovn_lflow_add_default_drop() in favor > of doing "ovn_lflow_add(..., debug_drop_action(), ..., WITH_DESC(...))". > That was the plan anyway to actually have all drops with a comment, > this will also force any future contributor to use only a single macro. IMO, this is outside the scope of this particular change set. This change aims to keep all installed flows the same while simplifying the method used to install them. Coming up with descriptions for all drop flows will result in flows being changed by this changeset. I think this can be done as a follow-up patch. Also, I think that if you want to enforce that drop flows have descriptions, then the best way to do that is by enforcing it via the macro definition. It's a bit ironic to be suggesting a new macro in this changeset that tries to reduce the macros, but having an ovn_lflow_add_drop() macro that requires a description, then that better enforces the policy that drop flows must have descriptions. > > 2) After the use of "ovn_lflow_add()" a lot of calls now spans on multiple > lines we should re-align those if applicable. I noticed this while making the changes. My thought process was that it was easier to review and we would potentially have fewer backport conflicts by keeping lines unchanged when possible. However, if the feedback is that we should prioritize condensing the code instead of preserving its layout, then I can make those changes. > > 3) If we want to make this happen it should be in 26.03, this makes it > way easier for us to backport later on. Ack, I'll get a v3 posted when I get an opportunity. > > Thanks, > Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
