| Message ID | 20220328053943.1606715-1-hzhou@ovn.org |
|---|---|
| Headers | show |
| Series | Use ct_mark for masked access to make flows HW-offloading friendly. | expand |
On Mon, Mar 28, 2022 at 1:40 AM Han Zhou <hzhou@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. > > There are no other flows relying on masked ct_label match, but it's > worth noting that the LB hairpin related flows using ct_label.natted > which were hardcoded directly in ovn-controller are still kept to avoid > traffic breaking during upgrading. It relies on the > northd-internal-version to internally determine if it is currently > upgrading from a version that requires the ct_label flows being > kept, and automatically removes the flows when northd-internal-version > is up-to-date. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 Thanks for the patch series. For the entire series: Acked-by: Numan Siddique <numans@ovn.org> Numan > > v1 -> v2: Fixed two system test cases. > v2 -> v3: > - Addressed Numan's comment regarding hairpin flow upgrading problem by > keeping ct_label flows together with ct_mark flows for hairpin, and > provided an option to disable ct_label after upgrading. > - Moved a misplaced chunk from patch2 to patch5 to fix related system tests. > v3 -> v4: > - Avoid the extra option to disable ct_label after upgrading for hairpin > flows. Instead, reuse the north-internal-ver to determine automatically. > For this to work, a new patch is added to handle SB_Global changes in I-P > engine of ovn-controller. > > Han Zhou (6): > ovn-sb.xml: Fix ct_lb documentation. > actions: Add action ct_lb_mark. > actions: Add stack push and pop actions. > ovn-northd: Improve the doc and tests for ecmp-symmetric-reply. > ovn-controller: Handle SB_Global:options:northd_internal_version in > I-P engine. > Use ct_mark for masked access to make flows HW-offloading friendly. > > NEWS | 2 + > controller/lflow.c | 33 ++- > controller/lflow.h | 1 + > controller/ovn-controller.c | 79 ++++++ > include/ovn/actions.h | 11 +- > include/ovn/logical-fields.h | 3 + > lib/actions.c | 128 ++++++++- > lib/logical-fields.c | 17 +- > lib/ovn-util.c | 25 +- > lib/ovn-util.h | 4 + > northd/northd.c | 107 ++++--- > northd/ovn-northd.8.xml | 59 ++-- > ovn-sb.xml | 54 +++- > tests/ovn-controller.at | 46 +++ > tests/ovn-northd.at | 526 ++++++++++++++++++----------------- > tests/ovn.at | 208 +++++++------- > tests/system-ovn.at | 178 ++++++------ > utilities/ovn-trace.c | 72 ++++- > 18 files changed, 1017 insertions(+), 536 deletions(-) > > -- > 2.30.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Mar 29, 2022 at 2:20 PM Numan Siddique <numans@ovn.org> wrote: > > On Mon, Mar 28, 2022 at 1:40 AM Han Zhou <hzhou@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. > > > > There are no other flows relying on masked ct_label match, but it's > > worth noting that the LB hairpin related flows using ct_label.natted > > which were hardcoded directly in ovn-controller are still kept to avoid > > traffic breaking during upgrading. It relies on the > > northd-internal-version to internally determine if it is currently > > upgrading from a version that requires the ct_label flows being > > kept, and automatically removes the flows when northd-internal-version > > is up-to-date. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 > > > Thanks for the patch series. > > For the entire series: > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan > Thanks Numan! I applied the series to main. Han > > > > v1 -> v2: Fixed two system test cases. > > v2 -> v3: > > - Addressed Numan's comment regarding hairpin flow upgrading problem by > > keeping ct_label flows together with ct_mark flows for hairpin, and > > provided an option to disable ct_label after upgrading. > > - Moved a misplaced chunk from patch2 to patch5 to fix related system tests. > > v3 -> v4: > > - Avoid the extra option to disable ct_label after upgrading for hairpin > > flows. Instead, reuse the north-internal-ver to determine automatically. > > For this to work, a new patch is added to handle SB_Global changes in I-P > > engine of ovn-controller. > > > > Han Zhou (6): > > ovn-sb.xml: Fix ct_lb documentation. > > actions: Add action ct_lb_mark. > > actions: Add stack push and pop actions. > > ovn-northd: Improve the doc and tests for ecmp-symmetric-reply. > > ovn-controller: Handle SB_Global:options:northd_internal_version in > > I-P engine. > > Use ct_mark for masked access to make flows HW-offloading friendly. > > > > NEWS | 2 + > > controller/lflow.c | 33 ++- > > controller/lflow.h | 1 + > > controller/ovn-controller.c | 79 ++++++ > > include/ovn/actions.h | 11 +- > > include/ovn/logical-fields.h | 3 + > > lib/actions.c | 128 ++++++++- > > lib/logical-fields.c | 17 +- > > lib/ovn-util.c | 25 +- > > lib/ovn-util.h | 4 + > > northd/northd.c | 107 ++++--- > > northd/ovn-northd.8.xml | 59 ++-- > > ovn-sb.xml | 54 +++- > > tests/ovn-controller.at | 46 +++ > > tests/ovn-northd.at | 526 ++++++++++++++++++----------------- > > tests/ovn.at | 208 +++++++------- > > tests/system-ovn.at | 178 ++++++------ > > utilities/ovn-trace.c | 72 ++++- > > 18 files changed, 1017 insertions(+), 536 deletions(-) > > > > -- > > 2.30.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Tue, Mar 29, 2022 at 3:06 PM Han Zhou <hzhou@ovn.org> wrote: > > > > On Tue, Mar 29, 2022 at 2:20 PM Numan Siddique <numans@ovn.org> wrote: > > > > On Mon, Mar 28, 2022 at 1:40 AM Han Zhou <hzhou@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. > > > > > > There are no other flows relying on masked ct_label match, but it's > > > worth noting that the LB hairpin related flows using ct_label.natted > > > which were hardcoded directly in ovn-controller are still kept to avoid > > > traffic breaking during upgrading. It relies on the > > > northd-internal-version to internally determine if it is currently > > > upgrading from a version that requires the ct_label flows being > > > kept, and automatically removes the flows when northd-internal-version > > > is up-to-date. > > > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 > > > > > > Thanks for the patch series. > > > > For the entire series: > > > > Acked-by: Numan Siddique <numans@ovn.org> > > > > Numan > > > Thanks Numan! I applied the series to main. > As discussed in the last OVN meetings, I backported this series to branch-22.03. It was also requested to backport to 21.12, but @Dumitru Ceara <dceara@redhat.com> volunteered to work on that branch. Please note that there will be conflict primarily because "log-related" was not supported in 21.12. Thanks, Han > Han > > > > > > > v1 -> v2: Fixed two system test cases. > > > v2 -> v3: > > > - Addressed Numan's comment regarding hairpin flow upgrading problem by > > > keeping ct_label flows together with ct_mark flows for hairpin, and > > > provided an option to disable ct_label after upgrading. > > > - Moved a misplaced chunk from patch2 to patch5 to fix related system tests. > > > v3 -> v4: > > > - Avoid the extra option to disable ct_label after upgrading for hairpin > > > flows. Instead, reuse the north-internal-ver to determine automatically. > > > For this to work, a new patch is added to handle SB_Global changes in I-P > > > engine of ovn-controller. > > > > > > Han Zhou (6): > > > ovn-sb.xml: Fix ct_lb documentation. > > > actions: Add action ct_lb_mark. > > > actions: Add stack push and pop actions. > > > ovn-northd: Improve the doc and tests for ecmp-symmetric-reply. > > > ovn-controller: Handle SB_Global:options:northd_internal_version in > > > I-P engine. > > > Use ct_mark for masked access to make flows HW-offloading friendly. > > > > > > NEWS | 2 + > > > controller/lflow.c | 33 ++- > > > controller/lflow.h | 1 + > > > controller/ovn-controller.c | 79 ++++++ > > > include/ovn/actions.h | 11 +- > > > include/ovn/logical-fields.h | 3 + > > > lib/actions.c | 128 ++++++++- > > > lib/logical-fields.c | 17 +- > > > lib/ovn-util.c | 25 +- > > > lib/ovn-util.h | 4 + > > > northd/northd.c | 107 ++++--- > > > northd/ovn-northd.8.xml | 59 ++-- > > > ovn-sb.xml | 54 +++- > > > tests/ovn-controller.at | 46 +++ > > > tests/ovn-northd.at | 526 ++++++++++++++++++----------------- > > > tests/ovn.at | 208 +++++++------- > > > tests/system-ovn.at | 178 ++++++------ > > > utilities/ovn-trace.c | 72 ++++- > > > 18 files changed, 1017 insertions(+), 536 deletions(-) > > > > > > -- > > > 2.30.2 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 5/23/22 20:07, Han Zhou wrote: > On Tue, Mar 29, 2022 at 3:06 PM Han Zhou <hzhou@ovn.org> wrote: >> >> >> >> On Tue, Mar 29, 2022 at 2:20 PM Numan Siddique <numans@ovn.org> wrote: >>> >>> On Mon, Mar 28, 2022 at 1:40 AM Han Zhou <hzhou@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. >>>> >>>> There are no other flows relying on masked ct_label match, but it's >>>> worth noting that the LB hairpin related flows using ct_label.natted >>>> which were hardcoded directly in ovn-controller are still kept to > avoid >>>> traffic breaking during upgrading. It relies on the >>>> northd-internal-version to internally determine if it is currently >>>> upgrading from a version that requires the ct_label flows being >>>> kept, and automatically removes the flows when northd-internal-version >>>> is up-to-date. >>>> >>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 >>> >>> >>> Thanks for the patch series. >>> >>> For the entire series: >>> >>> Acked-by: Numan Siddique <numans@ovn.org> >>> >>> Numan >>> >> Thanks Numan! I applied the series to main. >> > > As discussed in the last OVN meetings, I backported this series to > branch-22.03. > It was also requested to backport to 21.12, but @Dumitru Ceara > <dceara@redhat.com> volunteered to work on that branch. Please note that > there will be conflict primarily because "log-related" was not supported in > 21.12. I also just posted the 21.12 backport: https://patchwork.ozlabs.org/project/ovn/list/?series=301671&state=* https://mail.openvswitch.org/pipermail/ovs-dev/2022-May/394245.html Thanks, Dumitru > > Thanks, > Han > >> Han >> >>>> >>>> v1 -> v2: Fixed two system test cases. >>>> v2 -> v3: >>>> - Addressed Numan's comment regarding hairpin flow upgrading > problem by >>>> keeping ct_label flows together with ct_mark flows for hairpin, > and >>>> provided an option to disable ct_label after upgrading. >>>> - Moved a misplaced chunk from patch2 to patch5 to fix related > system tests. >>>> v3 -> v4: >>>> - Avoid the extra option to disable ct_label after upgrading for > hairpin >>>> flows. Instead, reuse the north-internal-ver to determine > automatically. >>>> For this to work, a new patch is added to handle SB_Global > changes in I-P >>>> engine of ovn-controller. >>>> >>>> Han Zhou (6): >>>> ovn-sb.xml: Fix ct_lb documentation. >>>> actions: Add action ct_lb_mark. >>>> actions: Add stack push and pop actions. >>>> ovn-northd: Improve the doc and tests for ecmp-symmetric-reply. >>>> ovn-controller: Handle SB_Global:options:northd_internal_version in >>>> I-P engine. >>>> Use ct_mark for masked access to make flows HW-offloading friendly. >>>> >>>> NEWS | 2 + >>>> controller/lflow.c | 33 ++- >>>> controller/lflow.h | 1 + >>>> controller/ovn-controller.c | 79 ++++++ >>>> include/ovn/actions.h | 11 +- >>>> include/ovn/logical-fields.h | 3 + >>>> lib/actions.c | 128 ++++++++- >>>> lib/logical-fields.c | 17 +- >>>> lib/ovn-util.c | 25 +- >>>> lib/ovn-util.h | 4 + >>>> northd/northd.c | 107 ++++--- >>>> northd/ovn-northd.8.xml | 59 ++-- >>>> ovn-sb.xml | 54 +++- >>>> tests/ovn-controller.at | 46 +++ >>>> tests/ovn-northd.at | 526 > ++++++++++++++++++----------------- >>>> tests/ovn.at | 208 +++++++------- >>>> tests/system-ovn.at | 178 ++++++------ >>>> utilities/ovn-trace.c | 72 ++++- >>>> 18 files changed, 1017 insertions(+), 536 deletions(-) >>>> >>>> -- >>>> 2.30.2 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >
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. There are no other flows relying on masked ct_label match, but it's worth noting that the LB hairpin related flows using ct_label.natted which were hardcoded directly in ovn-controller are still kept to avoid traffic breaking during upgrading. It relies on the northd-internal-version to internally determine if it is currently upgrading from a version that requires the ct_label flows being kept, and automatically removes the flows when northd-internal-version is up-to-date. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1957786 v1 -> v2: Fixed two system test cases. v2 -> v3: - Addressed Numan's comment regarding hairpin flow upgrading problem by keeping ct_label flows together with ct_mark flows for hairpin, and provided an option to disable ct_label after upgrading. - Moved a misplaced chunk from patch2 to patch5 to fix related system tests. v3 -> v4: - Avoid the extra option to disable ct_label after upgrading for hairpin flows. Instead, reuse the north-internal-ver to determine automatically. For this to work, a new patch is added to handle SB_Global changes in I-P engine of ovn-controller. Han Zhou (6): ovn-sb.xml: Fix ct_lb documentation. actions: Add action ct_lb_mark. actions: Add stack push and pop actions. ovn-northd: Improve the doc and tests for ecmp-symmetric-reply. ovn-controller: Handle SB_Global:options:northd_internal_version in I-P engine. Use ct_mark for masked access to make flows HW-offloading friendly. NEWS | 2 + controller/lflow.c | 33 ++- controller/lflow.h | 1 + controller/ovn-controller.c | 79 ++++++ include/ovn/actions.h | 11 +- include/ovn/logical-fields.h | 3 + lib/actions.c | 128 ++++++++- lib/logical-fields.c | 17 +- lib/ovn-util.c | 25 +- lib/ovn-util.h | 4 + northd/northd.c | 107 ++++--- northd/ovn-northd.8.xml | 59 ++-- ovn-sb.xml | 54 +++- tests/ovn-controller.at | 46 +++ tests/ovn-northd.at | 526 ++++++++++++++++++----------------- tests/ovn.at | 208 +++++++------- tests/system-ovn.at | 178 ++++++------ utilities/ovn-trace.c | 72 ++++- 18 files changed, 1017 insertions(+), 536 deletions(-)