On Tue, Jan 6, 2026 at 4:04 PM Mark Michelson <[email protected]> wrote:
> 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. > That's fair, I'm ok with deferring this to a follow up. > > > > > 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. > My vote goes for condensing the code while we are making the changes, it's not that big of a change. > > > > 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 > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
