Re: [ovs-dev] [PATCH ovn 2/2] ci: Remove the ASAN ARM64 ASAN workaround
On Mon, Oct 16, 2023 at 10:20 PM Mark Michelson wrote: > > Hi Ales, > > The patch makes sense to me. Is there something that guarantees that > clang >= 16 is installed when running CI? Yeah, clang 16 is default on Fedora 38 which we use for ARM64 runs. > > Thanks, > Mark Michelson > > On 10/16/23 04:08, Ales Musil wrote: > > The clang from version 16 and further fixes > > the issue which was causing the slowness. > > Remove the workaround which allows > > the leak sanitizers to run on ARM64 as well. > > > > Signed-off-by: Ales Musil > > --- > > .ci/ci.sh | 5 - > > 1 file changed, 5 deletions(-) > > > > diff --git a/.ci/ci.sh b/.ci/ci.sh > > index 6bb211f2c..282d30b84 100755 > > --- a/.ci/ci.sh > > +++ b/.ci/ci.sh > > @@ -148,11 +148,6 @@ while true; do > > shift > > done > > > > -# Workaround for https://bugzilla.redhat.com/2153359 > > -if [ "$ARCH" = "aarch64" ]; then > > -ASAN_OPTIONS="detect_leaks=0" > > -fi > > - > > if [ -z "$DPDK" ]; then > > mkdir -p "$DPDK_PATH" > > fi > 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] conntrack: Short the scope of ct_lock in new conn setup.
From: wenxu
There is a big scope ct_lock for new conn setup. The
ct_lock should be hold only for conns map insert and
expire rculist insert.
Signed-off-by: wenxu
---
lib/conntrack.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443f..678e23b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -50,6 +50,7 @@ COVERAGE_DEFINE(conntrack_full);
COVERAGE_DEFINE(conntrack_l3csum_err);
COVERAGE_DEFINE(conntrack_l4csum_err);
COVERAGE_DEFINE(conntrack_lookup_natted_miss);
+COVERAGE_DEFINE(conntrack_duplicate);
struct conn_lookup_ctx {
struct conn_key key;
@@ -893,9 +894,9 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
const struct nat_action_info_t *nat_action_info,
const char *helper, const struct alg_exp_node *alg_exp,
enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id)
-OVS_REQUIRES(ct->ct_lock)
{
struct conn *nc = NULL;
+uint32_t rev_hash;
if (!valid_new(pkt, >key)) {
pkt->md.ct_state = CS_INVALID;
@@ -961,17 +962,27 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
}
nat_packet(pkt, nc, false, ctx->icmp_related);
-uint32_t rev_hash = conn_key_hash(_key_node->key,
- ct->hash_basis);
-cmap_insert(>conns, _key_node->cm_node, rev_hash);
+rev_hash = conn_key_hash(_key_node->key, ct->hash_basis);
}
ovs_mutex_init_adaptive(>lock);
atomic_flag_clear(>reclaimed);
fwd_key_node->dir = CT_DIR_FWD;
rev_key_node->dir = CT_DIR_REV;
+ovs_mutex_lock(>ct_lock);
+if (conn_lookup(ct, >key, now, NULL, NULL)) {
+ovs_mutex_unlock(>ct_lock);
+COVERAGE_INC(conntrack_duplicate);
+ovs_mutex_destroy(>lock);
+delete_conn__(nc);
+return NULL;
+}
+if (nat_action_info) {
+cmap_insert(>conns, _key_node->cm_node, rev_hash);
+}
cmap_insert(>conns, _key_node->cm_node, ctx->hash);
conn_expire_push_front(ct, nc);
+ovs_mutex_unlock(>ct_lock);
atomic_count_inc(>n_conn);
ctx->conn = nc; /* For completeness. */
if (zl) {
@@ -1290,12 +1301,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
}
ovs_rwlock_unlock(>resources_lock);
-ovs_mutex_lock(>ct_lock);
-if (!conn_lookup(ct, >key, now, NULL, NULL)) {
-conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
- helper, alg_exp, ct_alg_ctl, tp_id);
-}
-ovs_mutex_unlock(>ct_lock);
+conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
+ helper, alg_exp, ct_alg_ctl, tp_id);
}
write_ct_md(pkt, zone, conn, >key, alg_exp);
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] ci: Remove the ASAN ARM64 ASAN workaround
Hi Ales, The patch makes sense to me. Is there something that guarantees that clang >= 16 is installed when running CI? Thanks, Mark Michelson On 10/16/23 04:08, Ales Musil wrote: The clang from version 16 and further fixes the issue which was causing the slowness. Remove the workaround which allows the leak sanitizers to run on ARM64 as well. Signed-off-by: Ales Musil --- .ci/ci.sh | 5 - 1 file changed, 5 deletions(-) diff --git a/.ci/ci.sh b/.ci/ci.sh index 6bb211f2c..282d30b84 100755 --- a/.ci/ci.sh +++ b/.ci/ci.sh @@ -148,11 +148,6 @@ while true; do shift done -# Workaround for https://bugzilla.redhat.com/2153359 -if [ "$ARCH" = "aarch64" ]; then -ASAN_OPTIONS="detect_leaks=0" -fi - if [ -z "$DPDK" ]; then mkdir -p "$DPDK_PATH" fi ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 6/6] system-tests: Do not use zone 0 for CT limit test.
On 10/10/23 16:12, Ales Musil wrote: > The zone 0 is default system zone, do not use this > zone for the test because it might contain some > entries already which could cause flakiness during > the check. > > Signed-off-by: Ales Musil Hi. While this change makes sense, we're likeley loosing the coverage for zone=0 configuration. Please add some tests for it to the unit-test suite that will ensure that we're parsing commands and interpret them correctly. Bets regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 5/6] ct-dpif: Enforce CT zone limit protection.
On 10/10/23 16:12, Ales Musil wrote:
> Make sure that if any zone limit was set via DB
> all zones are forced to be set there also. This
> is done by tracking which datapath has zone limit
> protection and it is reflected in the dpctl command.
>
> If the datapath is protected the dpctl command will
> return permission error.
>
> Signed-off-by: Ales Musil
> ---
> v4: Rebase on top of current master.
> Make the protection datapath wide.
> ---
> lib/ct-dpif.c | 27 +++
> lib/ct-dpif.h | 2 ++
> lib/dpctl.c| 10 ++
> ofproto/ofproto-dpif.c | 14 ++
> ofproto/ofproto-provider.h | 5 +
> ofproto/ofproto.c | 11 +++
> ofproto/ofproto.h | 2 ++
> tests/system-traffic.at| 36
> vswitchd/bridge.c | 9 +
> 9 files changed, 116 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 686e95c92..a75a8c532 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -23,6 +23,7 @@
> #include "openvswitch/ofp-ct.h"
> #include "openvswitch/ofp-parse.h"
> #include "openvswitch/vlog.h"
> +#include "sset.h"
>
> VLOG_DEFINE_THIS_MODULE(ct_dpif);
>
> @@ -32,6 +33,10 @@ struct flags {
> const char *name;
> };
>
> +/* Protection for CT zone limit per datapath. */
> +static struct sset ct_limit_protection =
> +SSET_INITIALIZER(_limit_protection);
> +
> static void ct_dpif_format_counters(struct ds *,
> const struct ct_dpif_counters *);
> static void ct_dpif_format_timestamp(struct ds *,
> @@ -1064,3 +1069,25 @@ ct_dpif_get_features(struct dpif *dpif, enum
> ct_features *features)
> ? dpif->dpif_class->ct_get_features(dpif, features)
> : EOPNOTSUPP);
> }
> +
> +void
> +ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected)
> +{
> +if (sset_contains(_limit_protection, dpif->full_name) == protected) {
> +return;
> +}
> +
> +if (protected) {
> +sset_add(_limit_protection, dpif->full_name);
> +} else {
> +sset_find_and_delete(_limit_protection, dpif->full_name);
> +}
> +VLOG_INFO("The CT zone limit protection is %s for \"%s\".",
> + protected ? "enabled" : "disabled", dpif->full_name);
> +}
> +
> +bool
> +ct_dpif_is_zone_limit_protected(struct dpif *dpif)
> +{
> +return sset_contains(_limit_protection, dpif->full_name);
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index c90dc9476..feb8b166a 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -352,5 +352,7 @@ int ct_dpif_get_timeout_policy_name(struct dpif *dpif,
> uint32_t tp_id,
> uint16_t dl_type, uint8_t nw_proto,
> char **tp_name, bool *is_generic);
> int ct_dpif_get_features(struct dpif *dpif, enum ct_features *features);
> +void ct_dpif_set_zone_limit_protection(struct dpif *dpif, bool protected);
> +bool ct_dpif_is_zone_limit_protected(struct dpif *dpif);
No need for the 'dpif' variable name.
>
> #endif /* CT_DPIF_H */
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 7113c2c12..3627d37d1 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2234,6 +2234,11 @@ dpctl_ct_set_limits(int argc, const char *argv[],
> ct_dpif_push_zone_limit(_limits, zone, limit, 0);
> }
>
> +if (ct_dpif_is_zone_limit_protected(dpif)) {
> +error = EPERM;
Some meaningful message directing users to use ovs-vsctl instead
might be nice here.
> +goto error;
> +}
> +
> error = ct_dpif_set_limits(dpif, _limits);
> if (!error) {
> ct_dpif_free_zone_limits(_limits);
> @@ -2309,6 +2314,11 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> }
> }
>
> +if (ct_dpif_is_zone_limit_protected(dpif)) {
> +error = EPERM;
And here.
> +goto error;
> +}
> +
> error = ct_dpif_del_limits(dpif, _limits);
> if (!error) {
> goto out;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4fdbf0ef0..4ea70f722 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5665,6 +5665,19 @@ ct_zone_limits_commit(struct dpif_backer *backer)
> }
> }
>
> +static void
> +ct_zone_limit_protection_update(const char *datapath_type, bool protected)
> +{
> +struct dpif_backer *backer = shash_find_data(_dpif_backers,
> + datapath_type);
> +if (!backer) {
> +return;
> +}
> +
> +ct_dpif_set_zone_limit_protection(backer->dpif, protected);
> +}
> +
> +
> static void
> get_datapath_cap(const char *datapath_type, struct smap *cap)
> {
> @@ -6955,4 +6968,5 @@ const struct ofproto_class ofproto_dpif_class = {
> ct_set_zone_timeout_policy,
> ct_del_zone_timeout_policy,
> ct_zone_limit_update,
> +ct_zone_limit_protection_update,
> };
> diff --git
Re: [ovs-dev] [PATCH v4 4/6] vswitchd, ofproto-dpif: Propagate the CT limit from database.
On 10/10/23 16:12, Ales Musil wrote:
> Propagate 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
> ---
> v4: Rebase on top of current master.
> Make sure that the values from DB are propagated only if set. That
> applies to both limit and policies.
> ---
> ofproto/ofproto-dpif.c | 41 ++
> ofproto/ofproto-dpif.h | 5
> ofproto/ofproto-provider.h | 4 +++
> ofproto/ofproto.c | 16 --
> ofproto/ofproto.h | 4 +++
> tests/system-traffic.at| 54 ++
> vswitchd/bridge.c | 60 +++---
> 7 files changed, 171 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ba5706f6a..4fdbf0ef0 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,40 @@ ct_del_zone_timeout_policy(const char *datapath_type,
> uint16_t zone_id)
> }
> }
>
> +BUILD_ASSERT_DECL(OFPROTO_CT_DEFAULT_ZONE_ID == CT_DPIF_DEFAULT_ZONE);
Why this assertion added here?
> +
> +static void
> +ct_zone_limit_update(const char *datapath_type, int32_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, >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 +6954,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..33fb99280 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1921,6 +1921,10 @@ 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);
> +
> +/* Updates the CT zone
Re: [ovs-dev] [PATCH v4 3/6] ovs-vsctl: Add limit to CT zone.
On 10/10/23 16:12, Ales Musil wrote:
> 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
> ---
> v4: Rebase on top of current master.
> Address comments from Ilya:
> - Make sure that the NEWS is clear on what has been added.
> - Make the usage of --may-exist and --if-exists more intuitive for the
> new commands.
> - Some cosmetics.
> Add command and column for default limit.
> ---
> NEWS | 8 ++
> tests/ovs-vsctl.at | 92
> utilities/ovs-vsctl.8.in | 45 --
> utilities/ovs-vsctl.c | 171 -
> vswitchd/vswitch.ovsschema | 14 ++-
> vswitchd/vswitch.xml | 11 +++
> 6 files changed, 331 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index df98e75a0..c0d96b894 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,14 @@ Post-v3.2.0
> - ovs-dpctl:
> * Support removal of default CT zone limit via ovs-dpctl, e.g.
> "ovs-appctl dpctl/ct-del-limits default"
> + - ovs-vsctl:
> + * New commands 'add-zone-limit', 'del-zone-limit' and 'list-zone-limit'
> + to manage the maximum number of connections in conntrack zones via
> + a new 'limit' column in the 'CT_Zone' database table.
> + * New command 'set-zone-default-limit' and 'del-zone-default-limit' to
> + manage the maximum number of connections in conntrack zones that are
> + not explicitly defined otherwise via new 'ct_zone_default_limit'
> column
> + in the 'Datapath' table.
Can we just add a way to specify 'default' instead of 'zone=N'
in the add/del-zone-limit commands? Two separate commands seem
redundant.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index a368bff6e..0d2fa68fb 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -975,6 +975,67 @@ 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([--if-exists 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([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([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([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([set-zone-default-limit netdev limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 5
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-default-limit netdev
> limit=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 10
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-default-limit netdev])])
> +
>
> 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 +1184,37 @@ 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
Re: [ovs-dev] [PATCH v4 2/6] dpctl: Allow the default CT zone limit to de deleted.
On 10/10/23 16:12, Ales Musil wrote:
> Add optional argument to dpctl ct-del-limits called
> "default", which allows to remove the default limit
> making it effectively system default.
>
> Signed-off-by: Ales Musil
> ---
> NEWS| 3 +++
> lib/dpctl.c | 20 ++--
> tests/system-traffic.at | 25 +
> 3 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 6b45492f1..df98e75a0 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,9 @@ 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.
> + - ovs-dpctl:
We should use ovs-appctl as a section or both. We should not really
promote use of ovs-dpctl.
> + * Support removal of default CT zone limit via ovs-dpctl, e.g.
> + "ovs-appctl dpctl/ct-del-limits default"
A few issues with this statement:
1. It is a bit inconsistent - talks about ovs-dpctl and then gives
ovs-appctl example.
2. And we're in the dpctl section, there is no need to repeat the
command name two more times.
3. NEWS entry should generally be phrased as an answer to the
'What changed?' question, i.e. 'Added support ...', 'Command <>
now supports ...', etc.
4. Missing period at the end.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index ad104372e..7113c2c12 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -2291,14 +2291,22 @@ dpctl_ct_del_limits(int argc, const char *argv[],
> int i = dp_arg_exists(argc, argv) ? 2 : 1;
> struct ovs_list zone_limits = OVS_LIST_INITIALIZER(_limits);
>
> -error = opt_dpif_open(argc, argv, dpctl_p, 3, );
> +error = opt_dpif_open(argc, argv, dpctl_p, 4, );
> if (error) {
> return error;
> }
>
> -error = parse_ct_limit_zones(argv[i], _limits, );
> -if (error) {
> -goto error;
> +/* Parse default limit */
Period at the end of a comment.
> +if (!strcmp(argv[i], "default")) {
> +ct_dpif_push_zone_limit(_limits, CT_DPIF_DEFAULT_ZONE, 0, 0);
> +i++;
> +}
> +
> +if (argc > i) {
> +error = parse_ct_limit_zones(argv[i], _limits, );
> +if (error) {
> +goto error;
> +}
> }
>
> error = ct_dpif_del_limits(dpif, _limits);
> @@ -3030,8 +3038,8 @@ static const struct dpctl_command all_commands[] = {
> { "ct-get-tcp-seq-chk", "[dp]", 0, 1, dpctl_ct_get_tcp_seq_chk, DP_RO },
> { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX,
> dpctl_ct_set_limits, DP_RO },
> -{ "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits,
> -DP_RO },
> +{ "ct-del-limits", "[dp] [default] [zone=N1[,N2]...]", 1, 3,
> +dpctl_ct_del_limits, DP_RO },
Hmm, not a problem of this patch, but both set and del commands
seem to incorrectly report themselves as read-only...
> { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits,
> DP_RO },
> { "ct-get-sweep-interval", "[dp]", 0, 1, dpctl_ct_get_sweep, DP_RO },
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 418cd32fe..f35cfaad9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5195,6 +5195,31 @@
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.
>
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> ])
>
> +dnl Test ct-del-limits for default zone.
Maybe add an empty line here, because this comment is not describing
the next immediate block of commands, but applies to all the command
til the end of a test instead.
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15 zone=4,limit=4])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=15
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits default])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=0
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-set-limits default=15])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=15
> +zone=4,limit=4,count=0
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits default zone=4])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits zone=4], [0], [dnl
> +default limit=0
> +zone=4,limit=0,count=0
> +])
> +
> OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> /could not create datapath/d
> /(Cannot allocate memory) on packet/d"])
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/6] ct-dpif: Handle default zone limit as the same way as other limits.
Hi, Ales. Thanks for the new version!
See some comments inline.
Best regards, Ilya Maximets.
On 10/10/23 16:12, Ales Musil wrote:
> Internally handle default CT zone limit as other limits that
> can be passed via the list with special value -1. Curently
* Currently
And there is seem to be an extra 'as' in the patch subject.
> the -1 is treated by both datapaths as default, add static
> asserts to make sure that this remains the case in the future.
> This allows us to easily delete the default zone limit.
>
> Signed-off-by: Ales Musil
> ---
> lib/conntrack.c | 2 +-
> lib/conntrack.h | 4 +++-
> lib/ct-dpif.c | 28 +++-
> lib/ct-dpif.h | 16
> lib/dpctl.c | 14 +++---
> lib/dpif-netdev.c | 17 +
> lib/dpif-netlink.c | 38 +-
> lib/dpif-provider.h | 9 +++--
> 8 files changed, 51 insertions(+), 77 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443fba..31f00a127 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -398,7 +398,7 @@ 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, int32_t zone)
> {
> ovs_mutex_lock(>ct_lock);
> struct zone_limit *zl = zone_limit_lookup_protected(ct, zone);
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 57d5159b6..4c3c4aaf8 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -127,6 +127,8 @@ enum {
> MAX_ZONE = 0x,
> };
>
> +BUILD_ASSERT_DECL(CT_DPIF_DEFAULT_ZONE == DEFAULT_ZONE);
> +
> struct ct_dpif_entry;
> struct ct_dpif_tuple;
>
> @@ -154,6 +156,6 @@ 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_delete(struct conntrack *ct, int32_t zone);
>
> #endif /* conntrack.h */
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index f59c6e560..686e95c92 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -398,23 +398,19 @@ ct_dpif_get_tcp_seq_chk(struct dpif *dpif, bool
> *enabled)
> }
>
> int
> -ct_dpif_set_limits(struct dpif *dpif, const uint32_t *default_limit,
> - const struct ovs_list *zone_limits)
> +ct_dpif_set_limits(struct dpif *dpif, const struct ovs_list *zone_limits)
> {
> return (dpif->dpif_class->ct_set_limits
> -? dpif->dpif_class->ct_set_limits(dpif, default_limit,
> - zone_limits)
> +? dpif->dpif_class->ct_set_limits(dpif, zone_limits)
> : EOPNOTSUPP);
> }
>
> int
> -ct_dpif_get_limits(struct dpif *dpif, uint32_t *default_limit,
> - const struct ovs_list *zone_limits_in,
> +ct_dpif_get_limits(struct dpif *dpif, const struct ovs_list *zone_limits_in,
> struct ovs_list *zone_limits_out)
> {
> return (dpif->dpif_class->ct_get_limits
> -? dpif->dpif_class->ct_get_limits(dpif, default_limit,
> - zone_limits_in,
> +? dpif->dpif_class->ct_get_limits(dpif, zone_limits_in,
>zone_limits_out)
> : EOPNOTSUPP);
> }
> @@ -854,7 +850,7 @@ ct_dpif_format_tcp_stat(struct ds * ds, int tcp_state,
> int conn_per_state)
>
>
> void
> -ct_dpif_push_zone_limit(struct ovs_list *zone_limits, uint16_t zone,
> +ct_dpif_push_zone_limit(struct ovs_list *zone_limits, int32_t zone,
> uint32_t limit, uint32_t count)
> {
> struct ct_dpif_zone_limit *zone_limit = xmalloc(sizeof *zone_limit);
> @@ -928,15 +924,21 @@ error:
> }
>
> void
> -ct_dpif_format_zone_limits(uint32_t default_limit,
> - const struct ovs_list *zone_limits, struct ds *ds)
> +ct_dpif_format_zone_limits(const struct ovs_list *zone_limits, struct ds *ds)
> {
> struct ct_dpif_zone_limit *zone_limit;
>
> -ds_put_format(ds, "default limit=%"PRIu32, default_limit);
> +LIST_FOR_EACH (zone_limit, node, zone_limits) {
> +if (zone_limit->zone == CT_DPIF_DEFAULT_ZONE) {
> +ds_put_format(ds, "default limit=%"PRIu32, zone_limit->limit);
> +}
> +}
>
> LIST_FOR_EACH (zone_limit, node, zone_limits) {
> -ds_put_format(ds, "\nzone=%"PRIu16, zone_limit->zone);
> +if (zone_limit->zone == CT_DPIF_DEFAULT_ZONE) {
> +continue;
> +}
> +ds_put_format(ds, "\nzone=%"PRIu16, (uint16_t) zone_limit->zone);
> ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit);
> ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
> }
> diff --git
Re: [ovs-dev] [PATCH ovn] controller, northd: Wait for cleanup before replying to exit
Hi Ales,
This looks good to me, thanks!
Acked-by: Mark Michelson
On 10/6/23 03:02, Ales Musil wrote:
The unixctl exit command would receive reply immediately
which is confusing and can cause some issues in some tests
if the cleanup takes longer than expected. To avoid that
make sure we reply to the exit command only after the
main cleanup was done so there shouldn't be any possible
window when the services are working when they are no longer
expected to.
Because it is in theory possible that we will receive multiple
exit commands while waiting for the cleanup, make sure that we
will reply to all of them by storing them in array.
At the same time unify the exit structure for both ovn-controller
and northd, so it can be easily extended as needed.
This is inspired by OvS commit that was solving similar issue:
24520a401e06 ("vswitchd: Wait for a bridge exit before replying to exit
unixctl.")
Signed-off-by: Ales Musil
---
controller/ovn-controller.c | 35 ---
lib/ovn-util.c | 31 +++
lib/ovn-util.h | 13 +
northd/ovn-northd.c | 27 ---
4 files changed, 60 insertions(+), 46 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 9a81f1a80..a8c48630f 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -88,7 +88,6 @@
VLOG_DEFINE_THIS_MODULE(main);
-static unixctl_cb_func ovn_controller_exit;
static unixctl_cb_func ct_zone_list;
static unixctl_cb_func extend_table_list;
static unixctl_cb_func inject_pkt;
@@ -4901,11 +4900,6 @@ controller_output_mac_cache_handler(struct engine_node
*node,
return true;
}
-struct ovn_controller_exit_args {
-bool *exiting;
-bool *restart;
-};
-
/* Handles sbrec_chassis changes.
* If a new chassis is added or removed return false, so that
* flows are recomputed. For any updates, there is no need for
@@ -4980,9 +4974,7 @@ int
main(int argc, char *argv[])
{
struct unixctl_server *unixctl;
-bool exiting;
-bool restart;
-struct ovn_controller_exit_args exit_args = {, };
+struct ovn_exit_args exit_args = {};
int retval;
/* Read from system-id-override file once on startup. */
@@ -5002,7 +4994,7 @@ main(int argc, char *argv[])
if (retval) {
exit(EXIT_FAILURE);
}
-unixctl_command_register("exit", "", 0, 1, ovn_controller_exit,
+unixctl_command_register("exit", "", 0, 1, ovn_exit_command_callback,
_args);
daemonize_complete();
@@ -5514,10 +5506,8 @@ main(int argc, char *argv[])
VLOG_INFO("OVN internal version is : [%s]", ovn_version);
/* Main loop. */
-exiting = false;
-restart = false;
bool sb_monitor_all = false;
-while (!exiting) {
+while (!exit_args.exiting) {
memory_run();
if (memory_should_report()) {
struct simap usage = SIMAP_INITIALIZER();
@@ -5954,7 +5944,7 @@ main(int argc, char *argv[])
unixctl_server_run(unixctl);
unixctl_server_wait(unixctl);
-if (exiting || pending_pkt.conn) {
+if (exit_args.exiting || pending_pkt.conn) {
poll_immediate_wake();
}
@@ -6005,7 +5995,7 @@ loop_done:
memory_wait();
poll_block();
if (should_service_stop()) {
-exiting = true;
+exit_args.exiting = true;
}
}
@@ -6013,7 +6003,7 @@ loop_done:
engine_cleanup();
/* It's time to exit. Clean up the databases if we are not restarting */
-if (!restart) {
+if (!exit_args.restart) {
bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl);
while (!done) {
update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl,
@@ -6065,7 +6055,6 @@ loop_done:
}
free(ovn_version);
-unixctl_server_destroy(unixctl);
lflow_destroy();
ofctrl_destroy();
pinctrl_destroy();
@@ -6090,6 +6079,8 @@ loop_done:
if (cli_system_id) {
free(cli_system_id);
}
+ovn_exit_args_finish(_args);
+unixctl_server_destroy(unixctl);
service_stop();
ovsrcu_exit();
@@ -6213,16 +6204,6 @@ usage(void)
exit(EXIT_SUCCESS);
}
-static void
-ovn_controller_exit(struct unixctl_conn *conn, int argc,
- const char *argv[], void *exit_args_)
-{
-struct ovn_controller_exit_args *exit_args = exit_args_;
-*exit_args->exiting = true;
-*exit_args->restart = argc == 2 && !strcmp(argv[1], "--restart");
-unixctl_command_reply(conn, NULL);
-}
-
static void
ct_zone_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED, void *ct_zones_)
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index ffe295696..33105202f 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -1302,3 +1302,34 @@
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On Mon, Oct 16, 2023 at 4:08 AM Roi Dayan via dev
wrote:
>
>
> On 16/10/2023 11:00, Roi Dayan wrote:
> >
> > On 16/10/2023 10:42, Eelco Chaudron wrote:
> >>
> >>
> >> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> >>
> >>> On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
> modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
> struct flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
> !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
> >>>
> >>>
> >>> Hi,
> >>>
> >>> I would like to consult another related issue to the original commit.
> >>>
> >>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> >>>
> >>> Now with geneve options the redundant mirror is removed but if there will
> >>> be
> >>> a real modification there will still be a mirror output but in an
> >>> incorrect place.
> >>>
> >>> For example adding dec_ttl, the action list will be like this:
> >>>
> >>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
> >>>
> >>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
> >>> the tunnel header already.
> >>>
> >>> When not using tunnels the mirror is fine and the action list will look
> >>> like this:
> >>>
> >>> actions:port1,dec_ttl,port2,port1
> >>>
> >>> So with tunnel the second mirror shouldn't have been somehow with the
> >>> dec_ttl action but without the tunnel header?
> >>>
> >>> Should the actions list somehow be like this
> >>>
> >>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
> >>
> >> Not sure I follow you, but do you think the dect ttl and set tunnel should
> >> have been swapped? I guess this would depend on your OpenFlow rule. Can
> >> you show an ofproto trace on your example, which might help clarify OVS’s
> >> reasoning?
> >>
> >> //Eelco
> >>
> >
> > yes. but not just dec_ttl. any header modification beside encap.
> > after all original commit explains users could reverse the packet with
> > rules so
> > there won't be a real packet coming so add mirror after modification.
> > but mirror is usually before encap or on recv after decap.
> >
> > I expected action list to be, maybe: mirrorPort, header mod like replace
> > src/dst and ping type, mirrorPort, encap, tunnelPort.
> >
> > i'll check about the ofproto trace
> >
> >>
> >>>
> >>> Am I looking at this wrong? What do you think?
> >>>
> >>> Thanks,
> >>> Roi
> >>
> >
> >
>
>
> # ovs-ofctl dump-flows br-ovs
> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
> actions=NORMAL
> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port=geneve1
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port="enp8s0f0_0"
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
> priority=0 actions=NORMAL
>
>
>
> # ovs-appctl ofproto/trace br-ovs
> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
> Flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>
> bridge("br-ovs")
>
> 0. ip,in_port=1, priority 32768
> set_field:0x1234->tun_metadata0
> dec_ttl
> output:3
> -> output to kernel tunnel
>
> Final flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
> Datapath actions:
>
Re: [ovs-dev] OVN weekly community meeting
On Thu, Oct 5, 2023 at 11:31 PM Han Zhou wrote: > > > > On Thu, Oct 5, 2023 at 11:23 PM Frode Nordahl wrote: > > > > On Thu, Oct 5, 2023 at 9:20 PM Ilya Maximets wrote: > > > > > > On 10/5/23 19:31, Dumitru Ceara wrote: > > > > Hi all, > > > > > > > > On 9/22/23 09:19, Frode Nordahl wrote: > > > >> Hello Dumitru and Team, > > > >> > > > >> On Thu, Sep 21, 2023 at 5:25 PM Dumitru Ceara wrote: > > > >>> > > > >>> Hi all, > > > >>> > > > >>> As you probably know and as the ovn.org website mentions [0] we have > > > >>> weekly OVN IRC meetings, in #openvswitch on irc.libera.chat (the time is > > > >>> slightly wrong but that's a different issue [1]). > > > >>> > > > >>> In my opinion these meetings are useful because we (well, anyone > > > >>> interested) can share status updates about what they currently work on > > > >>> in the OVN world. > > > >> > > > >> The weekly IRC meetings are indeed valuable, they are very useful for > > > >> coordinating ongoing community activities and highlighting/discussing > > > >> any issues that are not obvious across company barriers on a day to > > > >> day basis. > > > >> > > > >>> I wonder however if it would make sense to have an alternative technical > > > >>> meeting (it doesn't necessarily have to replace the weekly IRC one but > > > >>> it may) where slightly more in depth technical discussion can happen. > > > >>> I'm thinking of this as an optional first step that might happen before > > > >>> posting the first (RFC) version of a feature, for example. Or as a > > > >>> place where objectives for a new LTS can be defined from a community > > > >>> perspective. > > > >>> > > > >>> At the same time I wonder if such a meeting would be more effective if a > > > >>> different communication medium would be used (e.g., an online video > > > >>> conference system). > > > >>> > > > >>> In short, would a monthly (audio/video) technical community meeting > > > >>> sound like an interesting idea to you? > > > >> > > > >> Thank you for bringing this up. My immediate reaction is positive, an > > > >> audio/video meeting would carry more bandwidth for exchanging thoughts > > > >> and ideas on any topic discussed. If we manage to create a safe space > > > >> for bouncing not necessarily fully thought through ideas off each > > > >> other, it could be quite powerful. > > > >> > > > >> I'm wondering a bit about the cadence though, IIUC the examples of > > > >> potential topics to discuss fall into the planning ahead category. > > > >> Would it perhaps make sense to have these meetings before the start of > > > >> each development cycle, i.e. a set of meetings every 6 months or so, > > > >> perhaps with touch points in between? > > > >> > > > > > > > > I'm going to try to summarize the points/concerns that were raised > > > > so far in this email thread and during the IRC meetings. Please > > > > do correct me if I missed or mis-interpreted something. > > > > > > > > - there seems to be positive reaction from the community to the > > > > proposal of having an occasional A/V technical community > > > > meeting. > > > > > > > > - there's a request to not replace the IRC weekly (that should > > > > stay weekly and be used for informal status updates). > > > > > > > > - the cadence of this new community meeting is not yet agreed > > > > upon: > > > > - there were a few suggestions to have a monthly meeting > > > > with a pre-defined agenda and with the option of > > > > cancelling the meeting if there's not enough to talk > > > > about. > > > > - an alternative would be to have these technical meetings > > > > at the beginning of each development cycle (every ~6 months) > > > > with an option of ad-hoc touching points in between. > > > > > > > > - there was a suggestion of recording the meetings and posting > > > > the recordings to ovn.org. Personally I'm not sure I'd favor > > > > that; the reason being that I think it might cause people to > > > > be more reluctant to join. I'm OK either way though. Related > > > > to this point, there was a suggestion to take and share > > > > meeting notes. > > > > > > > > - we briefly discussed about the medium to use and, at least > > > > during the IRC meeting, it seemed like Google Meet would > > > > be an OK option for now. > > > > Thank you for summarizing, your account above resonates with my > > perspective on the discussions so far as well. > > > > > > With all this in mind and (luckily) because we're at the > > > > beginning of the 24.03 development cycle, my suggestion > > > > would be to schedule a first community A/V technical meeting > > > > with two agenda items: > > > > > > > > 1. What new features or relevant changes does the community > > > >think are important to have in the next release (24.03.0)? > > > > 2. Decide on a cadence for the technical meetings. > > > > > > > > Here are some proposals for dates and times for a first > > > > meeting. Please reply with a list of
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 14:02, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 12:52, Roi Dayan wrote:
>
>> On 16/10/2023 11:31, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>>>
On 16/10/2023 11:00, Roi Dayan wrote:
>
> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>
>>> On 09/10/2023 15:05, Roi Dayan wrote:
The cited commit fixed missing mirror packets by reset mirror when
packets are modified but setting geneve options was also treated as
a modified packet but should be treated as a part of set_tunnel
which doesn't reset mirror.
Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
modified.")
Signed-off-by: Roi Dayan
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
Notes:
v2:
- user correct sha in fixes line.
ofproto/ofproto-dpif-xlate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c
b/ofproto/ofproto-dpif-xlate.c
index be4bd6657688..e243773307b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
struct flow *flow,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
-if (mf_are_prereqs_ok(mf, flow, NULL)) {
+if (mf_are_prereqs_ok(mf, flow, NULL) &&
!mf_is_tun_metadata(mf)) {
ctx->mirrors = 0;
}
return;
>>>
>>>
>>> Hi,
>>>
>>> I would like to consult another related issue to the original commit.
>>>
>>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>>
>>> Now with geneve options the redundant mirror is removed but if there
>>> will be
>>> a real modification there will still be a mirror output but in an
>>> incorrect place.
>>>
>>> For example adding dec_ttl, the action list will be like this:
>>>
>>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>>
>>> A mirror output enp8s0f0_1 added at the end but the second mirror is
>>> with the tunnel header already.
>>>
>>> When not using tunnels the mirror is fine and the action list will look
>>> like this:
>>>
>>> actions:port1,dec_ttl,port2,port1
>>>
>>> So with tunnel the second mirror shouldn't have been somehow with the
>>> dec_ttl action but without the tunnel header?
>>>
>>> Should the actions list somehow be like this
>>>
>>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>
>> Not sure I follow you, but do you think the dect ttl and set tunnel
>> should have been swapped? I guess this would depend on your OpenFlow
>> rule. Can you show an ofproto trace on your example, which might help
>> clarify OVS’s reasoning?
>>
>> //Eelco
>>
>
> yes. but not just dec_ttl. any header modification beside encap.
> after all original commit explains users could reverse the packet with
> rules so
> there won't be a real packet coming so add mirror after modification.
> but mirror is usually before encap or on recv after decap.
>
> I expected action list to be, maybe: mirrorPort, header mod like replace
> src/dst and ping type, mirrorPort, encap, tunnelPort.
>
> i'll check about the ofproto trace
>
>>
>>>
>>> Am I looking at this wrong? What do you think?
>>>
>>> Thanks,
>>> Roi
>>
>
>
# ovs-ofctl dump-flows br-ovs
cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
actions=NORMAL
cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
ip,in_port=geneve1
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
ip,in_port="enp8s0f0_0"
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
priority=0 actions=NORMAL
# ovs-appctl ofproto/trace br-ovs
in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
Flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
bridge("br-ovs")
0. ip,in_port=1, priority 32768
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Pause revalidators when purging.
On 16 Oct 2023, at 14:24, Eelco Chaudron wrote: > On 16 Oct 2023, at 14:22, David Marchand wrote: > >> On Fri, Oct 13, 2023 at 11:49 AM Eelco Chaudron wrote: The check-offloads target can run fine if removing the exception on "failed to flow_get" and "failed to acquire ukey" warning logs. >>> >>> Would it be good to add another patch in the series, to remove these? >>> >> >> Yes, I intend to send a followup patch if this patch gets merged. > > Thanks! Just to clarify this. I did run the specific offload test with the below diff 100x and it did not show the errors it was previously excluding. Cheers, Eelco diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 7215e36e2..cf2a3c2e3 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -799,8 +799,6 @@ AT_CHECK([test $(ovs-appctl dpctl/dump-flows | grep -c "eth_type(0x0800)") -eq 0 OVS_TRAFFIC_VSWITCHD_STOP(["/could not open network device ovs-p0/d /on nonexistent port/d -/failed to flow_get/d -/Failed to acquire udpif_key/d /No such device/d /failed to offload flow/d "]) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Pause revalidators when purging.
On 16 Oct 2023, at 14:22, David Marchand wrote: > On Fri, Oct 13, 2023 at 11:49 AM Eelco Chaudron wrote: >>> The check-offloads target can run fine if removing the exception on >>> "failed to flow_get" and "failed to acquire ukey" warning logs. >> >> Would it be good to add another patch in the series, to remove these? >> > > Yes, I intend to send a followup patch if this patch gets merged. Thanks! //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall: Pause revalidators when purging.
On Fri, Oct 13, 2023 at 11:49 AM Eelco Chaudron wrote: > > The check-offloads target can run fine if removing the exception on > > "failed to flow_get" and "failed to acquire ukey" warning logs. > > Would it be good to add another patch in the series, to remove these? > Yes, I intend to send a followup patch if this patch gets merged. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 12:52, Roi Dayan wrote:
> On 16/10/2023 11:31, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>>
>>> On 16/10/2023 11:00, Roi Dayan wrote:
On 16/10/2023 10:42, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>
>> On 09/10/2023 15:05, Roi Dayan wrote:
>>> The cited commit fixed missing mirror packets by reset mirror when
>>> packets are modified but setting geneve options was also treated as
>>> a modified packet but should be treated as a part of set_tunnel
>>> which doesn't reset mirror.
>>>
>>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>>> modified.")
>>> Signed-off-by: Roi Dayan
>>> Acked-by: Simon Horman
>>> Acked-by: Eelco Chaudron
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - user correct sha in fixes line.
>>>
>>> ofproto/ofproto-dpif-xlate.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index be4bd6657688..e243773307b7 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
>>> struct flow *flow,
>>>
>>> set_field = ofpact_get_SET_FIELD(a);
>>> mf = set_field->field;
>>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>>> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
>>> !mf_is_tun_metadata(mf)) {
>>> ctx->mirrors = 0;
>>> }
>>> return;
>>
>>
>> Hi,
>>
>> I would like to consult another related issue to the original commit.
>>
>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>
>> Now with geneve options the redundant mirror is removed but if there
>> will be
>> a real modification there will still be a mirror output but in an
>> incorrect place.
>>
>> For example adding dec_ttl, the action list will be like this:
>>
>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>
>> A mirror output enp8s0f0_1 added at the end but the second mirror is
>> with the tunnel header already.
>>
>> When not using tunnels the mirror is fine and the action list will look
>> like this:
>>
>> actions:port1,dec_ttl,port2,port1
>>
>> So with tunnel the second mirror shouldn't have been somehow with the
>> dec_ttl action but without the tunnel header?
>>
>> Should the actions list somehow be like this
>>
>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>
> Not sure I follow you, but do you think the dect ttl and set tunnel
> should have been swapped? I guess this would depend on your OpenFlow
> rule. Can you show an ofproto trace on your example, which might help
> clarify OVS’s reasoning?
>
> //Eelco
>
yes. but not just dec_ttl. any header modification beside encap.
after all original commit explains users could reverse the packet with
rules so
there won't be a real packet coming so add mirror after modification.
but mirror is usually before encap or on recv after decap.
I expected action list to be, maybe: mirrorPort, header mod like replace
src/dst and ping type, mirrorPort, encap, tunnelPort.
i'll check about the ofproto trace
>
>>
>> Am I looking at this wrong? What do you think?
>>
>> Thanks,
>> Roi
>
>>>
>>>
>>> # ovs-ofctl dump-flows br-ovs
>>> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
>>> actions=NORMAL
>>> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
>>> ip,in_port=geneve1
>>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
>>> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
>>> ip,in_port="enp8s0f0_0"
>>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
>>> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
>>> priority=0 actions=NORMAL
>>>
>>>
>>>
>>> # ovs-appctl ofproto/trace br-ovs
>>> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
>>> Flow:
>>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>>>
>>> bridge("br-ovs")
>>>
>>> 0. ip,in_port=1, priority 32768
>>> set_field:0x1234->tun_metadata0
>>> dec_ttl
>>> output:3
>>> -> output to kernel tunnel
>>>
>>> Final flow:
>>>
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 11:31, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 10:07, Roi Dayan wrote:
>
>> On 16/10/2023 11:00, Roi Dayan wrote:
>>>
>>> On 16/10/2023 10:42, Eelco Chaudron wrote:
On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote:
>> The cited commit fixed missing mirror packets by reset mirror when
>> packets are modified but setting geneve options was also treated as
>> a modified packet but should be treated as a part of set_tunnel
>> which doesn't reset mirror.
>>
>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>> modified.")
>> Signed-off-by: Roi Dayan
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
>> ---
>>
>> Notes:
>> v2:
>> - user correct sha in fixes line.
>>
>> ofproto/ofproto-dpif-xlate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index be4bd6657688..e243773307b7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
>> struct flow *flow,
>>
>> set_field = ofpact_get_SET_FIELD(a);
>> mf = set_field->field;
>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
>> !mf_is_tun_metadata(mf)) {
>> ctx->mirrors = 0;
>> }
>> return;
>
>
> Hi,
>
> I would like to consult another related issue to the original commit.
>
> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>
> Now with geneve options the redundant mirror is removed but if there will
> be
> a real modification there will still be a mirror output but in an
> incorrect place.
>
> For example adding dec_ttl, the action list will be like this:
>
> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>
> A mirror output enp8s0f0_1 added at the end but the second mirror is with
> the tunnel header already.
>
> When not using tunnels the mirror is fine and the action list will look
> like this:
>
> actions:port1,dec_ttl,port2,port1
>
> So with tunnel the second mirror shouldn't have been somehow with the
> dec_ttl action but without the tunnel header?
>
> Should the actions list somehow be like this
>
> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Not sure I follow you, but do you think the dect ttl and set tunnel should
have been swapped? I guess this would depend on your OpenFlow rule. Can
you show an ofproto trace on your example, which might help clarify OVS’s
reasoning?
//Eelco
>>>
>>> yes. but not just dec_ttl. any header modification beside encap.
>>> after all original commit explains users could reverse the packet with
>>> rules so
>>> there won't be a real packet coming so add mirror after modification.
>>> but mirror is usually before encap or on recv after decap.
>>>
>>> I expected action list to be, maybe: mirrorPort, header mod like replace
>>> src/dst and ping type, mirrorPort, encap, tunnelPort.
>>>
>>> i'll check about the ofproto trace
>>>
>
> Am I looking at this wrong? What do you think?
>
> Thanks,
> Roi
>>>
>>>
>>
>>
>> # ovs-ofctl dump-flows br-ovs
>> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
>> actions=NORMAL
>> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port=geneve1
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
>> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
>> ip,in_port="enp8s0f0_0"
>> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
>> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
>> priority=0 actions=NORMAL
>>
>>
>>
>> # ovs-appctl ofproto/trace br-ovs
>> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
>> Flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>>
>> bridge("br-ovs")
>>
>> 0. ip,in_port=1, priority 32768
>> set_field:0x1234->tun_metadata0
>> dec_ttl
>> output:3
>> -> output to kernel tunnel
>>
>> Final flow:
>> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>> Megaflow:
Re: [ovs-dev] [PATCH v2 2/2] net: openvswitch: Annotate struct mask_array with __counted_by
On Sat, 14 Oct 2023, Kees Cook wrote:
> On Sat, Oct 14, 2023 at 08:34:53AM +0200, 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
> > ---
> > v2: Fix the subject [Ilya Maximets]
> > fix the field name used with __counted_by [Ilya Maximets]
> >
> > v1:
> > https://lore.kernel.org/all/f66ddcf1ef9328f10292ea75a17b584359b6cde3.1696156198.git.christophe.jail...@wanadoo.fr/
> >
> >
> > 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].
What was the problem with the semantic patch in this case?
thanks,
julia
> >
> > 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..f524dc3e4862 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(max);
> > };
>
> Yup, this looks correct to me. Thanks!
>
> Reviewed-by: Kees Cook
>
> --
> Kees Cook
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/2] ci: Use proper uname argument to get the HW type
On Mon, Oct 16, 2023 at 10:36 AM Simon Horman wrote:
> On Mon, Oct 16, 2023 at 10:08:19AM +0200, Ales Musil wrote:
> > The -i option is not portable and doesn't work
> > on all platforms. Use -m instead.
>
> This seems pretty broken.
> Is it actually used anywhere?
>
It was used only for the check on ARM64 and for that it works fine.
>
> Also, perhaps a fixes tag is appropriate here.
>
Makes sense I'll fix that in v2.
>
> > Signed-off-by: Ales Musil
> > ---
> > .ci/ci.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/.ci/ci.sh b/.ci/ci.sh
> > index 10f11939c..6bb211f2c 100755
> > --- a/.ci/ci.sh
> > +++ b/.ci/ci.sh
> > @@ -23,7 +23,7 @@ CONTAINER_WORKDIR="/workspace/ovn-tmp"
> > IMAGE_NAME=${IMAGE_NAME:-"ovn-org/ovn-tests"}
> >
> > # Test variables
> > -ARCH=${ARCH:-$(uname -i)}
> > +ARCH=${ARCH:-$(uname -m)}
> > CC=${CC:-gcc}
> >
> >
> > --
> > 2.41.0
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
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
Re: [ovs-dev] [PATCH v4 0/6] Expose CT limit via DB
On Tue, Oct 10, 2023 at 04:12:18PM +0200, Ales Musil wrote: > 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 datapath is protected. > The dpctl will return an error if the limit is > already set in DB for that datapath. > > Ales Musil (6): > ct-dpif: Handle default zone limit as the same way as other limits. > dpctl: Allow the default CT zone limit to de deleted. > ovs-vsctl: Add limit to CT zone. > vswitchd, ofproto-dpif: Propagate the CT limit from database. > ct-dpif: Enforce CT zone limit protection. > system-tests: Do not use zone 0 for CT limit test. For series, Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/2] ci: Use proper uname argument to get the HW type
On Mon, Oct 16, 2023 at 10:08:19AM +0200, Ales Musil wrote:
> The -i option is not portable and doesn't work
> on all platforms. Use -m instead.
This seems pretty broken.
Is it actually used anywhere?
Also, perhaps a fixes tag is appropriate here.
> Signed-off-by: Ales Musil
> ---
> .ci/ci.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.ci/ci.sh b/.ci/ci.sh
> index 10f11939c..6bb211f2c 100755
> --- a/.ci/ci.sh
> +++ b/.ci/ci.sh
> @@ -23,7 +23,7 @@ CONTAINER_WORKDIR="/workspace/ovn-tmp"
> IMAGE_NAME=${IMAGE_NAME:-"ovn-org/ovn-tests"}
>
> # Test variables
> -ARCH=${ARCH:-$(uname -i)}
> +ARCH=${ARCH:-$(uname -m)}
> CC=${CC:-gcc}
>
>
> --
> 2.41.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 10:07, Roi Dayan wrote:
> On 16/10/2023 11:00, Roi Dayan wrote:
>>
>> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>>
>>>
>>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>>
On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
> modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const
> struct flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) &&
> !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
Hi,
I would like to consult another related issue to the original commit.
feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
Now with geneve options the redundant mirror is removed but if there will
be
a real modification there will still be a mirror output but in an
incorrect place.
For example adding dec_ttl, the action list will be like this:
actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
A mirror output enp8s0f0_1 added at the end but the second mirror is with
the tunnel header already.
When not using tunnels the mirror is fine and the action list will look
like this:
actions:port1,dec_ttl,port2,port1
So with tunnel the second mirror shouldn't have been somehow with the
dec_ttl action but without the tunnel header?
Should the actions list somehow be like this
actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>>
>>> Not sure I follow you, but do you think the dect ttl and set tunnel should
>>> have been swapped? I guess this would depend on your OpenFlow rule. Can you
>>> show an ofproto trace on your example, which might help clarify OVS’s
>>> reasoning?
>>>
>>> //Eelco
>>>
>>
>> yes. but not just dec_ttl. any header modification beside encap.
>> after all original commit explains users could reverse the packet with rules
>> so
>> there won't be a real packet coming so add mirror after modification.
>> but mirror is usually before encap or on recv after decap.
>>
>> I expected action list to be, maybe: mirrorPort, header mod like replace
>> src/dst and ping type, mirrorPort, encap, tunnelPort.
>>
>> i'll check about the ofproto trace
>>
>>>
Am I looking at this wrong? What do you think?
Thanks,
Roi
>>>
>>
>>
>
>
> # ovs-ofctl dump-flows br-ovs
> cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
> actions=NORMAL
> cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port=geneve1
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
> cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
> ip,in_port="enp8s0f0_0"
> actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
> cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846,
> priority=0 actions=NORMAL
>
>
>
> # ovs-appctl ofproto/trace br-ovs
> in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
> Flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
>
> bridge("br-ovs")
>
> 0. ip,in_port=1, priority 32768
> set_field:0x1234->tun_metadata0
> dec_ttl
> output:3
> -> output to kernel tunnel
>
> Final flow:
> tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
> Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
> Datapath actions:
> 3,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),6,3
Re: [ovs-dev] [PATCH v7 2/3] dpif: Probe support for OVS_ACTION_ATTR_DROP.
On Thu, Sep 28, 2023 at 01:50:43PM +0200, Eelco Chaudron wrote: > > > On 25 Sep 2023, at 20:04, Eric Garver wrote: > > > Kernel support has been added for this action. As such, we need to probe > > the datapath for support. > > > > Signed-off-by: Eric Garver > > This patch looks fine to me, and I’ll ack it. However, we should not apply > this series until we have a final version of Ilya’s RFC patch applied. > > This is the patch I’m referring to: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20230908234009.892803-1-i.maxim...@ovn.org/ Hi Eric, With the above in mind I am marking this series as Deferred in patchwork. Please repost the patch once the dependencies are in place, or when otherwise you think the time is right. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] flow: fix sanity check for unexpected ip header length field
On Wed, Mar 15, 2023 at 05:11:01PM +0800, Faicker Mo wrote: > Derivation cases of CVE-2020-35498: > 1. invalid ipv4 header total-length field > 2. invalid ipv6 header payload-length field > These may cause unwanted flow to send to datapath. > > > Signed-off-by: Faicker Mo Hi Faiker, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-lib: accept optional timeout value for upgrade_cluster
On Thu, Dec 15, 2022 at 11:57:27AM -0600, Dan Williams wrote: > Signed-off-by: Dan Williams Hi Dan, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 1/1] datapath-windows: Fix first tunnel packet drop before MAC learned.
On Mon, Aug 22, 2022 at 05:00:05PM +0800, Frank Guo wrote: > From: Frank Guo > > when traffic is sent through tunnel, it will drop untile MAC is learned. > > I tested with installingw ovsext driver, normally after driver installed and > all is setup, the first ping packet will drop, with this patch, the first > ping packet is OK. > > Reported-at:openvswitch/ovs-issues#253 > Signed-off-by: Frank Guo Hi Frank, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] conntrack: narrow the scope of ct_lock in new conn setup
On Tue, Aug 02, 2022 at 12:01:07AM -0400, we...@chinatelecom.cn wrote: > From: wenxu > > There is a big scope ct_lock for new conn setup. The > ct_lock should be hold only for conns map insert and > expire rculist insert. > > Signed-off-by: wenxu Hi Wenxu, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] faq: Fix features not available in userspace datapath.
On Wed, Apr 27, 2022 at 11:24:11AM +, Cian Ferriter wrote:
> Tunnel virtual ports are supported in the userspace datapath. Update the
> answer to reflect this.
>
> Fixes: a36de779d739 ("openvswitch: Userspace tunneling.")
> Signed-off-by: Cian Ferriter
Hi Cian,
This patch appears to have gone stale in patchwork, for one reason or
another. If it is still relevant then I think it needs to be revisited,
by being reposted after appropriate preparation.
As such I'm marking this patch as "Deferred" in patchwork.
No action is required unless there is a desire to revisit this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 2/2] ci: Remove the ASAN ARM64 ASAN workaround
The clang from version 16 and further fixes the issue which was causing the slowness. Remove the workaround which allows the leak sanitizers to run on ARM64 as well. Signed-off-by: Ales Musil --- .ci/ci.sh | 5 - 1 file changed, 5 deletions(-) diff --git a/.ci/ci.sh b/.ci/ci.sh index 6bb211f2c..282d30b84 100755 --- a/.ci/ci.sh +++ b/.ci/ci.sh @@ -148,11 +148,6 @@ while true; do shift done -# Workaround for https://bugzilla.redhat.com/2153359 -if [ "$ARCH" = "aarch64" ]; then -ASAN_OPTIONS="detect_leaks=0" -fi - if [ -z "$DPDK" ]; then mkdir -p "$DPDK_PATH" fi -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/2] ci: Use proper uname argument to get the HW type
The -i option is not portable and doesn't work
on all platforms. Use -m instead.
Signed-off-by: Ales Musil
---
.ci/ci.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.ci/ci.sh b/.ci/ci.sh
index 10f11939c..6bb211f2c 100755
--- a/.ci/ci.sh
+++ b/.ci/ci.sh
@@ -23,7 +23,7 @@ CONTAINER_WORKDIR="/workspace/ovn-tmp"
IMAGE_NAME=${IMAGE_NAME:-"ovn-org/ovn-tests"}
# Test variables
-ARCH=${ARCH:-$(uname -i)}
+ARCH=${ARCH:-$(uname -m)}
CC=${CC:-gcc}
--
2.41.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 11:00, Roi Dayan wrote:
>
> On 16/10/2023 10:42, Eelco Chaudron wrote:
>>
>>
>> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>>
>>> On 09/10/2023 15:05, Roi Dayan wrote:
The cited commit fixed missing mirror packets by reset mirror when
packets are modified but setting geneve options was also treated as
a modified packet but should be treated as a part of set_tunnel
which doesn't reset mirror.
Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
modified.")
Signed-off-by: Roi Dayan
Acked-by: Simon Horman
Acked-by: Eelco Chaudron
---
Notes:
v2:
- user correct sha in fixes line.
ofproto/ofproto-dpif-xlate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index be4bd6657688..e243773307b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
flow *flow,
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
-if (mf_are_prereqs_ok(mf, flow, NULL)) {
+if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf))
{
ctx->mirrors = 0;
}
return;
>>>
>>>
>>> Hi,
>>>
>>> I would like to consult another related issue to the original commit.
>>>
>>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>>
>>> Now with geneve options the redundant mirror is removed but if there will be
>>> a real modification there will still be a mirror output but in an incorrect
>>> place.
>>>
>>> For example adding dec_ttl, the action list will be like this:
>>>
>>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>>
>>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
>>> the tunnel header already.
>>>
>>> When not using tunnels the mirror is fine and the action list will look
>>> like this:
>>>
>>> actions:port1,dec_ttl,port2,port1
>>>
>>> So with tunnel the second mirror shouldn't have been somehow with the
>>> dec_ttl action but without the tunnel header?
>>>
>>> Should the actions list somehow be like this
>>>
>>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>>
>> Not sure I follow you, but do you think the dect ttl and set tunnel should
>> have been swapped? I guess this would depend on your OpenFlow rule. Can you
>> show an ofproto trace on your example, which might help clarify OVS’s
>> reasoning?
>>
>> //Eelco
>>
>
> yes. but not just dec_ttl. any header modification beside encap.
> after all original commit explains users could reverse the packet with rules
> so
> there won't be a real packet coming so add mirror after modification.
> but mirror is usually before encap or on recv after decap.
>
> I expected action list to be, maybe: mirrorPort, header mod like replace
> src/dst and ping type, mirrorPort, encap, tunnelPort.
>
> i'll check about the ofproto trace
>
>>
>>>
>>> Am I looking at this wrong? What do you think?
>>>
>>> Thanks,
>>> Roi
>>
>
>
# ovs-ofctl dump-flows br-ovs
cookie=0x0, duration=317.989s, table=0, n_packets=8, n_bytes=376, arp
actions=NORMAL
cookie=0x0, duration=317.982s, table=0, n_packets=21, n_bytes=2058,
ip,in_port=geneve1
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:"enp8s0f0_0"
cookie=0x0, duration=317.975s, table=0, n_packets=21, n_bytes=2058,
ip,in_port="enp8s0f0_0"
actions=set_field:0x1234->tun_metadata0,dec_ttl,output:geneve1
cookie=0x0, duration=318.117s, table=0, n_packets=11, n_bytes=846, priority=0
actions=NORMAL
# ovs-appctl ofproto/trace br-ovs
in_port=enp8s0f0_0,tcp,nw_ttl=64,nw_src=1.1.1.7,tcp_dst=22
Flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
bridge("br-ovs")
0. ip,in_port=1, priority 32768
set_field:0x1234->tun_metadata0
dec_ttl
output:3
-> output to kernel tunnel
Final flow:
tcp,in_port=1,vlan_tci=0x,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:00:00,nw_src=1.1.1.7,nw_dst=0.0.0.0,nw_tos=0,nw_ecn=0,nw_ttl=63,nw_frag=no,tp_src=0,tp_dst=22,tcp_flags=0
Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_ecn=0,nw_ttl=64,nw_frag=no
Datapath actions:
3,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),6,3
so mirror port 3 is mirroring the tx packet and then after tunnel and dec ttl
the final packet
but we get packet with the encap header.
If we look in the original commit purpose is to have a mirror in
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16/10/2023 10:42, Eelco Chaudron wrote:
>
>
> On 16 Oct 2023, at 9:09, Roi Dayan wrote:
>
>> On 09/10/2023 15:05, Roi Dayan wrote:
>>> The cited commit fixed missing mirror packets by reset mirror when
>>> packets are modified but setting geneve options was also treated as
>>> a modified packet but should be treated as a part of set_tunnel
>>> which doesn't reset mirror.
>>>
>>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>>> modified.")
>>> Signed-off-by: Roi Dayan
>>> Acked-by: Simon Horman
>>> Acked-by: Eelco Chaudron
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - user correct sha in fixes line.
>>>
>>> ofproto/ofproto-dpif-xlate.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index be4bd6657688..e243773307b7 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
>>> flow *flow,
>>>
>>> set_field = ofpact_get_SET_FIELD(a);
>>> mf = set_field->field;
>>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>>> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
>>> ctx->mirrors = 0;
>>> }
>>> return;
>>
>>
>> Hi,
>>
>> I would like to consult another related issue to the original commit.
>>
>> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>>
>> Now with geneve options the redundant mirror is removed but if there will be
>> a real modification there will still be a mirror output but in an incorrect
>> place.
>>
>> For example adding dec_ttl, the action list will be like this:
>>
>> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>>
>> A mirror output enp8s0f0_1 added at the end but the second mirror is with
>> the tunnel header already.
>>
>> When not using tunnels the mirror is fine and the action list will look like
>> this:
>>
>> actions:port1,dec_ttl,port2,port1
>>
>> So with tunnel the second mirror shouldn't have been somehow with the
>> dec_ttl action but without the tunnel header?
>>
>> Should the actions list somehow be like this
>>
>> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
>
> Not sure I follow you, but do you think the dect ttl and set tunnel should
> have been swapped? I guess this would depend on your OpenFlow rule. Can you
> show an ofproto trace on your example, which might help clarify OVS’s
> reasoning?
>
> //Eelco
>
yes. but not just dec_ttl. any header modification beside encap.
after all original commit explains users could reverse the packet with rules so
there won't be a real packet coming so add mirror after modification.
but mirror is usually before encap or on recv after decap.
I expected action list to be, maybe: mirrorPort, header mod like replace
src/dst and ping type, mirrorPort, encap, tunnelPort.
i'll check about the ofproto trace
>
>>
>> Am I looking at this wrong? What do you think?
>>
>> Thanks,
>> Roi
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote:
>> The cited commit fixed missing mirror packets by reset mirror when
>> packets are modified but setting geneve options was also treated as
>> a modified packet but should be treated as a part of set_tunnel
>> which doesn't reset mirror.
>>
>> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are
>> modified.")
>> Signed-off-by: Roi Dayan
>> Acked-by: Simon Horman
>> Acked-by: Eelco Chaudron
>> ---
>>
>> Notes:
>> v2:
>> - user correct sha in fixes line.
>>
>> ofproto/ofproto-dpif-xlate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index be4bd6657688..e243773307b7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
>> flow *flow,
>>
>> set_field = ofpact_get_SET_FIELD(a);
>> mf = set_field->field;
>> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
>> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
>> ctx->mirrors = 0;
>> }
>> return;
>
>
> Hi,
>
> I would like to consult another related issue to the original commit.
>
> feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
>
> Now with geneve options the redundant mirror is removed but if there will be
> a real modification there will still be a mirror output but in an incorrect
> place.
>
> For example adding dec_ttl, the action list will be like this:
>
> actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
>
> A mirror output enp8s0f0_1 added at the end but the second mirror is with the
> tunnel header already.
>
> When not using tunnels the mirror is fine and the action list will look like
> this:
>
> actions:port1,dec_ttl,port2,port1
>
> So with tunnel the second mirror shouldn't have been somehow with the dec_ttl
> action but without the tunnel header?
>
> Should the actions list somehow be like this
>
> actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Not sure I follow you, but do you think the dect ttl and set tunnel should have
been swapped? I guess this would depend on your OpenFlow rule. Can you show an
ofproto trace on your example, which might help clarify OVS’s reasoning?
//Eelco
>
> Am I looking at this wrong? What do you think?
>
> Thanks,
> Roi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/3] ofproto-dpif-upcall: Fix redundant mirror on geneve tunnel options.
On 09/10/2023 15:05, Roi Dayan wrote:
> The cited commit fixed missing mirror packets by reset mirror when
> packets are modified but setting geneve options was also treated as
> a modified packet but should be treated as a part of set_tunnel
> which doesn't reset mirror.
>
> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
> Signed-off-by: Roi Dayan
> Acked-by: Simon Horman
> Acked-by: Eelco Chaudron
> ---
>
> Notes:
> v2:
> - user correct sha in fixes line.
>
> ofproto/ofproto-dpif-xlate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657688..e243773307b7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct
> flow *flow,
>
> set_field = ofpact_get_SET_FIELD(a);
> mf = set_field->field;
> -if (mf_are_prereqs_ok(mf, flow, NULL)) {
> +if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) {
> ctx->mirrors = 0;
> }
> return;
Hi,
I would like to consult another related issue to the original commit.
feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.")
Now with geneve options the redundant mirror is removed but if there will be
a real modification there will still be a mirror output but in an incorrect
place.
For example adding dec_ttl, the action list will be like this:
actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0x,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1
A mirror output enp8s0f0_1 added at the end but the second mirror is with the
tunnel header already.
When not using tunnels the mirror is fine and the action list will look like
this:
actions:port1,dec_ttl,port2,port1
So with tunnel the second mirror shouldn't have been somehow with the dec_ttl
action but without the tunnel header?
Should the actions list somehow be like this
actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081
Am I looking at this wrong? What do you think?
Thanks,
Roi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
