Re: [ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
On Fri, Mar 26, 2021 at 9:16 AM Ben Pfaff wrote:
>
> On Mon, Mar 22, 2021 at 04:29:11PM +0530, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > Presently we add 65535 priority lflows in the stages -
> > 'ls_in_acl' and 'ls_out_acl' to drop packets which
> > match on 'ct.inv'.
> >
> > This patch provides an option to not use this field in the
> > logical flow matches for the following
> > reasons:
> >
> > • Some of the smart NICs which support offloading datapath
> >flows may not support this field. In such cases, almost
> >all the datapath flows cannot be offloaded if ACLs/load balancers
> >are configured on the logical switch datapath.
> >
> > • A recent commit in kernel ovs datapath sets the committed
> >connection tracking entry to be liberal for out-of-window
> >tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
> >packets will not be marked as invalid.
> >
> > There are still certain scenarios where a packet can be marked
> > invalid. Like
> > • If the tcp flags are not correct.
> >
> > • If there are checksum errors.
> >
> > In such cases, the packet will be delivered to the destination
> > port. So CMS should evaluate their usecases before enabling
> > this option.
> >
> > Signed-off-by: Numan Siddique
>
> Thanks for writing dldog code! I have a few comments.
>
> This looks correct:
>
> function can_use_ct_inv_match(options: Map): bool {
> map_get_bool_def(options, "use_ct_inv_match", true)
> }
>
> relation UseCtInvMatch(use_ct_inv_match: bool)
> UseCtInvMatch(can_use_ct_inv_match(options)) :-
> nb::NB_Global(._uuid = uuid, .options = options).
>
> I would make a few stylistic changes if I were writing it. I would drop
> the _uuid match because it is not used for anything. I would use a
> relation that just contains a bool, instead of a struct that contains a
> bool, because then there are fewer names to invent. I would usually
> drop the function, since it is only used in one place and it is a
> one-liner. So I'd end up with this:
>
> relation UseCtInvMatch[bool]
> UseCtInvMatch[use_ct_inv_match] :-
> nb::NB_Global(.options = options),
> var use_ct_inv_match = map_get_bool_def(options, "use_ct_inv_match",
> true).
>
> Or possibly I'd keep the map_get_bool_def(...) expression inside the
> brackets, I haven't decided what's really the best way:
>
> relation UseCtInvMatch[bool]
> UseCtInvMatch[map_get_bool_def(options, "use_ct_inv_match", true)] :-
> nb::NB_Global(.options = options).
>
> I'd also add a rule to handle the case where NB_Global doesn't exist.
> This is really out of paranoia. I don't know whether this can really
> happen in practice, but it costs only a few lines and avoids a problem
> if it does ever somehow happen:
>
> UseCtInvMatch[true] :-
> Unit(),
> not nb in nb::NB_Global().
>
> I noticed is that this global feature flag is getting copied inside
> every Switch. That wastes a little memory and I do not know of a
> benefit. So I'd drop it from Switch and join with UseCtInvMatch in the
> one place where it's needed.
>
> Finally, there's are lots of redundant strings in ovn_northd.dl. We can
> make that better with a little parameterization, something like this:
> @@ -2290,10 +2290,15 @@ for (Reject(lsuuid, pipeline, stage, acl,
> fair_meter, extra_match_, extra_action
> .actions = actions,
> .external_ids = stage_hint(acl._uuid))
> }
>
> /* build_acls */
> +for (UseCtInvMatch[use_ct_inv_match]) {
> +(var ct_inv_or, var and_not_ct_inv) = match (use_ct_inv_match) {
> +true -> ("ct.inv || ", "&& !ct.inv "),
> +false -> ("", ""),
> +} in
> for (sw in (.ls = ls))
> var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
> {
> /* Ingress and Egress ACL Table (Priority 0): Packets are
> allowed by
> * default. A related rule at priority 1 is added below if there
> @@ -2354,17 +2359,17 @@ var has_stateful = sw.has_stateful_acl or
> sw.has_lb_vip in
> *
> * This is enforced at a higher priority than ACLs can be
> defined. */
> Flow(.logical_datapath = ls._uuid,
> .stage= switch_stage(IN, ACL),
> .priority = 65535,
> - .__match = "ct.inv || (ct.est && ct.rpl &&
> ct_label.blocked == 1)",
> + .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
> ct_label.blocked == 1)",
> .actions = "drop;",
> .external_ids = map_empty());
> Flow(.logical_datapath = ls._uuid,
> .stage= switch_stage(OUT, ACL),
> .priority = 65535,
> - .__match = "ct.inv || (ct.est && ct.rpl &&
>
Re: [ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
On Mon, Mar 22, 2021 at 04:29:11PM +0530, num...@ovn.org wrote:
> From: Numan Siddique
>
> Presently we add 65535 priority lflows in the stages -
> 'ls_in_acl' and 'ls_out_acl' to drop packets which
> match on 'ct.inv'.
>
> This patch provides an option to not use this field in the
> logical flow matches for the following
> reasons:
>
> • Some of the smart NICs which support offloading datapath
>flows may not support this field. In such cases, almost
>all the datapath flows cannot be offloaded if ACLs/load balancers
>are configured on the logical switch datapath.
>
> • A recent commit in kernel ovs datapath sets the committed
>connection tracking entry to be liberal for out-of-window
>tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
>packets will not be marked as invalid.
>
> There are still certain scenarios where a packet can be marked
> invalid. Like
> • If the tcp flags are not correct.
>
> • If there are checksum errors.
>
> In such cases, the packet will be delivered to the destination
> port. So CMS should evaluate their usecases before enabling
> this option.
>
> Signed-off-by: Numan Siddique
Thanks for writing dldog code! I have a few comments.
This looks correct:
function can_use_ct_inv_match(options: Map): bool {
map_get_bool_def(options, "use_ct_inv_match", true)
}
relation UseCtInvMatch(use_ct_inv_match: bool)
UseCtInvMatch(can_use_ct_inv_match(options)) :-
nb::NB_Global(._uuid = uuid, .options = options).
I would make a few stylistic changes if I were writing it. I would drop
the _uuid match because it is not used for anything. I would use a
relation that just contains a bool, instead of a struct that contains a
bool, because then there are fewer names to invent. I would usually
drop the function, since it is only used in one place and it is a
one-liner. So I'd end up with this:
relation UseCtInvMatch[bool]
UseCtInvMatch[use_ct_inv_match] :-
nb::NB_Global(.options = options),
var use_ct_inv_match = map_get_bool_def(options, "use_ct_inv_match",
true).
Or possibly I'd keep the map_get_bool_def(...) expression inside the
brackets, I haven't decided what's really the best way:
relation UseCtInvMatch[bool]
UseCtInvMatch[map_get_bool_def(options, "use_ct_inv_match", true)] :-
nb::NB_Global(.options = options).
I'd also add a rule to handle the case where NB_Global doesn't exist.
This is really out of paranoia. I don't know whether this can really
happen in practice, but it costs only a few lines and avoids a problem
if it does ever somehow happen:
UseCtInvMatch[true] :-
Unit(),
not nb in nb::NB_Global().
I noticed is that this global feature flag is getting copied inside
every Switch. That wastes a little memory and I do not know of a
benefit. So I'd drop it from Switch and join with UseCtInvMatch in the
one place where it's needed.
Finally, there's are lots of redundant strings in ovn_northd.dl. We can
make that better with a little parameterization, something like this:
@@ -2290,10 +2290,15 @@ for (Reject(lsuuid, pipeline, stage, acl,
fair_meter, extra_match_, extra_action
.actions = actions,
.external_ids = stage_hint(acl._uuid))
}
/* build_acls */
+for (UseCtInvMatch[use_ct_inv_match]) {
+(var ct_inv_or, var and_not_ct_inv) = match (use_ct_inv_match) {
+true -> ("ct.inv || ", "&& !ct.inv "),
+false -> ("", ""),
+} in
for (sw in (.ls = ls))
var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
{
/* Ingress and Egress ACL Table (Priority 0): Packets are allowed
by
* default. A related rule at priority 1 is added below if there
@@ -2354,17 +2359,17 @@ var has_stateful = sw.has_stateful_acl or
sw.has_lb_vip in
*
* This is enforced at a higher priority than ACLs can be
defined. */
Flow(.logical_datapath = ls._uuid,
.stage= switch_stage(IN, ACL),
.priority = 65535,
- .__match = "ct.inv || (ct.est && ct.rpl &&
ct_label.blocked == 1)",
+ .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
ct_label.blocked == 1)",
.actions = "drop;",
.external_ids = map_empty());
Flow(.logical_datapath = ls._uuid,
.stage= switch_stage(OUT, ACL),
.priority = 65535,
- .__match = "ct.inv || (ct.est && ct.rpl &&
ct_label.blocked == 1)",
+ .__match = ct_inv_or ++ "(ct.est && ct.rpl &&
ct_label.blocked == 1)",
.actions = "drop;",
.external_ids = map_empty());
/* Ingress and
Re: [ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
Hi Ben,
Need your eye on this patch for the ddlog stuff. I was wondering
if there is a better way to do than what I've done.
Not urgent. Please take your time.
Thanks
Numan
On Mon, Mar 22, 2021 at 4:29 PM wrote:
>
> From: Numan Siddique
>
> Presently we add 65535 priority lflows in the stages -
> 'ls_in_acl' and 'ls_out_acl' to drop packets which
> match on 'ct.inv'.
>
> This patch provides an option to not use this field in the
> logical flow matches for the following
> reasons:
>
> • Some of the smart NICs which support offloading datapath
>flows may not support this field. In such cases, almost
>all the datapath flows cannot be offloaded if ACLs/load balancers
>are configured on the logical switch datapath.
>
> • A recent commit in kernel ovs datapath sets the committed
>connection tracking entry to be liberal for out-of-window
>tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
>packets will not be marked as invalid.
>
> There are still certain scenarios where a packet can be marked
> invalid. Like
> • If the tcp flags are not correct.
>
> • If there are checksum errors.
>
> In such cases, the packet will be delivered to the destination
> port. So CMS should evaluate their usecases before enabling
> this option.
>
> Signed-off-by: Numan Siddique
> ---
> NEWS | 5 +++
> northd/lswitch.dl| 13 +++-
> northd/ovn-northd.c | 41 ++-
> northd/ovn_northd.dl | 51 +++--
> ovn-nb.xml | 11 +++
> tests/ovn-northd.at | 77
> 6 files changed, 171 insertions(+), 27 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 530c5d42fe..3181649ba8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,11 @@ Post-v21.03.0
> (This may take testing and tuning to be effective.) This version of OVN
> requires DDLog 0.36.
>- Introduce ovn-controller incremetal processing engine statistics
> + - Add a new NB Global option - us_ct_inv_match with the default value of
> true.
> +If this is set to false, then the logical field - "ct.inv" will not be
> +used in the logical flow matches. CMS can consider setting this to
> false,
> +if they want to use smart NICs which don't support offloading datapath
> flows
> +with this field used.
>
> OVN v21.03.0 - 12 Mar 2021
> -
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index 4bf8a5b907..1daf249382 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -197,6 +197,7 @@ relation (
> ipv6_prefix: Option,
> mcast_cfg: Ref,
> is_vlan_transparent: bool,
> +use_ct_inv_match: bool,
>
> /* Does this switch have at least one port with type != "router"? */
> has_non_router_port: bool
> @@ -223,7 +224,8 @@ function ipv6_parse_prefix(s: string): Option {
> .ipv6_prefix = ipv6_prefix,
> .mcast_cfg = mcast_cfg,
> .has_non_router_port = has_non_router_port,
> -.is_vlan_transparent = is_vlan_transparent) :-
> +.is_vlan_transparent = is_vlan_transparent,
> +.use_ct_inv_match = use_ct_inv_match) :-
> nb::Logical_Switch[ls],
> LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
> LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
> @@ -232,6 +234,7 @@ function ipv6_parse_prefix(s: string): Option {
> LogicalSwitchLocalnetPorts(ls._uuid, localnet_ports),
> LogicalSwitchHasNonRouterPort(ls._uuid, has_non_router_port),
> mcast_cfg in (.datapath = ls._uuid),
> +UseCtInvMatch(use_ct_inv_match),
> var subnet =
> match (ls.other_config.get("subnet")) {
> None -> None,
> @@ -744,3 +747,11 @@ function put_svc_monitor_mac(options: Map,
> relation SvcMonitorMac(mac: eth_addr)
> SvcMonitorMac(get_svc_monitor_mac(options, uuid)) :-
> nb::NB_Global(._uuid = uuid, .options = options).
> +
> +function can_use_ct_inv_match(options: Map): bool {
> +map_get_bool_def(options, "use_ct_inv_match", true)
> +}
> +
> +relation UseCtInvMatch(use_ct_inv_match: bool)
> +UseCtInvMatch(can_use_ct_inv_match(options)) :-
> +nb::NB_Global(._uuid = uuid, .options = options).
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3221fb9ff7..6baed2a418 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4066,6 +4066,10 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
> * logical datapath only by creating a datapath group. */
> static bool use_logical_dp_groups = false;
>
> +/* If this option is 'true' northd will make use of ct.inv match fields.
> + * Otherwise, it will avoid using it. The default is true. */
> +static bool use_ct_inv_match = true;
> +
> /* Adds a row with the specified contents to the Logical_Flow table. */
> static void
> ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
> @@ -5681,12 +5685,13 @@ build_acls(struct ovn_datapath *od, struct
[ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.
From: Numan Siddique
Presently we add 65535 priority lflows in the stages -
'ls_in_acl' and 'ls_out_acl' to drop packets which
match on 'ct.inv'.
This patch provides an option to not use this field in the
logical flow matches for the following
reasons:
• Some of the smart NICs which support offloading datapath
flows may not support this field. In such cases, almost
all the datapath flows cannot be offloaded if ACLs/load balancers
are configured on the logical switch datapath.
• A recent commit in kernel ovs datapath sets the committed
connection tracking entry to be liberal for out-of-window
tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP
packets will not be marked as invalid.
There are still certain scenarios where a packet can be marked
invalid. Like
• If the tcp flags are not correct.
• If there are checksum errors.
In such cases, the packet will be delivered to the destination
port. So CMS should evaluate their usecases before enabling
this option.
Signed-off-by: Numan Siddique
---
NEWS | 5 +++
northd/lswitch.dl| 13 +++-
northd/ovn-northd.c | 41 ++-
northd/ovn_northd.dl | 51 +++--
ovn-nb.xml | 11 +++
tests/ovn-northd.at | 77
6 files changed, 171 insertions(+), 27 deletions(-)
diff --git a/NEWS b/NEWS
index 530c5d42fe..3181649ba8 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,11 @@ Post-v21.03.0
(This may take testing and tuning to be effective.) This version of OVN
requires DDLog 0.36.
- Introduce ovn-controller incremetal processing engine statistics
+ - Add a new NB Global option - us_ct_inv_match with the default value of
true.
+If this is set to false, then the logical field - "ct.inv" will not be
+used in the logical flow matches. CMS can consider setting this to false,
+if they want to use smart NICs which don't support offloading datapath
flows
+with this field used.
OVN v21.03.0 - 12 Mar 2021
-
diff --git a/northd/lswitch.dl b/northd/lswitch.dl
index 4bf8a5b907..1daf249382 100644
--- a/northd/lswitch.dl
+++ b/northd/lswitch.dl
@@ -197,6 +197,7 @@ relation (
ipv6_prefix: Option,
mcast_cfg: Ref,
is_vlan_transparent: bool,
+use_ct_inv_match: bool,
/* Does this switch have at least one port with type != "router"? */
has_non_router_port: bool
@@ -223,7 +224,8 @@ function ipv6_parse_prefix(s: string): Option {
.ipv6_prefix = ipv6_prefix,
.mcast_cfg = mcast_cfg,
.has_non_router_port = has_non_router_port,
-.is_vlan_transparent = is_vlan_transparent) :-
+.is_vlan_transparent = is_vlan_transparent,
+.use_ct_inv_match = use_ct_inv_match) :-
nb::Logical_Switch[ls],
LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
@@ -232,6 +234,7 @@ function ipv6_parse_prefix(s: string): Option {
LogicalSwitchLocalnetPorts(ls._uuid, localnet_ports),
LogicalSwitchHasNonRouterPort(ls._uuid, has_non_router_port),
mcast_cfg in (.datapath = ls._uuid),
+UseCtInvMatch(use_ct_inv_match),
var subnet =
match (ls.other_config.get("subnet")) {
None -> None,
@@ -744,3 +747,11 @@ function put_svc_monitor_mac(options: Map,
relation SvcMonitorMac(mac: eth_addr)
SvcMonitorMac(get_svc_monitor_mac(options, uuid)) :-
nb::NB_Global(._uuid = uuid, .options = options).
+
+function can_use_ct_inv_match(options: Map): bool {
+map_get_bool_def(options, "use_ct_inv_match", true)
+}
+
+relation UseCtInvMatch(use_ct_inv_match: bool)
+UseCtInvMatch(can_use_ct_inv_match(options)) :-
+nb::NB_Global(._uuid = uuid, .options = options).
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3221fb9ff7..6baed2a418 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4066,6 +4066,10 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
ovn_datapath *od,
* logical datapath only by creating a datapath group. */
static bool use_logical_dp_groups = false;
+/* If this option is 'true' northd will make use of ct.inv match fields.
+ * Otherwise, it will avoid using it. The default is true. */
+static bool use_ct_inv_match = true;
+
/* Adds a row with the specified contents to the Logical_Flow table. */
static void
ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
@@ -5681,12 +5685,13 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
* for deletion (bit 0 of ct_label is set).
*
* This is enforced at a higher priority than ACLs can be defined. */
+char *match = xasprintf("%s(ct.est && ct.rpl && ct_label.blocked ==
1)",
+use_ct_inv_match ? "ct.inv || " : "");
ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
- "ct.inv || (ct.est && ct.rpl &&
