On Mon, Mar 21, 2022 at 9:49 AM Numan Siddique <num...@ovn.org> wrote: > > On Sun, Mar 13, 2022 at 7:58 PM Han Zhou <hz...@ovn.org> wrote: > > > > Some NICs support HW offloading for datapath flows, but masked access to > > the 128-bit ct_label field may prevent a flow being offloaded due to HW > > limitations. OVN's use of ct_label currently includes: > > - ct_label.blocked (1 bit) > > - ct_label.natted (1 bit) > > - ct_label.ecmp_reply_port (16 bits) > > - ct_label.ecmp_reply_eth (48 bits) > > - ct_label.label (32 bits) > > > > This patch moves the bits blocked, natted and ecmp_reply_port to use > > ct_mark (18 bits in total among the 32-bit ct_mark), and keep the rest > > of the fields in ct_label: > > - ct_mark.blocked (1 bit) > > - ct_mark.natted (1 bit) > > - ct_mark.ecmp_reply_port (16 bits) > > - ct_label.ecmp_reply_eth (48 bits) > > - ct_label.label (32 bits) > > > > This would allow HW offloading to work for most of the cases. > > > > For ct_label.ecmp_reply_eth, the flow matching it still uses masked > > access, but it doesn't matter because the flow is for new connections > > and requires ct_commit in its actions, so it wouldn't be offloaded > > anyway for those NICs. There is a flow for established connections that > > would access the masked field in the actions, while in this patch it > > avoids masked access by using a register xxreg1 to temporarily read the > > whole ct_label, and then use masked access to xxreg1 to read the actual > > value. > > > > The only exception is for ct_label.label, there is a flow that matches > > the masked field for ACL logging of reply direction. This patch cannot > > avoid the masked access to ct_label in this case. This flow is enabled > > only for the feature "log-related". So offloading may still not work for > > some NICs when an ACL is configured with a label and with "log-related" > > enabled. > > > > The change is backward compatible as long as the ovn-controller (on > > worker nodes) are upgraded before the ovn-northd (on central nodes). > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 > > Signed-off-by: Han Zhou <hz...@ovn.org> > > Hi Han, > > Overall the patch LGTM. There is one problem though. > > Please see below. > > Numan > > > > --- > > controller/lflow.c | 6 +- > > include/ovn/logical-fields.h | 3 + > > lib/logical-fields.c | 17 +- > > northd/northd.c | 107 ++++--- > > northd/ovn-northd.8.xml | 48 ++-- > > tests/ovn-northd.at | 524 +++++++++++++++++------------------ > > tests/ovn.at | 172 ++++++------ > > tests/system-ovn.at | 170 ++++++------ > > 8 files changed, 549 insertions(+), 498 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index e169edef1..c09b4e07e 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -1985,10 +1985,8 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb, > > * - the destination protocol and port must be of a valid backend that > > * has the same IP as ip.dst. > > */ > > - ovs_u128 lb_ct_label = { > > - .u64.lo = OVN_CT_NATTED, > > - }; > > - match_set_ct_label_masked(&hairpin_match, lb_ct_label, lb_ct_label); > > + uint32_t lb_ct_mark = OVN_CT_NATTED; > > + match_set_ct_mark_masked(&hairpin_match, lb_ct_mark, lb_ct_mark); > > > > I think the above code change here would break the hairpin use case > until ovn-northd is upgraded to the version which would have this > patch. > > ovn-controller is adding an openflow to match on ct_mark.natted > whereas the old logical flow would set ct_label.natted. > > Thanks > Numan > Thanks Numan for spotting this. Yes, it is indeed a problem for upgrading. I sent v3 that adds both ct_label and ct_mark flows for this particular case, so that upgrading is not broken, and adds a new option to disable the ct_label flow whenever users are happy to do so after upgrading. Please take a look: https://patchwork.ozlabs.org/project/ovn/list/?series=291560
In addition, as a thought, we'd better be careful in the future when adding flows in ovn-controller directly, and avoid possible compatibility issues with northd. Thanks, Han _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
