[ovs-dev] [PATCH 2/2] openflow: Allow CT flush to match on mark and labels.
Extend the current NX_CT_FLUSH with four additional fields,
that allow to match on CT entry "mark" or "labels". This
is encoded as separate TLV values which is backward compatible.
Versions that do not support them will simply ignore it.
Extend also the ovs-dpctl and ovs-ofctl command line tools with
option to specify those two matching parameters for the "ct-flush"
command.
Reported-at: https://issues.redhat.com/browse/FDP-55
Signed-off-by: Ales Musil
---
include/openflow/nicira-ext.h | 4 +
include/openvswitch/ofp-ct.h | 14 ++-
lib/ct-dpif.c | 14 ++-
lib/dpctl.c | 46 ++---
lib/ofp-ct.c | 181 +-
lib/ofp-print.c | 2 +-
tests/ofp-print.at| 56 +++
tests/ovs-ofctl.at| 32 ++
tests/system-traffic.at | 112 +
utilities/ovs-ofctl.8.in | 31 +++---
utilities/ovs-ofctl.c | 42 ++--
11 files changed, 398 insertions(+), 136 deletions(-)
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 768775898..959845ce6 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1075,6 +1075,10 @@ enum nx_ct_flush_tlv_type {
* by 'enum nx_ct_flush_tuple_tlv_type'*/
/* Primitive types. */
NXT_CT_ZONE_ID = 2,/* be16 zone id. */
+NXT_CT_MARK = 3, /* be32 mark. */
+NXT_CT_MARK_MASK = 4, /* be32 mark mask. */
+NXT_CT_LABELS = 5, /* be128 labels. */
+NXT_CT_LABELS_MASK = 6,/* be128 labels mask. */
};
/* CT flush nested TLVs. */
diff --git a/include/openvswitch/ofp-ct.h b/include/openvswitch/ofp-ct.h
index c8023c309..d57b62678 100644
--- a/include/openvswitch/ofp-ct.h
+++ b/include/openvswitch/ofp-ct.h
@@ -51,15 +51,21 @@ struct ofp_ct_match {
struct ofp_ct_tuple tuple_orig;
struct ofp_ct_tuple tuple_reply;
+
+uint32_t mark;
+uint32_t mark_mask;
+
+ovs_u128 labels;
+ovs_u128 labels_mask;
};
bool ofp_ct_match_is_zero(const struct ofp_ct_match *);
-bool ofp_ct_tuple_is_zero(const struct ofp_ct_tuple *, uint8_t ip_proto);
-bool ofp_ct_tuple_is_five_tuple(const struct ofp_ct_tuple *, uint8_t ip_proto);
+bool ofp_ct_match_is_five_tuple(const struct ofp_ct_match *);
void ofp_ct_match_format(struct ds *, const struct ofp_ct_match *);
-bool ofp_ct_tuple_parse(struct ofp_ct_tuple *, const char *,
-struct ds *, uint8_t *ip_proto, uint16_t *l3_type);
+bool ofp_ct_match_parse(const char **, int argc, struct ds *,
+struct ofp_ct_match *, bool *with_zone,
+uint16_t *zone_id);
enum ofperr ofp_ct_match_decode(struct ofp_ct_match *, bool *with_zone,
uint16_t *zone_id, const struct ofp_header *);
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index f59c6e560..9ff91c955 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -269,6 +269,15 @@ ct_dpif_entry_cmp(const struct ct_dpif_entry *entry,
return false;
}
+if ((match->mark & match->mark_mask) != (entry->mark & match->mark_mask)) {
+return false;
+}
+
+if (!ovs_u128_equals(ovs_u128_and(match->labels, match->labels_mask),
+ ovs_u128_and(entry->labels, match->labels_mask))) {
+return false;
+}
+
return true;
}
@@ -288,15 +297,14 @@ ct_dpif_flush_tuple(struct dpif *dpif, const uint16_t
*zone,
if (VLOG_IS_DBG_ENABLED()) {
struct ds ds = DS_EMPTY_INITIALIZER;
ofp_ct_match_format(, match);
-VLOG_DBG("%s: ct_flush: zone=%d %s", dpif_name(dpif), zone ? *zone : 0,
+VLOG_DBG("%s: ct_flush: zone=%d%s", dpif_name(dpif), zone ? *zone : 0,
ds_cstr());
ds_destroy();
}
/* If we have full five tuple in original and empty reply tuple just
* do the flush over original tuple directly. */
-if (ofp_ct_tuple_is_five_tuple(>tuple_orig, match->ip_proto) &&
-ofp_ct_tuple_is_zero(>tuple_reply, match->ip_proto)) {
+if (ofp_ct_match_is_five_tuple(match)) {
struct ct_dpif_tuple tuple;
ct_dpif_tuple_from_ofp_ct_tuple(>tuple_orig, ,
diff --git a/lib/dpctl.c b/lib/dpctl.c
index cd12625a1..15b04b7a2 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1773,48 +1773,17 @@ dpctl_flush_conntrack(int argc, const char *argv[],
struct dpif *dpif = NULL;
struct ofp_ct_match match = {0};
struct ds ds = DS_EMPTY_INITIALIZER;
-uint16_t zone, *pzone = NULL;
+uint16_t zone;
int error;
int args = argc - 1;
-int zone_pos = 1;
+bool with_zone = false;
if (dp_arg_exists(argc, argv)) {
args--;
-zone_pos = 2;
-}
-
-/* Parse zone. */
-if (args && !strncmp(argv[zone_pos], "zone=", 5)) {
-if (!ovs_scan(argv[zone_pos], "zone=%"SCNu16, )) {
-ds_put_cstr(, "failed to parse zone");
-
[ovs-dev] [PATCH 1/2] ofp-prop: Add helper for parsing and storing of ovs_u128.
Add helper methods that allow us to store and parse the
ovs_u128 type.
Signed-off-by: Ales Musil
---
include/openvswitch/ofp-prop.h | 3 +++
lib/ofp-prop.c | 30 ++
2 files changed, 33 insertions(+)
diff --git a/include/openvswitch/ofp-prop.h b/include/openvswitch/ofp-prop.h
index e676f8dc0..14a451ea0 100644
--- a/include/openvswitch/ofp-prop.h
+++ b/include/openvswitch/ofp-prop.h
@@ -88,6 +88,7 @@ enum ofperr ofpprop_parse_u8(const struct ofpbuf *, uint8_t
*value);
enum ofperr ofpprop_parse_u16(const struct ofpbuf *, uint16_t *value);
enum ofperr ofpprop_parse_u32(const struct ofpbuf *, uint32_t *value);
enum ofperr ofpprop_parse_u64(const struct ofpbuf *, uint64_t *value);
+enum ofperr ofpprop_parse_u128(const struct ofpbuf *, ovs_u128 *value);
enum ofperr ofpprop_parse_uuid(const struct ofpbuf *, struct uuid *);
enum ofperr ofpprop_parse_nested(const struct ofpbuf *, struct ofpbuf *);
@@ -98,10 +99,12 @@ void *ofpprop_put_zeros(struct ofpbuf *, uint64_t type,
size_t len);
void ofpprop_put_be16(struct ofpbuf *, uint64_t type, ovs_be16 value);
void ofpprop_put_be32(struct ofpbuf *, uint64_t type, ovs_be32 value);
void ofpprop_put_be64(struct ofpbuf *, uint64_t type, ovs_be64 value);
+void ofpprop_put_be128(struct ofpbuf *, uint64_t type, ovs_be128 value);
void ofpprop_put_u8(struct ofpbuf *, uint64_t type, uint8_t value);
void ofpprop_put_u16(struct ofpbuf *, uint64_t type, uint16_t value);
void ofpprop_put_u32(struct ofpbuf *, uint64_t type, uint32_t value);
void ofpprop_put_u64(struct ofpbuf *, uint64_t type, uint64_t value);
+void ofpprop_put_u128(struct ofpbuf *, uint64_t type, ovs_u128 value);
void ofpprop_put_bitmap(struct ofpbuf *, uint64_t type, uint64_t bitmap);
void ofpprop_put_flag(struct ofpbuf *, uint64_t type);
void ofpprop_put_uuid(struct ofpbuf *, uint64_t type, const struct uuid *);
diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
index 8b2d8a85a..6884456ad 100644
--- a/lib/ofp-prop.c
+++ b/lib/ofp-prop.c
@@ -250,6 +250,20 @@ ofpprop_parse_u64(const struct ofpbuf *property, uint64_t
*value)
return 0;
}
+/* Attempts to parse 'property' as a property containing a 128-bit value. If
+ * successful, stores the value into '*value' and returns 0; otherwise returns
+ * an OpenFlow error. */
+enum ofperr
+ofpprop_parse_u128(const struct ofpbuf *property, ovs_u128 *value)
+{
+ovs_be128 *p = property->msg;
+if (ofpbuf_msgsize(property) != sizeof *p) {
+return OFPERR_OFPBPC_BAD_LEN;
+}
+*value = ntoh128(*p);
+return 0;
+}
+
/* Attempts to parse 'property' as a property containing a UUID. If
* successful, stores the value into '*uuid' and returns 0; otherwise returns
* an OpenFlow error. */
@@ -351,6 +365,15 @@ ofpprop_put_be64(struct ofpbuf *msg, uint64_t type,
ovs_be64 value)
ofpprop_end(msg, start);
}
+/* Adds a property with the given 'type' and 128-bit 'value' to 'msg'. */
+void
+ofpprop_put_be128(struct ofpbuf *msg, uint64_t type, ovs_be128 value)
+{
+size_t start = ofpprop_start(msg, type);
+ofpbuf_put(msg, , sizeof value);
+ofpprop_end(msg, start);
+}
+
/* Adds a property with the given 'type' and 8-bit 'value' to 'msg'. */
void
ofpprop_put_u8(struct ofpbuf *msg, uint64_t type, uint8_t value)
@@ -381,6 +404,13 @@ ofpprop_put_u64(struct ofpbuf *msg, uint64_t type,
uint64_t value)
ofpprop_put_be64(msg, type, htonll(value));
}
+/* Adds a property with the given 'type' and 64-bit 'value' to 'msg'. */
+void
+ofpprop_put_u128(struct ofpbuf *msg, uint64_t type, ovs_u128 value)
+{
+ofpprop_put_be128(msg, type, hton128(value));
+}
+
/* Appends a property to 'msg' whose type is 'type' and whose contents is a
* series of property headers, one for each 1-bit in 'bitmap'. */
void
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Add monitor condition for FDB.
On Mon, Oct 2, 2023 at 1:00 AM Dumitru Ceara wrote:
>
> On 10/1/23 21:26, Han Zhou wrote:
> > Use conditional monitor for the FDB for local datapaths.
> >
> > Signed-off-by: Han Zhou
> > ---
>
> Hi Han,
>
> This change makes complete sense and I think it should be backported to
> all supported stable branches:
>
> Fixes: 6ec3b12590f9 ("MAC learning: Add a new FDB table in southbound
db.")
>
> There's however one minor issue (also reported by ovsrobot CI at
> https://github.com/ovsrobot/ovn/actions/runs/6373049552).
>
> > v2: remove the accidentally modified OVS commit id of v1.
> >
> > controller/ovn-controller.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 859d9cab917b..2ff78619a3a7 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -211,8 +211,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > {
> > /* Monitor Port_Bindings rows for local interfaces and local
datapaths.
> > *
> > - * Monitor Logical_Flow, MAC_Binding, Multicast_Group, and DNS
tables for
> > - * local datapaths.
> > + * Monitor Logical_Flow, MAC_Binding, FDB, Multicast_Group, and
DNS tables
> > + * for local datapaths.
> > *
> > * Monitor Controller_Event rows for local chassis.
> > *
> > @@ -230,6 +230,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > struct ovsdb_idl_condition lf = OVSDB_IDL_CONDITION_INIT();
> > struct ovsdb_idl_condition ldpg = OVSDB_IDL_CONDITION_INIT();
> > struct ovsdb_idl_condition mb = OVSDB_IDL_CONDITION_INIT();
> > +struct ovsdb_idl_condition fdb = OVSDB_IDL_CONDITION_INIT();
> > struct ovsdb_idl_condition mg = OVSDB_IDL_CONDITION_INIT();
> > struct ovsdb_idl_condition dns = OVSDB_IDL_CONDITION_INIT();
> > struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT();
> > @@ -248,6 +249,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > ovsdb_idl_condition_add_clause_true();
> > ovsdb_idl_condition_add_clause_true();
> > ovsdb_idl_condition_add_clause_true();
> > +ovsdb_idl_condition_add_clause_true();
> > ovsdb_idl_condition_add_clause_true();
> > ovsdb_idl_condition_add_clause_true();
> > ovsdb_idl_condition_add_clause_true();
> > @@ -337,6 +339,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > sbrec_logical_flow_add_clause_logical_datapath(,
OVSDB_F_EQ,
> > uuid);
> > sbrec_mac_binding_add_clause_datapath(, OVSDB_F_EQ,
uuid);
> > +sbrec_fdb_add_clause_dp_key(, OVSDB_F_EQ,
> > +ld->datapath->tunnel_key);
> > sbrec_multicast_group_add_clause_datapath(, OVSDB_F_EQ,
uuid);
> > sbrec_dns_add_clause_datapaths(, OVSDB_F_INCLUDES,
, 1);
> > sbrec_ip_multicast_add_clause_datapath(_mcast,
OVSDB_F_EQ,
> > @@ -360,6 +364,7 @@ out:;
> > sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, ),
> > sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group,
),
> > sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, ),
> > +sb_table_set_req_mon_condition(ovnsb_idl, fdb, ),
> > sb_table_set_req_mon_condition(ovnsb_idl, multicast_group,
),
> > sb_table_set_req_mon_condition(ovnsb_idl, dns, ),
> > sb_table_set_req_mon_condition(ovnsb_idl, controller_event,
),
>
> We're leaking the condition at the end of the function. We're
> missing:
>
> ovsdb_idl_condition_destroy();
Oops, sorry! Thank you (and the robot) for spotting this.
>
> With that addressed and assuming all the OVN tests pass with
> this applied:
>
> Acked-by: Dumitru Ceara
Applied to main and backported down to 22.03.
Thanks,
Han
>
> Thanks,
> Dumitru
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/14] Batch 1: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 11:26:35AM -0700, Jakub Kicinski wrote: > On Wed, 27 Sep 2023 08:57:36 -0700 Kees Cook wrote: > > > Since the element count member must be set before accessing the annotated > > > flexible array member, some patches also move the member's initialization > > > earlier. (These are noted in the individual patches.) > > > > Hi, just checking on this batch of changes. Is it possible to take the > > 1-13 subset: > > On it, sorry for the delay. No worries; thanks for grabbing them! -- Kees Cook ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/14] Batch 1: Annotate structs with __counted_by
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski : On Fri, 22 Sep 2023 10:28:42 -0700 you wrote: > Hi, > > This is the batch 1 of patches touching netdev for preparing for > the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > [...] Here is the summary with links: - [01/14] ipv4: Annotate struct fib_info with __counted_by https://git.kernel.org/netdev/net-next/c/5b98fd5dc1e3 - [02/14] ipv4/igmp: Annotate struct ip_sf_socklist with __counted_by https://git.kernel.org/netdev/net-next/c/210d4e9c732f - [03/14] ipv6: Annotate struct ip6_sf_socklist with __counted_by https://git.kernel.org/netdev/net-next/c/5d22b6528073 - [04/14] net: hns: Annotate struct ppe_common_cb with __counted_by https://git.kernel.org/netdev/net-next/c/5b829c8460ae - [05/14] net: enetc: Annotate struct enetc_int_vector with __counted_by https://git.kernel.org/netdev/net-next/c/dd8e215ea9a8 - [06/14] net: hisilicon: Annotate struct rcb_common_cb with __counted_by https://git.kernel.org/netdev/net-next/c/2290999d278e - [07/14] net: mana: Annotate struct mana_rxq with __counted_by https://git.kernel.org/netdev/net-next/c/a3d7a1209bbb - [08/14] net: ipa: Annotate struct ipa_power with __counted_by https://git.kernel.org/netdev/net-next/c/20551ee45d7d - [09/14] net: mana: Annotate struct hwc_dma_buf with __counted_by https://git.kernel.org/netdev/net-next/c/59656519763d - [10/14] net: openvswitch: Annotate struct dp_meter_instance with __counted_by https://git.kernel.org/netdev/net-next/c/e7b34822fa4d - [11/14] net: enetc: Annotate struct enetc_psfp_gate with __counted_by https://git.kernel.org/netdev/net-next/c/93bc6ab6b19d - [12/14] net: openvswitch: Annotate struct dp_meter with __counted_by https://git.kernel.org/netdev/net-next/c/16ae53d80c00 - [13/14] net: tulip: Annotate struct mediatable with __counted_by https://git.kernel.org/netdev/net-next/c/0d01cfe5aaaf - [14/14] net: sched: Annotate struct tc_pedit with __counted_by (no matching commit) You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 00/14] Batch 1: Annotate structs with __counted_by
On Wed, 27 Sep 2023 08:57:36 -0700 Kees Cook wrote: > > Since the element count member must be set before accessing the annotated > > flexible array member, some patches also move the member's initialization > > earlier. (These are noted in the individual patches.) > > Hi, just checking on this batch of changes. Is it possible to take the > 1-13 subset: On it, sorry for the delay. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: Annotate struct mask_array with __counted_byUse struct_size()
On 10/1/23 13:07, Christophe JAILLET wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> Signed-off-by: Christophe JAILLET
> ---
> This patch is part of a work done in parallel of what is currently worked
> on by Kees Cook.
>
> My patches are only related to corner cases that do NOT match the
> semantic of his Coccinelle script[1].
>
> In this case, in tbl_mask_array_alloc(), several things are allocated with
> a single allocation. Then, some pointer arithmetic computes the address of
> the memory after the flex-array.
>
> [1]
> https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> ---
> net/openvswitch/flow_table.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
> index 9e659db78c05..8d9e83b4d62c 100644
> --- a/net/openvswitch/flow_table.h
> +++ b/net/openvswitch/flow_table.h
> @@ -48,7 +48,7 @@ struct mask_array {
> int count, max;
> struct mask_array_stats __percpu *masks_usage_stats;
> u64 *masks_usage_zero_cntr;
> - struct sw_flow_mask __rcu *masks[];
> + struct sw_flow_mask __rcu *masks[] __counted_by(size);
Did you mean 'max'? There is no 'size' in the structure.
Also, the patch subject is messed up a bit.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH, v3] netdev: Sync'ed and cleaned {get, set}_config().
On Mon, Oct 2, 2023 at 1:52 PM Simon Horman wrote:
>
> On Wed, Sep 27, 2023 at 03:24:07PM +0200, jm...@redhat.com wrote:
> > From: Jakob Meng
> >
> > For better usability, the function pairs get_config() and
> > set_config() for each netdev should be symmetric: Options which are
> > accepted by set_config() should be returned by get_config() and the
> > latter should output valid options for set_config() only.
> >
> > This patch moves key-value pairs which are no valid options from
> > get_config() to the get_status() callback. For example, get_config()
> > in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
> > previously. For requested rx queues the proper option name is n_rxq,
> > so requested_rx_queues has been renamed respectively. Tx queues
> > cannot be changed by the user, hence requested_tx_queues has been
> > dropped. Both configured_{rx,tx}_queues will be returned as
> > n_{r,t}xq in the get_status() callback.
> >
> > The documentation in vswitchd/vswitch.xml for status columns as well
> > as tests have been updated accordingly.
> >
> > Reported-at: https://bugzilla.redhat.com/1949855
> > Signed-off-by: Jakob Meng
>
> Hi Jacob,
>
> I see some CI failures with this change,
> but I'm unsure if they are related to the change or not.
>
> Could you take a look?
I can reproduce issues on vhost-user port mtu tests.
I understand we have a race now.
Previously, the test was calling an explicit dpctl/show which
synchronuously invoked each port get_config(), then matching this
command output against mtu=9000.
Now with this patch, the test looks at the interface status field in ovsdb.
My understanding is that this status field may not have been refreshed
*yet* (or there is another issue and I missed some sync point..).
Adding a sleep 1 before "ovs-vsctl get Interface dpdkvhostuserclient0
mtu" does the trick for me...
I tested adding a check on the vhost-user port link status (see below).
Link status of a vhu port is supposed to be up once the port has been
reconfigured (vhost_reconfigured == which means that
netdev_dpdk_mempool_configure() has been called).
This seems to work, though I never noticed a "down" status in my runs.
So maybe adding one ovs-vsctl command was enough to put some delay in the test..
$ git diff
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 95aa3e906b..59fec6903f 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -749,6 +749,7 @@ tail -f /dev/null | dpdk-testpmd
--socket-mem="$(cat NUMA_NODE)" --no-pci\
--single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
link_state | grep -w up])
dnl Check MTU value in the datapath
AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
@@ -887,6 +888,7 @@ tail -f /dev/null | dpdk-testpmd
--socket-mem="$(cat NUMA_NODE)" --no-pci\
--single-file-segments -- -a
>$OVS_RUNDIR/testpmd-dpdkvhostuserclient0.log 2>&1 &
OVS_WAIT_UNTIL([grep "virtio is now ready for processing" ovs-vswitchd.log])
+OVS_WAIT_UNTIL([ovs-vsctl get Interface dpdkvhostuserclient0
link_state | grep -w up])
dnl Check MTU value in the datapath
AT_CHECK([ovs-vsctl get Interface dpdkvhostuserclient0 mtu], [0], [dnl
HTH.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/3] vswitchd, ofproto-dpif: Propagate the CT limit from database
On Mon, Oct 02, 2023 at 12:33:57PM +0200, Ales Musil wrote:
> On Thu, Sep 28, 2023 at 9:35 AM Simon Horman wrote:
>
> > On Tue, Sep 26, 2023 at 12:03:51PM +0200, Ales Musil wrote:
> > > Progpagate the CT limit that is present in the DB into
> > > datapath. The limit is currently only propagated on change
> > > and can be overwritten by the dpctl commands.
> > >
> > > Signed-off-by: Ales Musil
> >
> > ...
> >
> > > @@ -6366,7 +6378,7 @@ handle_flow_mod__(struct ofproto *ofproto, const
> > struct ofputil_flow_mod *fm,
> > > error = ofproto_flow_mod_start(ofproto, );
> > > if (!error) {
> > > ofproto_bump_tables_version(ofproto);
> > > -error = ofproto_flow_mod_finish(ofproto, , req);
> > > +error = ofproto_flow_mod_finish(ofproto, , req);
> >
> > Hi Ales,
> >
>
> Hi Simon,
>
> thank you for the review.
>
>
> > this hunk appears to be a whitespace change that
> > is unrelated to the subject of this patch.
> >
>
> Yes that indeed slipped through, it should be fixed in v3.
>
> >
> > > ofmonitor_flush(ofproto->connmgr);
> > > }
> > > ovs_mutex_unlock(_mutex);
> > > @@ -8437,7 +8449,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t
> > id, uint16_t flags)
> > > /* Send error referring to the original message. */
> > > ofconn_send_error(ofconn, be->msg, error);
> > > error = OFPERR_OFPBFC_MSG_FAILED;
> > > -
> > > +
> >
> > Ditto.
> >
> > > /* 2. Revert. Undo all the changes made above. */
> > > LIST_FOR_EACH_REVERSE_CONTINUE(be, node, >msg_list)
> > {
> > > if (be->type == OFPTYPE_FLOW_MOD) {
> >
> > Also, please add a '.' to the end of the patch subject,
> > as flagged by the 0-day Robot.
> >
> > Other than the above this patch looks good to me.
> >
> >
> Yeah I didn't realize that this new rule is in place, v3 has the dot at the
> end.
Thanks, v3 looks good to me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 branch-2.17 2/2] conntrack: Remove nat_conn introducing key directionality.
On Wed, Sep 27, 2023 at 11:31:22AM -0400, Aaron Conole wrote: > From: Peng He > > The patch avoids the extra allocation for nat_conn. > Currently, when doing NAT, the userspace conntrack will use an extra > conn for the two directions in a flow. However, each conn has actually > the two keys for both orig and rev directions. This patch introduces a > key_node[CT_DIRS] member as per Aaron's suggestion in the conn which > consists of a key, direction, and a cmap_node for hash lookup so > addressing the feedback received by the original patch [0]. > > With this adjustment, we also remove the assertion that connections in > the table are DEFAULT while updating connection state and/or removing > connections. > > [0] > https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/ > > [Aaron resolved numerous conflicts due to lack of multiple commits] Hi Aaron, unfortunately I see numerous failures flagged in CI with this change applied. And I was able to see similar failures locally. For test #98, the key in the logs of my local run seemed to be: 2023-10-02T12:02:50Z|1|unixctl|WARN|error communicating with unix:.../tests/system-userspace-testsuite.dir/098/ovs-vswitchd.813454.ctl: End of file ovs-appctl: ovs-vswitchd: transaction error (End of file) ./system-traffic.at:4206: exit code was 1, expected 0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH 0/7] net: openvswitch: Reduce stack usage
On 9/29/23 09:06, Nicholas Piggin wrote: > On Wed Sep 27, 2023 at 6:36 PM AEST, Ilya Maximets wrote: >> On 9/27/23 02:13, Nicholas Piggin wrote: >>> Hi, >>> >>> We've got a report of a stack overflow on ppc64le with a 16kB kernel >>> stack. Openvswitch is just one of many things in the stack, but it >>> does cause recursion and contributes to some usage. >>> >>> Here are a few patches for reducing stack overhead. I don't know the >>> code well so consider them just ideas. GFP_ATOMIC allocations >>> introduced in a couple of places might be controversial, but there >>> is still some savings to be had if you skip those. >>> >>> Here is one place detected where the stack reaches >14kB before >>> overflowing a little later. I massaged the output so it just shows >>> the stack frame address on the left. >> >> Hi, Nicholas. Thanks for the patches! >> >> Though it looks like OVS is not really playing a huge role in the >> stack trace below. How much of the stack does the patch set save >> in total? How much patches 2-7 contribute (I posted a patch similar >> to the first one last week, so we may not count it)? > > Stack usage was tested for the same path (this is backported to > RHEL9 kernel), and saving was 2080 bytes for that. It's enough > to get us out of trouble. But if it was a config that caused more > recursions then it might still be a problem. The 2K total value likely means that only patches 1 and 4 actually contribute much into the savings. And I agree that running at 85%+ stack utilization seems risky. It can likely be overflowed by just a few more recirculations in OVS pipeline or traversing one more network namespace on a way out. And it's possible that some of the traffic will take such a route in your system even if you didn't see it yet. >> Also, most of the changes introduced here has a real chance to >> noticeably impact performance. Did you run any performance tests >> with this to assess the impact? > > Some numbers were posted by Aaron as you would see. 2-4% for that > patch, but I suspect the rest should have much smaller impact. They also seem to have a very small impact on the stack usage, so may be not worth touching at all, since performance evaluation for them will be necessary before they can be accepted. > > Maybe patch 2 if you were doing a lot of push_nsh operations, but > that might be less important since it's out of the recursive path. It's also unlikely that you have NHS pipeline configured in OVS. > >> >> One last thing is that at least some of the patches seem to change >> non-inlined non-recursive functions. Seems unnecessary. >> >> Best regards, Ilya Maximets. >> > > One thing I do notice in the trace: > > > clone_execute is an action which can be deferred AFAIKS, but it is > not deferred until several recursions deep. > > If we deferred always when possible, then might avoid such a big > stack (at least for this config). Is it very costly to defer? Would > it help here, or is it just going to process it right away and > cause basically the same call chain? It may save at most two stack frames maybe, because deferred actions will be called just one function above in ovs_execute_actions(), and it will not save us from packets exiting openvswitch module and re-entering from a different port, which is a case in the provided trace. Also, I'd vote against deferring, because then we'll start hitting the limit of deferred actions much faster causing packet drops, which is already a problem for some OVN deployments. And deferring involves copying a lot of memory, which will hit performance once again. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC PATCH 0/7] net: openvswitch: Reduce stack usage
On 9/28/23 03:52, Nicholas Piggin wrote: > On Wed Sep 27, 2023 at 6:36 PM AEST, Ilya Maximets wrote: >> On 9/27/23 02:13, Nicholas Piggin wrote: >>> Hi, >>> >>> We've got a report of a stack overflow on ppc64le with a 16kB kernel >>> stack. Openvswitch is just one of many things in the stack, but it >>> does cause recursion and contributes to some usage. >>> >>> Here are a few patches for reducing stack overhead. I don't know the >>> code well so consider them just ideas. GFP_ATOMIC allocations >>> introduced in a couple of places might be controversial, but there >>> is still some savings to be had if you skip those. >>> >>> Here is one place detected where the stack reaches >14kB before >>> overflowing a little later. I massaged the output so it just shows >>> the stack frame address on the left. >> >> Hi, Nicholas. Thanks for the patches! > > Hey, sorry your mail didn't come through for me (though it's on the > list)... Anyway thanks for the feedback. > > And the important thing I forgot to mention: this was reproduced on a > RHEL9 kernel and that's where the traces are from. Upstream is quite > similar though so the code and call chains and stack use should be > pretty close. > > It's a complicated configuration we're having difficulty with testing > upstream kernel. People are working to test things on the RHEL kernel > but I wanted to bring this upstream before we get too far down that > road. > > Unfortunately that means I don't have performance or exact stack > use savings yet. But I will let you know if/when I get results. > >> Though it looks like OVS is not really playing a huge role in the >> stack trace below. How much of the stack does the patch set save >> in total? How much patches 2-7 contribute (I posted a patch similar >> to the first one last week, so we may not count it)? > > ovs functions themselves are maybe 30% of stack use, so significant. I > did find they are the ones with some of the biggest structures in local > variables though, so low hanging fruit. This series should save about > 2kB of stack, by eyeball. Should be enough to get us out of trouble for > this scenario, at least. Unfortunately, the only low handing fruit in this set is patch #1, the rest needs a serious performance evaluation. > > I don't suggest ovs is the only problem, I'm just trying to trim things > where possible. I have been trying to find other savings too, e.g., > https://lore.kernel.org/linux-nfs/20230927001624.750031-1-npig...@gmail.com/ > > Recursion is a difficulty. I think we recursed 3 times in ovs, and it > looks like there's either 1 or 2 more recursions possible before the > limit (depending on how the accounting works, not sure if it stops at > 4 or 5), so we're a long way off. ppc64le doesn't use an unusually large > amount of stack, probably more than x86-64, but shouldn't be by a big > factor. So it could be risky for any arch with 16kB stack. The stack trace looks like a very standard trace for something like an ovn-kubernetes setup. And I haven't seen such issues on x86 or aarch64 systems. What architectures beside ppc64le use 16kB stack? > > I wonder if we should have an arch function that can be called by > significant recursion points such as this, which signals free stack is > low and you should bail out ASAP. I don't think it's reasonable to > expect ovs to know about all arch size and usage of stack. You could > keep your hard limit for consistency, but if that goes wrong the > low free stack indication could save you. Every part of the code will need to react somehow to such a signal, so I'm not sure how the implementations would look like. > >> >> Also, most of the changes introduced here has a real chance to >> noticeably impact performance. Did you run any performance tests >> with this to assess the impact? > > Will see if we can do that, but I doubt this setup would be too > sensitive to small changes so it might be something upstream would have > to help evaluate. Function calls and even small kmalloc/free on the same > CPU shouldn't be too costly I hope, but I don't know how hot these paths > can get. This code path unfortunately is as hot as it gets as it needs to be able to process millions of packets per second. Even small changes may cause significant performance impact. > >> >> One last thing is that at least some of the patches seem to change >> non-inlined non-recursive functions. Seems unnecessary. > > I was concentrating on functions in the recursive path, but there > were one or two big ones just off the side that still can be called > when you're deep into stack. In general it's just a good idea to > be frugal as reasonably possible with kernel stack always so I > wouldn't say unnecessary, but yes arguably less important. I defer > to your judgement about cost and benefit of all these changes > though. > > Thanks, > Nick ___ dev mailing list d...@openvswitch.org
Re: [ovs-dev] [PATCH, v3] netdev: Sync'ed and cleaned {get, set}_config().
On Wed, Sep 27, 2023 at 03:24:07PM +0200, jm...@redhat.com wrote:
> From: Jakob Meng
>
> For better usability, the function pairs get_config() and
> set_config() for each netdev should be symmetric: Options which are
> accepted by set_config() should be returned by get_config() and the
> latter should output valid options for set_config() only.
>
> This patch moves key-value pairs which are no valid options from
> get_config() to the get_status() callback. For example, get_config()
> in lib/netdev-dpdk.c returned {configured,requested}_{rx,tx}_queues
> previously. For requested rx queues the proper option name is n_rxq,
> so requested_rx_queues has been renamed respectively. Tx queues
> cannot be changed by the user, hence requested_tx_queues has been
> dropped. Both configured_{rx,tx}_queues will be returned as
> n_{r,t}xq in the get_status() callback.
>
> The documentation in vswitchd/vswitch.xml for status columns as well
> as tests have been updated accordingly.
>
> Reported-at: https://bugzilla.redhat.com/1949855
> Signed-off-by: Jakob Meng
Hi Jacob,
I see some CI failures with this change,
but I'm unsure if they are related to the change or not.
Could you take a look?
https://patchwork.ozlabs.org/project/openvswitch/patch/20230927132406.2076833-1-jm...@redhat.com/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/1] ofproto-dpif-trace: Improve conjunctive match tracing.
+ Mike Pattrick On Mon, Sep 25, 2023 at 06:09:00PM +0900, Nobuhiro MIKI wrote: > A conjunctive flow consists of two or more multiple flows with > conjunction actions. When input to the ofproto/trace command > matches a conjunctive flow, it outputs flows of all dimensions. > > Signed-off-by: Nobuhiro MIKI > --- > v3: > * Remove struct flow changes. > * Use struct 'cls_rule' instead of struct 'flow'. > * Add priority and id conditionals for 'soft' arrays. > * Use 'minimask' in struct 'cls_rule' as mask. Hi Miki-san, I am not seeing minimask used in this patch. > * Use hmapx instead of ovs_list to store conj flows. > * Passe 'conj_flows' as an argument only when tracing. Other than the minimask question above, it seems to me that Ilya's review of v2 has been addressed. ... > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c ... > @@ -918,6 +941,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct > rule_dpif *rule, > ctx->xin->trace = _report(ctx->xin->trace, OFT_TABLE, >ds_cstr())->subs; > ds_destroy(); > + > +xlate_report_conj_matches(ctx); > } > > /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' > @@ -4653,7 +4678,8 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t > in_port, uint8_t table_id, > ctx->xin->resubmit_stats, > >table_id, in_port, > may_packet_in, honor_table_miss, > - ctx->xin->xcache); > + ctx->xin->xcache, > + >xout->conj_flows); As per my comment here [1] on another patch, I'm concerned about functions with too many (say more than 6 arguments). For one thing. AFAIK, there is the overhead of needing to pass arguments the stack rather than purely in registers. For another, I think there are questions of readability and maintainability. It is already the case before this patch, although this patch does make it slightly worse. [1] Re: [PATCH v3] ofproto-dpif-mirror: Add support for pre-selection filter https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408228.html ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/3] vswitchd, ofproto-dpif: Propagate the CT limit from database
On Thu, Sep 28, 2023 at 9:35 AM Simon Horman wrote:
> On Tue, Sep 26, 2023 at 12:03:51PM +0200, Ales Musil wrote:
> > Progpagate the CT limit that is present in the DB into
> > datapath. The limit is currently only propagated on change
> > and can be overwritten by the dpctl commands.
> >
> > Signed-off-by: Ales Musil
>
> ...
>
> > @@ -6366,7 +6378,7 @@ handle_flow_mod__(struct ofproto *ofproto, const
> struct ofputil_flow_mod *fm,
> > error = ofproto_flow_mod_start(ofproto, );
> > if (!error) {
> > ofproto_bump_tables_version(ofproto);
> > -error = ofproto_flow_mod_finish(ofproto, , req);
> > +error = ofproto_flow_mod_finish(ofproto, , req);
>
> Hi Ales,
>
Hi Simon,
thank you for the review.
> this hunk appears to be a whitespace change that
> is unrelated to the subject of this patch.
>
Yes that indeed slipped through, it should be fixed in v3.
>
> > ofmonitor_flush(ofproto->connmgr);
> > }
> > ovs_mutex_unlock(_mutex);
> > @@ -8437,7 +8449,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t
> id, uint16_t flags)
> > /* Send error referring to the original message. */
> > ofconn_send_error(ofconn, be->msg, error);
> > error = OFPERR_OFPBFC_MSG_FAILED;
> > -
> > +
>
> Ditto.
>
> > /* 2. Revert. Undo all the changes made above. */
> > LIST_FOR_EACH_REVERSE_CONTINUE(be, node, >msg_list)
> {
> > if (be->type == OFPTYPE_FLOW_MOD) {
>
> Also, please add a '.' to the end of the patch subject,
> as flagged by the 0-day Robot.
>
> Other than the above this patch looks good to me.
>
>
Yeah I didn't realize that this new rule is in place, v3 has the dot at the
end.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 0/3] Expose CT limit via DB
The series exposes CT limit via DB, adding user friendly ovs-vsctl interface. The DB value has priority before the dpctl interface, this is achieved by storing which CT limit is protected. The dpctl will return an error if the limit is already set in DB for that zone. Ales Musil (3): ovs-vsctl: Add limit to CT zone. vswitchd, ofproto-dpif: Propagate the CT limit from database. netlink, netdev: Enforce CT limit protection. NEWS | 2 + lib/conntrack.c| 51 +++--- lib/conntrack.h| 5 +- lib/ct-dpif.c | 9 ++-- lib/ct-dpif.h | 5 +- lib/dpctl.c| 4 +- lib/dpif-netdev.c | 12 +++-- lib/dpif-netlink.c | 37 +++-- lib/dpif-provider.h| 13 +++-- ofproto/ofproto-dpif.c | 41 ++ ofproto/ofproto-dpif.h | 5 ++ ofproto/ofproto-provider.h | 5 ++ ofproto/ofproto.c | 12 + ofproto/ofproto.h | 2 + tests/ovs-vsctl.at | 58 tests/system-traffic.at| 74 - utilities/ovs-vsctl.8.in | 20 ++- utilities/ovs-vsctl.c | 108 - vswitchd/bridge.c | 9 vswitchd/vswitch.ovsschema | 9 +++- vswitchd/vswitch.xml | 5 ++ 21 files changed, 439 insertions(+), 47 deletions(-) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 3/3] netlink, netdev: Enforce CT limit protection.
Enforce the CT limit protection, it ensures that
any CT limit value that was set by forced operation,
currently the DB CT limit, will be protected against
overwrite from other sources, e.g. the dpctl command.
Signed-off-by: Ales Musil
Acked-by: Simon Horman
---
v3: Rebase on top of current master.
Add ack from Simon and fix the missing '.'.
---
lib/conntrack.c | 51 ++---
lib/conntrack.h | 5 ++--
lib/ct-dpif.c | 9
lib/ct-dpif.h | 5 ++--
lib/dpctl.c | 4 ++--
lib/dpif-netdev.c | 12 ++
lib/dpif-netlink.c | 37 ++
lib/dpif-provider.h | 13 +++
ofproto/ofproto-dpif.c | 6 +++--
tests/system-traffic.at | 28 ++
10 files changed, 126 insertions(+), 44 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443fba..b5b5d4a4c 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -85,6 +85,7 @@ enum ct_alg_ctl_type {
struct zone_limit {
struct cmap_node node;
struct conntrack_zone_limit czl;
+bool limit_protected;
};
static bool conn_key_extract(struct conntrack *, struct dp_packet *,
@@ -344,17 +345,13 @@ zone_limit_get(struct conntrack *ct, int32_t zone)
}
static int
-zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit)
+zone_limit_create(struct conntrack *ct, int32_t zone, uint32_t limit,
+ bool limit_protected)
OVS_REQUIRES(ct->ct_lock)
{
-struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
-
-if (zl) {
-return 0;
-}
-
if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
-zl = xzalloc(sizeof *zl);
+struct zone_limit *zl = xzalloc(sizeof *zl);
+zl->limit_protected = limit_protected;
zl->czl.limit = limit;
zl->czl.zone = zone;
zl->czl.zone_limit_seq = ct->zone_limit_seq++;
@@ -366,18 +363,28 @@ zone_limit_create(struct conntrack *ct, int32_t zone,
uint32_t limit)
}
}
+static inline bool
+can_update_zone_limit(struct zone_limit *zl, bool force)
+{
+return !(zl && zl->limit_protected && !force);
+}
+
int
-zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit)
+zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
+ bool force)
{
int err = 0;
-struct zone_limit *zl = zone_limit_lookup(ct, zone);
-if (zl) {
+ovs_mutex_lock(>ct_lock);
+
+struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
+if (!can_update_zone_limit(zl, force)) {
+err = EPERM;
+} else if (zl) {
zl->czl.limit = limit;
+zl->limit_protected = force;
VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
} else {
-ovs_mutex_lock(>ct_lock);
-err = zone_limit_create(ct, zone, limit);
-ovs_mutex_unlock(>ct_lock);
+err = zone_limit_create(ct, zone, limit, force);
if (!err) {
VLOG_INFO("Created zone limit of %u for zone %d", limit, zone);
} else {
@@ -385,6 +392,8 @@ zone_limit_update(struct conntrack *ct, int32_t zone,
uint32_t limit)
zone);
}
}
+
+ovs_mutex_unlock(>ct_lock);
return err;
}
@@ -398,20 +407,24 @@ zone_limit_clean(struct conntrack *ct, struct zone_limit
*zl)
}
int
-zone_limit_delete(struct conntrack *ct, uint16_t zone)
+zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force)
{
+int err = 0;
ovs_mutex_lock(>ct_lock);
+
struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
-if (zl) {
+if (!can_update_zone_limit(zl, force)) {
+err = EPERM;
+} else if (zl) {
zone_limit_clean(ct, zl);
-ovs_mutex_unlock(>ct_lock);
VLOG_INFO("Deleted zone limit for zone %d", zone);
} else {
-ovs_mutex_unlock(>ct_lock);
VLOG_INFO("Attempted delete of non-existent zone limit: zone %d",
zone);
}
-return 0;
+
+ovs_mutex_unlock(>ct_lock);
+return err;
}
static void
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 57d5159b6..a58a800f9 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -153,7 +153,8 @@ bool conntrack_get_tcp_seq_chk(struct conntrack *ct);
struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
struct conntrack_zone_limit zone_limit_get(struct conntrack *ct,
int32_t zone);
-int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit);
-int zone_limit_delete(struct conntrack *ct, uint16_t zone);
+int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit,
+ bool force);
+int zone_limit_delete(struct conntrack *ct, uint16_t zone, bool force);
#endif /* conntrack.h */
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index f59c6e560..335ba09f9 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -399,11 +399,11 @@
[ovs-dev] [PATCH v3 2/3] vswitchd, ofproto-dpif: Propagate the CT limit from database.
Progpagate the CT limit that is present in the DB into
datapath. The limit is currently only propagated on change
and can be overwritten by the dpctl commands.
Signed-off-by: Ales Musil
Acked-by: Simon Horman
---
v3: Rebase on top of current master.
Add ack from Simon and fix the missing '.'.
Revert the unrelated change.
---
ofproto/ofproto-dpif.c | 39
ofproto/ofproto-dpif.h | 5 +
ofproto/ofproto-provider.h | 5 +
ofproto/ofproto.c | 12 ++
ofproto/ofproto.h | 2 ++
tests/system-traffic.at| 46 +-
vswitchd/bridge.c | 9
7 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ba5706f6a..55eaeefa3 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -220,6 +220,7 @@ static void ofproto_unixctl_init(void);
static void ct_zone_config_init(struct dpif_backer *backer);
static void ct_zone_config_uninit(struct dpif_backer *backer);
static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
+static void ct_zone_limits_commit(struct dpif_backer *backer);
static inline struct ofproto_dpif *
ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -513,6 +514,7 @@ type_run(const char *type)
process_dpif_port_changes(backer);
ct_zone_timeout_policy_sweep(backer);
+ct_zone_limits_commit(backer);
return 0;
}
@@ -5522,6 +5524,8 @@ ct_zone_config_init(struct dpif_backer *backer)
cmap_init(>ct_zones);
hmap_init(>ct_tps);
ovs_list_init(>ct_tp_kill_list);
+ovs_list_init(>ct_zone_limits_to_add);
+ovs_list_init(>ct_zone_limits_to_del);
clear_existing_ct_timeout_policies(backer);
}
@@ -5545,6 +5549,8 @@ ct_zone_config_uninit(struct dpif_backer *backer)
id_pool_destroy(backer->tp_ids);
cmap_destroy(>ct_zones);
hmap_destroy(>ct_tps);
+ct_dpif_free_zone_limits(>ct_zone_limits_to_add);
+ct_dpif_free_zone_limits(>ct_zone_limits_to_del);
}
static void
@@ -5625,6 +5631,38 @@ ct_del_zone_timeout_policy(const char *datapath_type,
uint16_t zone_id)
}
}
+static void
+ct_zone_limit_update(const char *datapath_type, uint16_t zone_id,
+ int64_t *limit)
+{
+struct dpif_backer *backer = shash_find_data(_dpif_backers,
+ datapath_type);
+if (!backer) {
+return;
+}
+
+if (limit) {
+ct_dpif_push_zone_limit(>ct_zone_limits_to_add, zone_id,
+*limit, 0);
+} else {
+ct_dpif_push_zone_limit(>ct_zone_limits_to_del, zone_id, 0, 0);
+}
+}
+
+static void
+ct_zone_limits_commit(struct dpif_backer *backer)
+{
+if (!ovs_list_is_empty(>ct_zone_limits_to_add)) {
+ct_dpif_set_limits(backer->dpif, NULL, >ct_zone_limits_to_add);
+ct_dpif_free_zone_limits(>ct_zone_limits_to_add);
+}
+
+if (!ovs_list_is_empty(>ct_zone_limits_to_del)) {
+ct_dpif_del_limits(backer->dpif, >ct_zone_limits_to_del);
+ct_dpif_free_zone_limits(>ct_zone_limits_to_del);
+}
+}
+
static void
get_datapath_cap(const char *datapath_type, struct smap *cap)
{
@@ -6914,4 +6952,5 @@ const struct ofproto_class ofproto_dpif_class = {
ct_flush, /* ct_flush */
ct_set_zone_timeout_policy,
ct_del_zone_timeout_policy,
+ct_zone_limit_update,
};
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d8e0cd37a..b863dd6fc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -284,6 +284,11 @@ struct dpif_backer {
feature than 'bt_support'. */
struct atomic_count tnl_count;
+
+struct ovs_list ct_zone_limits_to_add; /* CT zone limits queued for
+ * addition into datapath. */
+struct ovs_list ct_zone_limits_to_del; /* CT zone limt queued for
+ * deletion from datapath. */
};
/* All existing ofproto_backer instances, indexed by ofproto->up.type. */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 9f7b8b6e8..5604aa65b 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1921,6 +1921,11 @@ struct ofproto_class {
/* Deletes the timeout policy associated with 'zone' in datapath type
* 'dp_type'. */
void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
+
+/* Queues the CT zone limit update. In order for this change to take
+ * effect it needs to be commited. */
+void (*ct_zone_limit_update)(const char *dp_type, uint16_t zone,
+ int64_t *limit);
};
extern const struct ofproto_class ofproto_dpif_class;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e78c80d11..e054753b8 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1026,6 +1026,18
[ovs-dev] [PATCH v3 1/3] ovs-vsctl: Add limit to CT zone.
Add limit to the CT zone DB table with ovs-vsctl
helper methods. The limit has two special values
besides any number, 0 is unlimited and empty limit
is to leave the value untouched in the datapath.
This is preparation step and the value is not yet
propagated to the datapath.
Signed-off-by: Ales Musil
Acked-by: Simon Horman
---
v3: Rebase on top of current master.
Add ack from Simon and fix the missing '.'.
---
NEWS | 2 +
tests/ovs-vsctl.at | 58
utilities/ovs-vsctl.8.in | 20 ++-
utilities/ovs-vsctl.c | 108 -
vswitchd/vswitch.ovsschema | 9 +++-
vswitchd/vswitch.xml | 5 ++
6 files changed, 198 insertions(+), 4 deletions(-)
diff --git a/NEWS b/NEWS
index 6b45492f1..e86e7f364 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,8 @@ Post-v3.2.0
from older version is supported but it may trigger more leader elections
during the process, and error logs complaining unrecognized fields may
be observed on old nodes.
+ - CT:
+* Add support for setting CT zone limit via DB.
v3.2.0 - 17 Aug 2023
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a368bff6e..b033eaf1f 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -975,6 +975,47 @@ AT_CHECK(
[0], [stdout])
AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:10, Timeout
Policies: system default
])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
+
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=1 limit=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 1, Limit: 1
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=1 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 1, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=10 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 10, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=10 icmp_first=1
icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
+])
+
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=10 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 10, Limit: 5
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: system default
+])
AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0
'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])],
[0], [stdout])
AT_CHECK([RUN_OVS_VSCTL([list-dp-cap system])], [0], [recirc=true
@@ -1123,6 +1164,23 @@ AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
Policies: icmp_first=2 icmp_reply=3
])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdevxx zone=5 limit=1])],
+ [1], [], [ovs-vsctl: datapath netdevxx does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=8 limit=1])],
+ [1], [], [ovs-vsctl: zone_id (8) out of range
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=-1])],
+ [1], [], [ovs-vsctl: limit (-1) out of range
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])],
+ [1], [], [ovs-vsctl: zone_id 10 does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=1])])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=2])],
+ [1], [], [ovs-vsctl: zone_id 5 already exists
+])
+
AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0
'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])],
[0], [stdout])
AT_CHECK([RUN_OVS_VSCTL([list-dp-cap nosystem])],
[1], [], [ovs-vsctl: datapath "nosystem" record not found
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 9e319aa1c..e6c0d6b2c 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -354,7 +354,7 @@ Prints the name of the bridge that contains \fIiface\fR on
standard
output.
.
.SS "Conntrack Zone Commands"
-These commands query and modify datapath CT zones and Timeout Policies.
+These commands query and modify datapath CT zones, Timeout Policies and Limits.
.
.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id
\fIpolicies\fR"
Creates a conntrack zone timeout policy with \fIzone_id\fR in
@@ -379,6 +379,24 @@ delete a zone that does not exist has no
Re: [ovs-dev] [PATCH v3] ofproto-dpif-mirror: Add support for pre-selection filter
On Fri, Sep 08, 2023 at 12:28:24PM -0400, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> I benchmarked this with two setups. A netlink based test with two veth
> pairs connected to a single bridge, and a netdev based test involving a
> mix of DPDK nics, and netdev-linux interfaces. Both tests involved
> saturating the link with iperf3 and then sending an icmp ping every
> second. I then measured the throughput on the link with no mirroring,
> icmp pre-selected mirroring, and full mirroring. The results, below,
> indicate a significant reduction to impact of mirroring when only a
> subset of the traffic on a port is selected for collection.
>
> Test No Mirror | No Filter | Filter | No Filter | Filter |
> +---+---+---+---+--+
> netlink | 39.0 Gbps | 36.1 Gbps | 38.2 Gbps | 7%|2%|
> netdev | 7.39 Gbps | 4.95 Gbps | 6.24 Gbps |33%| 15%|
>
> The ratios above are the percent reduction in total throughput when
> mirroring is used either with or without a filter.
>
> Signed-off-by: Mike Pattrick
...
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
...
> @@ -212,7 +218,9 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> struct ofbundle **dsts, size_t n_dsts,
> unsigned long *src_vlans, struct ofbundle *out_bundle,
> uint16_t snaplen,
> - uint16_t out_vlan)
> + uint16_t out_vlan,
> + const char *filter,
> + const struct ofproto *ofproto)
Hi Mike,
I am concerned about the implications of passing large numbers of
function arguments, which seems to happen several places in this patch.
> {
> struct mbundle *mbundle, *out;
> mirror_mask_t mirror_bit;
...
> @@ -430,6 +482,7 @@ mirror_get(struct mbridge *mbridge, int index, const
> unsigned long **vlans,
> if (!mirror) {
> return false;
> }
> +
> /* Assume 'mirror' is RCU protected, i.e., it will not be freed until
> this
> * thread quiesces. */
>
nit: this hunk seems unrelated to the subject of this patch.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-build] |fail| pw1840316 [ovs-dev, v2, branch-2.17, 2/2] conntrack: Remove nat_conn introducing key directionality.
On 9/29/23 15:11, Phelan, Michael wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Wednesday, September 27, 2023 5:52 PM >> To: Phelan, Michael ; Stokes, Ian >> >> Cc: i.maxim...@ovn.org; Aaron Conole ; ovs-dev >> >> Subject: Re: [ovs-build] |fail| pw1840316 [ovs-dev, v2, branch-2.17, 2/2] >> conntrack: Remove nat_conn introducing key directionality. >> >>> Test-Label: intel-ovs-compilation >>> Test-Status: fail >>> http://patchwork.ozlabs.org/api/patches/1840316/ >>> >>> AVX-512_compilation: failed >>> DPLCS Test: fail >>> DPIF Test: fail >>> MFEX Test: fail >>> Actions Test: fail >>> Errors in DPCLS test: >>> make check-dpdk >>> make all-am >>> make[1]: Entering directory '/root/ovs-dev' >>> make[1]: Leaving directory '/root/ovs-dev' >>> set /bin/bash './tests/system-dpdk-testsuite' -C tests >>> AUTOTEST_PATH='utilities:vswitchd:ovsdb:vtep:tests:ipsec::'; \ "$@" >>> -j1 || (test X'' = Xyes && "$@" --recheck) ## >>> -- ## ## openvswitch 3.2.90 test suite. ## >>> ## -- ## >> >> >> Hi, Michael. >> >> There is something wrong with builds on older branches. >> This patch is for branch-2.17, but the log above says version 3.2.90. >> >> Could you, please, take a look? > Hi Ilya, > Sorry for the delay in getting back to you. The patch header was formatted > differently to how the CI expects so the branch was not picked up correctly. > Actual results for this patchset were sent to the ML this morning. Thanks! > > Thanks, > Michael. >> >> Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] ovn-controller: Add monitor condition for FDB.
On 10/1/23 21:26, Han Zhou wrote:
> Use conditional monitor for the FDB for local datapaths.
>
> Signed-off-by: Han Zhou
> ---
Hi Han,
This change makes complete sense and I think it should be backported to
all supported stable branches:
Fixes: 6ec3b12590f9 ("MAC learning: Add a new FDB table in southbound db.")
There's however one minor issue (also reported by ovsrobot CI at
https://github.com/ovsrobot/ovn/actions/runs/6373049552).
> v2: remove the accidentally modified OVS commit id of v1.
>
> controller/ovn-controller.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 859d9cab917b..2ff78619a3a7 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -211,8 +211,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> {
> /* Monitor Port_Bindings rows for local interfaces and local datapaths.
> *
> - * Monitor Logical_Flow, MAC_Binding, Multicast_Group, and DNS tables for
> - * local datapaths.
> + * Monitor Logical_Flow, MAC_Binding, FDB, Multicast_Group, and DNS
> tables
> + * for local datapaths.
> *
> * Monitor Controller_Event rows for local chassis.
> *
> @@ -230,6 +230,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> struct ovsdb_idl_condition lf = OVSDB_IDL_CONDITION_INIT();
> struct ovsdb_idl_condition ldpg = OVSDB_IDL_CONDITION_INIT();
> struct ovsdb_idl_condition mb = OVSDB_IDL_CONDITION_INIT();
> +struct ovsdb_idl_condition fdb = OVSDB_IDL_CONDITION_INIT();
> struct ovsdb_idl_condition mg = OVSDB_IDL_CONDITION_INIT();
> struct ovsdb_idl_condition dns = OVSDB_IDL_CONDITION_INIT();
> struct ovsdb_idl_condition ce = OVSDB_IDL_CONDITION_INIT();
> @@ -248,6 +249,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> ovsdb_idl_condition_add_clause_true();
> ovsdb_idl_condition_add_clause_true();
> ovsdb_idl_condition_add_clause_true();
> +ovsdb_idl_condition_add_clause_true();
> ovsdb_idl_condition_add_clause_true();
> ovsdb_idl_condition_add_clause_true();
> ovsdb_idl_condition_add_clause_true();
> @@ -337,6 +339,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> sbrec_logical_flow_add_clause_logical_datapath(, OVSDB_F_EQ,
> uuid);
> sbrec_mac_binding_add_clause_datapath(, OVSDB_F_EQ, uuid);
> +sbrec_fdb_add_clause_dp_key(, OVSDB_F_EQ,
> +ld->datapath->tunnel_key);
> sbrec_multicast_group_add_clause_datapath(, OVSDB_F_EQ, uuid);
> sbrec_dns_add_clause_datapaths(, OVSDB_F_INCLUDES, , 1);
> sbrec_ip_multicast_add_clause_datapath(_mcast, OVSDB_F_EQ,
> @@ -360,6 +364,7 @@ out:;
> sb_table_set_req_mon_condition(ovnsb_idl, logical_flow, ),
> sb_table_set_req_mon_condition(ovnsb_idl, logical_dp_group, ),
> sb_table_set_req_mon_condition(ovnsb_idl, mac_binding, ),
> +sb_table_set_req_mon_condition(ovnsb_idl, fdb, ),
> sb_table_set_req_mon_condition(ovnsb_idl, multicast_group, ),
> sb_table_set_req_mon_condition(ovnsb_idl, dns, ),
> sb_table_set_req_mon_condition(ovnsb_idl, controller_event, ),
We're leaking the condition at the end of the function. We're
missing:
ovsdb_idl_condition_destroy();
With that addressed and assuming all the OVN tests pass with
this applied:
Acked-by: Dumitru Ceara
Thanks,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
