Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 21 Feb 2024, at 17:45, Adrian Moreno wrote: > On 2/21/24 16:42, Eelco Chaudron wrote: >> >> >> On 21 Feb 2024, at 16:03, Adrian Moreno wrote: >> >>> On 2/21/24 15:49, Aaron Conole wrote: Adrian Moreno writes: > On 2/20/24 19:06, Aaron Conole wrote: >> Eelco Chaudron writes: >> >>> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >>> Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a > particular flow > gets removed from the system. This can be useful when > debugging performance > issues tied to ofproto flow changes, trying to determine > deployed traffic > patterns, or while debugging dynamic systems where ports come > and go. > > Prior to this change, there was a lack of visibility around > flow expiration. > The existing debugging infrastructure could tell us when a > flow was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the > revalidator > determines that the flow should be removed. Additionally, we > track the > reason for the flow eviction and provide that information as > well. With > this change, we can track the complete flow lifecycle for the > netlink > datapath by hooking the upcall tracepoint in kernel, the flow > put USDT, and > the revaldiator USDT, letting us watch as flows are added and > removed from > the kernel datapath. > > This change only enables this information via USDT probe, so > it won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe > might be used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- > Documentation/topics/usdt-probes.rst | 1 + > ofproto/ofproto-dpif-upcall.c| 42 +- > utilities/automake.mk| 3 + > utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ > 4 files changed, 693 insertions(+), 6 deletions(-) > create mode 100755 > utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: > - dpif_recv:recv_upcall > - main:poll_block > - main:run_start > +- revalidate:flow_result > - revalidate_ukey\_\_:entry > - revalidate_ukey\_\_:exit > - udpif_revalidator:start_dump You are missing the specific flow_result result > section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe
Re: [ovs-dev] [PATCH 0/3] Fix ignoring of IPv6 'local_ip' for native tunnels.
On 2/20/24 23:35, Ilya Maximets wrote: > The whole patch set is needed for a two-line fix in the third patch, > more precisely, for the ability to test these two lines. > > First patch in the set is not necessary, but it makes a second patch > a little cleaner. > > The second patch is a rebased an taken one step further original patch > from Ihar: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20201216024108.1227014-1-ihrac...@redhat.com/ > It is necessary, because we need a way to create 'local' routes in > the routing table in order for local_ip functionality to work for > userspace tunnels in unit tests. Without local routes, route lookup > with a source IP specified will fail and the packets will be dropped > and there is no way to create them in the current code. > > Admittedly, I could have created a system test for this issue instead, > but it would be much more complex on its own and we would likely not > be able to check everything that unit tests do since implementation > of kernel and userspace tunnels is very different. And being able to > test this functionality with simpler unit tests seems like a better > idea overall. > > The last patch in the set is an actual bug fix for the issue reported > here: > https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052938.html > More details in the commit message. > > Since all the changes in this set are only related to the test code, > beside the 2 lines in the 3rd patch, it should be fine to backport > the set as a whole so we'll have a test coverage on all branches. > > Ihar Hrachyshka (1): > netdev-dummy: Add local route entries for IP addresses. > > Ilya Maximets (2): > tests: Move the non-local port as tunnel endpoint test. > ofproto-dpif-xlate: Fix ignoring IPv6 local_ip for native tunnels. > > AUTHORS.rst| 1 + > lib/netdev-dummy.c | 17 ++- > lib/ovs-router.c | 14 ++ > lib/ovs-router.h | 5 + > ofproto/ofproto-dpif-xlate.c | 2 + > tests/nsh.at | 14 +- > tests/ofproto-dpif.at | 15 ++- > tests/packet-type-aware.at | 21 ++- > tests/system-layer3-tunnels.at | 55 > tests/tunnel-push-pop-ipv6.at | 116 +++-- > tests/tunnel-push-pop.at | 226 + > tests/tunnel.at| 18 ++- > 12 files changed, 369 insertions(+), 135 deletions(-) > Thanks, Mike and Eelco for review! Applied. Also backported down to 2.17. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-thread: Log pthread failures.
On 2/21/24 15:34, Eelco Chaudron wrote: > > > On 15 Feb 2024, at 13:00, Ilya Maximets wrote: > >> Currently, failures of pthread_* functions are printed to stderr >> only and then OVS aborts. These error messages are hard to find >> and may be even just lost. >> >> Use VLOG_ABORT() instead. It will do the same thing, but will try >> to log the error to the log file and syslog first, if configured. >> >> Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort() >> logic and not just exit with a failure code, because it's likely >> we want a core dump if one of these function failed. For example, >> we would like to have a stack trace in a core dump in case a mutex >> lock failed with 'deadlock avoided'. >> >> Signed-off-by: Ilya Maximets > > Changes look good to me! > > Acked-by: Eelco Chaudron Thanks, Eelco and Simon! Applied. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 3/7] selftests: openvswitch: use non-graceful kills when needed
Adrian Moreno writes:
> On 2/16/24 16:28, Aaron Conole wrote:
>> Normally a spawned process under OVS is given a SIGTERM when the test
>> ends as part of cleanup. However, in case the process is still lingering
>> for some reason, we also send a SIGKILL to force it down faster.
>> Signed-off-by: Aaron Conole
>> ---
>> tools/testing/selftests/net/openvswitch/openvswitch.sh | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> index a5dbde482ba4..678a72ad47c1 100755
>> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> @@ -91,7 +91,8 @@ ovs_add_if () {
>> python3 $ovs_base/ovs-dpctl.py add-if \
>> -u "$2" "$3" >$ovs_dir/$3.out 2>$ovs_dir/$3.err &
>> pid=$!
>> -on_exit "ovs_sbx $1 kill -TERM $pid 2>/dev/null"
>> +on_exit "ovs_sbx $1 kill --timeout 1000 TERM \
>> +--timeout 1000 KILL $pid
>> 2>/dev/null"
>> fi
>> }
>>
>
> AFAIK, this will immediately send TERM, then wait 1s, send TERM again,
> wait 1s then send KILL. Is that what you intended? To avoid the double
> TERM you could:
Okay, I'll adjust it.
> --
> Adrián Moreno
>
>> @@ -108,7 +109,8 @@ ovs_netns_spawn_daemon() {
>> info "spawning cmd: $*"
>> ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
>> pid=$!
>> -ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
>> +ovs_sbx "$sbx" on_exit "kill --timeout 1000 TERM \
>> +--timeout 1000 KILL $pid 2>/dev/null"
>> }
>> ovs_add_netns_and_veths () {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] bond: Reset stats when deleting post recirc rule.
On 2/20/24 07:50, Adrian Moreno wrote:
> In order to properly balance bond traffic, ofproto/bond periodically
> reads usage statistics of the post-recirculation rules (which are added
> to a hidden internal table).
>
> To do that, each "struct bond_entry" (which represents a hash within a
> bond) stores the last seen statistics for its rule. When a hash is moved
> to another member (due to a bond rebalance or the previous member going
> down), the rule is typically just modified, i.e: same match different
> actions. In this case, statistics are preserved and accounting continues
> to work.
>
> However, if the rule gets completely deleted (e.g: when all bond members
> go down) and then re-created, the new rule will have 0 tx_bytes but its
> associated entry will still store a non-zero last-seen value.
> This situation leads to an overflow of the delta calculation (computed
> as [current_stats_value - last_seen_value]), which can affect traffic
> as the hash will be considered to carry a lot of traffic and rebalancing
> will kick in.
Hi, Adrian. Thanks for the v2!
The code seems fine, but I can still reproduce the issue described here
in the commit message. I just added the following lines to an existing
rebalancing test and see overflowed load counters on hashes:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index daeea7775..ca478bb29 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -547,6 +547,22 @@ ovs-appctl time/warp 1000 100
ovs-appctl bond/show > bond3.txt
AT_CHECK([sed -n '/member p2/,/^$/p' bond3.txt | grep 'hash'], [0], [ignore])
+AT_CHECK([ovs-appctl netdev-dummy/set-admin-state p1 down], 0, [OK
+])
+AT_CHECK([ovs-appctl netdev-dummy/set-admin-state p2 down], 0, [OK
+])
+ovs-appctl time/warp 1000 100
+AT_CHECK([ovs-appctl netdev-dummy/set-admin-state p1 up], 0, [OK
+])
+AT_CHECK([ovs-appctl netdev-dummy/set-admin-state p2 up], 0, [OK
+])
+ovs-appctl time/warp 1000 100
+
+AT_CHECK([SEND_TCP_BOND_PKTS([p5], [5], [65500])])
+# We sent 49125 KB of data total in 3 batches. No hash should have more
+# than that amount of load. Just checking that it is within 5 digits.
+AT_CHECK([ovs-appctl bond/show | grep -E '[[0-9]]{6}'], [1])
+
OVS_VSWITCHD_STOP()
AT_CLEANUP
---
./ofproto-dpif.at:564: ovs-appctl bond/show | grep -E '[0-9]{6}'
--- /dev/null 2024-01-29 23:31:19.129371175 +0100
+++ /ovs/tests/testsuite.dir/at-groups/1132/stdout
@@ -0,0 +1,62 @@
+ hash 0: 9007199254740940 kB load
+ hash 2: 9007199254740888 kB load
+ hash 25: 9007199254740940 kB load
+ hash 30: 9007199254740940 kB load
+ hash 33: 9007199254740940 kB load
+ hash 35: 9007199254740940 kB load
+ hash 41: 9007199254740940 kB load
...
Could you, please, take a look?
Also, we should probably add a test like that to the patch.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Userspace: Software fallback for UDP encapsulated TCP segmentation.
On Tue, Feb 20, 2024 at 11:09 PM Mike Pattrick wrote:
>
> When sending packets that are flagged as requiring segmentation to an
> interface that doens't support this feature, send the packet to the TSO
> software fallback instead of dropping it.
>
> Signed-off-by: Mike Pattrick
Recheck-request: github-robot
> ---
> lib/dp-packet-gso.c | 73 +
> lib/dp-packet.h | 26 +++
> lib/netdev-native-tnl.c | 8 +
> lib/netdev.c| 37 +
> tests/system-traffic.at | 58
> 5 files changed, 167 insertions(+), 35 deletions(-)
>
> diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> index 847685ad9..f25abf436 100644
> --- a/lib/dp-packet-gso.c
> +++ b/lib/dp-packet-gso.c
> @@ -47,6 +47,8 @@ dp_packet_gso_seg_new(const struct dp_packet *p, size_t
> hdr_len,
> seg->l2_5_ofs = p->l2_5_ofs;
> seg->l3_ofs = p->l3_ofs;
> seg->l4_ofs = p->l4_ofs;
> +seg->inner_l3_ofs = p->inner_l3_ofs;
> +seg->inner_l4_ofs = p->inner_l4_ofs;
>
> /* The protocol headers remain the same, so preserve hash and mark. */
> *dp_packet_rss_ptr(seg) = *dp_packet_rss_ptr(p);
> @@ -71,7 +73,12 @@ dp_packet_gso_nr_segs(struct dp_packet *p)
> const char *data_tail;
> const char *data_pos;
>
> -data_pos = dp_packet_get_tcp_payload(p);
> +if (dp_packet_hwol_is_tunnel_vxlan(p) ||
> +dp_packet_hwol_is_tunnel_geneve(p)) {
> +data_pos = dp_packet_get_inner_tcp_payload(p);
> +} else {
> +data_pos = dp_packet_get_tcp_payload(p);
> +}
> data_tail = (char *) dp_packet_tail(p) - dp_packet_l2_pad_size(p);
>
> return DIV_ROUND_UP(data_tail - data_pos, segsz);
> @@ -91,12 +98,15 @@ dp_packet_gso(struct dp_packet *p, struct dp_packet_batch
> **batches)
> struct tcp_header *tcp_hdr;
> struct ip_header *ip_hdr;
> struct dp_packet *seg;
> +const char *data_pos;
> uint16_t tcp_offset;
> uint16_t tso_segsz;
> +uint16_t ip_id = 0;
> uint32_t tcp_seq;
> -uint16_t ip_id;
> +bool outer_ipv4;
> int hdr_len;
> int seg_len;
> +bool tnl;
>
> tso_segsz = dp_packet_get_tso_segsz(p);
> if (!tso_segsz) {
> @@ -105,20 +115,35 @@ dp_packet_gso(struct dp_packet *p, struct
> dp_packet_batch **batches)
> return false;
> }
>
> -tcp_hdr = dp_packet_l4(p);
> -tcp_offset = TCP_OFFSET(tcp_hdr->tcp_ctl);
> -tcp_seq = ntohl(get_16aligned_be32(_hdr->tcp_seq));
> -hdr_len = ((char *) dp_packet_l4(p) - (char *) dp_packet_eth(p))
> - + tcp_offset * 4;
> -ip_id = 0;
> -if (dp_packet_hwol_is_ipv4(p)) {
> +if (dp_packet_hwol_is_tunnel_vxlan(p) ||
> +dp_packet_hwol_is_tunnel_geneve(p)) {
> +data_pos = dp_packet_get_inner_tcp_payload(p);
> +outer_ipv4 = dp_packet_hwol_is_outer_ipv4(p);
> +tcp_hdr = dp_packet_inner_l4(p);
> +ip_hdr = dp_packet_inner_l3(p);
> +tnl = true;
> +if (outer_ipv4) {
> +ip_id = ntohs(((struct ip_header *) dp_packet_l3(p))->ip_id);
> +} else if (dp_packet_hwol_is_ipv4(p)) {
> +ip_id = ntohs(ip_hdr->ip_id);
> +}
> +} else {
> +data_pos = dp_packet_get_tcp_payload(p);
> +outer_ipv4 = dp_packet_hwol_is_ipv4(p);
> +tcp_hdr = dp_packet_l4(p);
> ip_hdr = dp_packet_l3(p);
> -ip_id = ntohs(ip_hdr->ip_id);
> +tnl = false;
> +if (outer_ipv4) {
> +ip_id = ntohs(ip_hdr->ip_id);
> +}
> }
>
> +tcp_offset = TCP_OFFSET(tcp_hdr->tcp_ctl);
> +tcp_seq = ntohl(get_16aligned_be32(_hdr->tcp_seq));
> +hdr_len = ((char *) tcp_hdr - (char *) dp_packet_eth(p))
> + + tcp_offset * 4;
> const char *data_tail = (char *) dp_packet_tail(p)
> - dp_packet_l2_pad_size(p);
> -const char *data_pos = dp_packet_get_tcp_payload(p);
> int n_segs = dp_packet_gso_nr_segs(p);
>
> for (int i = 0; i < n_segs; i++) {
> @@ -130,8 +155,26 @@ dp_packet_gso(struct dp_packet *p, struct
> dp_packet_batch **batches)
> seg = dp_packet_gso_seg_new(p, hdr_len, data_pos, seg_len);
> data_pos += seg_len;
>
> +if (tnl) {
> +/* Update tunnel L3 header. */
> +if (dp_packet_hwol_is_ipv4(seg)) {
> +ip_hdr = dp_packet_inner_l3(seg);
> +ip_hdr->ip_tot_len = htons(sizeof *ip_hdr +
> + dp_packet_inner_l4_size(seg));
> +ip_hdr->ip_id = htons(ip_id);
> +ip_hdr->ip_csum = 0;
> +ip_id++;
> +} else {
> +struct ovs_16aligned_ip6_hdr *ip6_hdr;
> +
> +ip6_hdr = dp_packet_inner_l3(seg);
> +ip6_hdr->ip6_ctlun.ip6_un1.ip6_un1_plen
> += htons(dp_packet_inner_l3_size(seg) - sizeof *ip6_hdr);
> +}
[ovs-dev] [PATCH v3] userspace: Allow UDP zero checksum with IPv6 tunnels.
This patch adopts the proposed RFC 6935 by allowing null UDP checksums
even if the tunnel protocol is IPv6. This is already supported by Linux
through the udp6zerocsumtx tunnel option. It is disabled by default and
IPv6 tunnels are flagged as requiring a checksum, but this patch enables
the user to set csum=false on IPv6 tunnels.
Signed-off-by: Mike Pattrick
---
v2: Changed documentation, and added a NEWS item
v3: NEWS file merge conflict
---
NEWS| 3 +++
lib/netdev-native-tnl.c | 2 +-
lib/netdev-vport.c | 13 +++--
lib/netdev.h| 9 -
ofproto/tunnel.c| 11 +--
tests/tunnel.at | 6 +++---
vswitchd/vswitch.xml| 11 ---
7 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/NEWS b/NEWS
index c9e4064e6..3a75d3850 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ Post-v3.3.0
* Conntrack now supports 'random' flag for selecting ports in a range
while natting and 'persistent' flag for selection of the IP address
from a range.
+ * IPv6 UDP tunnels will now honour the csum option. Configuring the
+ interface with "options:csum=false" now has the same effect in OVS
+ as the udp6zerocsumtx option has with kernel UDP tunnels.
v3.3.0 - 16 Feb 2024
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..e8258bc4e 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config *tnl_cfg,
udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
udp->udp_dst = tnl_cfg->dst_port;
-if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
+if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
/* Write a value in now to mark that we should compute the checksum
* later. 0x is handy because it is transparent to the
* calculation. */
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 60caa02fb..f9a778988 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap
*args, char **errp)
tnl_cfg.dst_port = htons(atoi(node->value));
} else if (!strcmp(node->key, "csum") && has_csum) {
if (!strcmp(node->value, "true")) {
-tnl_cfg.csum = true;
+tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
+} else if (!strcmp(node->value, "false")) {
+tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
}
} else if (!strcmp(node->key, "seq") && has_seq) {
if (!strcmp(node->value, "true")) {
@@ -850,6 +852,11 @@ set_tunnel_config(struct netdev *dev_, const struct smap
*args, char **errp)
}
}
+/* The default csum state for GRE is special. */
+if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) {
+tnl_cfg.csum = NETDEV_TNL_CSUM_DEFAULT_GRE;
+}
+
enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
const char *full_type = (strcmp(type, "vxlan") ? type
: (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
@@ -1026,8 +1033,10 @@ get_tunnel_config(const struct netdev *dev, struct smap
*args)
}
}
-if (tnl_cfg->csum) {
+if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
smap_add(args, "csum", "true");
+} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
+smap_add(args, "csum", "false");
}
if (tnl_cfg->set_seq) {
diff --git a/lib/netdev.h b/lib/netdev.h
index 67a8486bd..a79531e6d 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -111,6 +111,13 @@ enum netdev_srv6_flowlabel {
SRV6_FLOWLABEL_COMPUTE,
};
+enum netdev_tnl_csum {
+NETDEV_TNL_CSUM_DEFAULT,
+NETDEV_TNL_CSUM_ENABLED,
+NETDEV_TNL_CSUM_DISABLED,
+NETDEV_TNL_CSUM_DEFAULT_GRE,
+};
+
/* Configuration specific to tunnels. */
struct netdev_tunnel_config {
ovs_be64 in_key;
@@ -139,7 +146,7 @@ struct netdev_tunnel_config {
uint8_t tos;
bool tos_inherit;
-bool csum;
+enum netdev_tnl_csum csum;
bool dont_fragment;
enum netdev_pt_mode pt_mode;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 80ddee78a..6f462874e 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -465,9 +465,14 @@ tnl_port_send(const struct ofport_dpif *ofport, struct
flow *flow,
flow->tunnel.flags &= ~(FLOW_TNL_F_MASK & ~FLOW_TNL_PUB_F_MASK);
flow->tunnel.flags |= (cfg->dont_fragment ? FLOW_TNL_F_DONT_FRAGMENT : 0)
-| (cfg->csum ? FLOW_TNL_F_CSUM : 0)
| (cfg->out_key_present ? FLOW_TNL_F_KEY : 0);
+if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
+flow->tunnel.flags |= FLOW_TNL_F_CSUM;
+} else if (cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst) {
+flow->tunnel.flags |= FLOW_TNL_F_CSUM;
+}
+
if (cfg->set_egress_pkt_mark) {
flow->pkt_mark =
Re: [ovs-dev] [PATCH v1] ovs-tcpdump: Cleanup mirror failed with twice fatal signals
On 2/21/24 15:27, Aaron Conole wrote: > Daniel Ding writes: > >> After running ovs-tcpdump and inputs multiple CTRL+C, the program will >> raise the following exception. >> >> Error in atexit._run_exitfuncs: >> Traceback (most recent call last): >> File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror >> ovsdb = OVSDB(db_sock) >> File "/usr/bin/ovs-tcpdump", line 168, in __init__ >> OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB >> File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change >> while idl.change_seqno == seq and not idl.run(): >> >> Signed-off-by: Daniel Ding >> --- > > LGTM for the linux side - maybe Alin might check the windows side. The code is copied from the fatal-signla library, so it is likely fine. However, I don;t think this issue should be fixed on the application level (ovs-tcpdump). There seems to be adifference in how C and python fatal-signla libraries behave. The C version calls the hooks in the run() function and the signal handler only updates the signal number variable. So, if the same signal arrives multiple times it will be handled only once and the run() function will not exit until all the hooks are done, unless it is a segfault. With python implementation we're calling hooks right from the signal handler (it's not a real signal handler, so we're allowed to use not really singal-safe functions there). So, if another signal arrives while we're executing the hooks, the second hook execution will be skipped due to 'recursion' check, but the signal will be re-raised immediately, killing the process. There is likley a way to fix that by using a mutex instead of recursion check and blocking it while executing the hooks. The signal number will need to be stored in the signal handler and the signal will need to be re-raised in the call_hooks() instead of signal handler. We can use mutex.acquire(blocking=False) as a way to prevent recursion. Does that make sense? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 3/7] selftests: openvswitch: use non-graceful kills when needed
On 2/16/24 16:28, Aaron Conole wrote:
Normally a spawned process under OVS is given a SIGTERM when the test
ends as part of cleanup. However, in case the process is still lingering
for some reason, we also send a SIGKILL to force it down faster.
Signed-off-by: Aaron Conole
---
tools/testing/selftests/net/openvswitch/openvswitch.sh | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index a5dbde482ba4..678a72ad47c1 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -91,7 +91,8 @@ ovs_add_if () {
python3 $ovs_base/ovs-dpctl.py add-if \
-u "$2" "$3" >$ovs_dir/$3.out 2>$ovs_dir/$3.err &
pid=$!
- on_exit "ovs_sbx $1 kill -TERM $pid 2>/dev/null"
+ on_exit "ovs_sbx $1 kill --timeout 1000 TERM \
+--timeout 1000 KILL $pid 2>/dev/null"
fi
}
AFAIK, this will immediately send TERM, then wait 1s, send TERM again, wait 1s
then send KILL. Is that what you intended? To avoid the double TERM you could:
kill --timeout 1000 KILL --signal TERM $pid
--
Adrián Moreno
@@ -108,7 +109,8 @@ ovs_netns_spawn_daemon() {
info "spawning cmd: $*"
ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
pid=$!
- ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
+ ovs_sbx "$sbx" on_exit "kill --timeout 1000 TERM \
+--timeout 1000 KILL $pid 2>/dev/null"
}
ovs_add_netns_and_veths () {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] userspace: Allow UDP zero checksum with IPv6 tunnels.
Bleep bloop. Greetings Mike Pattrick, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 userspace: Allow UDP zero checksum with IPv6 tunnels. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] userspace: Allow UDP zero checksum with IPv6 tunnels.
This patch adopts the proposed RFC 6935 by allowing null UDP checksums
even if the tunnel protocol is IPv6. This is already supported by Linux
through the udp6zerocsumtx tunnel option. It is disabled by default and
IPv6 tunnels are flagged as requiring a checksum, but this patch enables
the user to set csum=false on IPv6 tunnels.
Signed-off-by: Mike Pattrick
---
v2: Changed documentation, and added a NEWS item
---
NEWS| 5 -
lib/netdev-native-tnl.c | 2 +-
lib/netdev-vport.c | 13 +++--
lib/netdev.h| 9 -
ofproto/tunnel.c| 11 +--
tests/tunnel.at | 6 +++---
vswitchd/vswitch.xml| 11 ---
7 files changed, 44 insertions(+), 13 deletions(-)
diff --git a/NEWS b/NEWS
index 0789dc0c6..84402ff8f 100644
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,9 @@
Post-v3.3.0
-
+ - Userspace datapath:
+ * IPv6 UDP tunnels will now honour the csum option. Configuring the
+ interface with "options:csum=false" now has the same effect in OVS
+ as the udp6zerocsumtx option has with kernel UDP tunnels.
v3.3.0 - 16 Feb 2024
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index dee9ab344..e8258bc4e 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config *tnl_cfg,
udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
udp->udp_dst = tnl_cfg->dst_port;
-if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
+if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
/* Write a value in now to mark that we should compute the checksum
* later. 0x is handy because it is transparent to the
* calculation. */
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 60caa02fb..f9a778988 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap
*args, char **errp)
tnl_cfg.dst_port = htons(atoi(node->value));
} else if (!strcmp(node->key, "csum") && has_csum) {
if (!strcmp(node->value, "true")) {
-tnl_cfg.csum = true;
+tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
+} else if (!strcmp(node->value, "false")) {
+tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
}
} else if (!strcmp(node->key, "seq") && has_seq) {
if (!strcmp(node->value, "true")) {
@@ -850,6 +852,11 @@ set_tunnel_config(struct netdev *dev_, const struct smap
*args, char **errp)
}
}
+/* The default csum state for GRE is special. */
+if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) {
+tnl_cfg.csum = NETDEV_TNL_CSUM_DEFAULT_GRE;
+}
+
enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
const char *full_type = (strcmp(type, "vxlan") ? type
: (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
@@ -1026,8 +1033,10 @@ get_tunnel_config(const struct netdev *dev, struct smap
*args)
}
}
-if (tnl_cfg->csum) {
+if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
smap_add(args, "csum", "true");
+} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
+smap_add(args, "csum", "false");
}
if (tnl_cfg->set_seq) {
diff --git a/lib/netdev.h b/lib/netdev.h
index 67a8486bd..a79531e6d 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -111,6 +111,13 @@ enum netdev_srv6_flowlabel {
SRV6_FLOWLABEL_COMPUTE,
};
+enum netdev_tnl_csum {
+NETDEV_TNL_CSUM_DEFAULT,
+NETDEV_TNL_CSUM_ENABLED,
+NETDEV_TNL_CSUM_DISABLED,
+NETDEV_TNL_CSUM_DEFAULT_GRE,
+};
+
/* Configuration specific to tunnels. */
struct netdev_tunnel_config {
ovs_be64 in_key;
@@ -139,7 +146,7 @@ struct netdev_tunnel_config {
uint8_t tos;
bool tos_inherit;
-bool csum;
+enum netdev_tnl_csum csum;
bool dont_fragment;
enum netdev_pt_mode pt_mode;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 80ddee78a..6f462874e 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -465,9 +465,14 @@ tnl_port_send(const struct ofport_dpif *ofport, struct
flow *flow,
flow->tunnel.flags &= ~(FLOW_TNL_F_MASK & ~FLOW_TNL_PUB_F_MASK);
flow->tunnel.flags |= (cfg->dont_fragment ? FLOW_TNL_F_DONT_FRAGMENT : 0)
-| (cfg->csum ? FLOW_TNL_F_CSUM : 0)
| (cfg->out_key_present ? FLOW_TNL_F_KEY : 0);
+if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
+flow->tunnel.flags |= FLOW_TNL_F_CSUM;
+} else if (cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst) {
+flow->tunnel.flags |= FLOW_TNL_F_CSUM;
+}
+
if (cfg->set_egress_pkt_mark) {
flow->pkt_mark = cfg->egress_pkt_mark;
wc->masks.pkt_mark = UINT32_MAX;
@@ -706,8 +711,10 @@ tnl_port_format(const struct tnl_port *tnl_port,
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/21/24 16:42, Eelco Chaudron wrote:
On 21 Feb 2024, at 16:03, Adrian Moreno wrote:
On 2/21/24 15:49, Aaron Conole wrote:
Adrian Moreno writes:
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result
section. This is from the previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 21 Feb 2024, at 16:03, Adrian Moreno wrote: > On 2/21/24 15:49, Aaron Conole wrote: >> Adrian Moreno writes: >> >>> On 2/20/24 19:06, Aaron Conole wrote: Eelco Chaudron writes: > On 19 Feb 2024, at 19:57, Aaron Conole wrote: > >> Eelco Chaudron writes: >> >>> On 12 Feb 2024, at 15:15, Aaron Conole wrote: >>> Aaron Conole writes: > Eelco Chaudron writes: > >> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: >> >>> On 2/1/24 10:02, Eelco Chaudron wrote: On 31 Jan 2024, at 18:03, Aaron Conole wrote: > Eelco Chaudron writes: > >> On 25 Jan 2024, at 21:55, Aaron Conole wrote: >> >>> From: Kevin Sprague >>> >>> During normal operations, it is useful to understand when a >>> particular flow >>> gets removed from the system. This can be useful when debugging >>> performance >>> issues tied to ofproto flow changes, trying to determine >>> deployed traffic >>> patterns, or while debugging dynamic systems where ports come >>> and go. >>> >>> Prior to this change, there was a lack of visibility around >>> flow expiration. >>> The existing debugging infrastructure could tell us when a flow >>> was added to >>> the datapath, but not when it was removed or why. >>> >>> This change introduces a USDT probe at the point where the >>> revalidator >>> determines that the flow should be removed. Additionally, we >>> track the >>> reason for the flow eviction and provide that information as >>> well. With >>> this change, we can track the complete flow lifecycle for the >>> netlink >>> datapath by hooking the upcall tracepoint in kernel, the flow >>> put USDT, and >>> the revaldiator USDT, letting us watch as flows are added and >>> removed from >>> the kernel datapath. >>> >>> This change only enables this information via USDT probe, so it >>> won't be >>> possible to access this information any other way (see: >>> Documentation/topics/usdt-probes.rst). >>> >>> Also included is a script >>> (utilities/usdt-scripts/flow_reval_monitor.py) >>> which serves as a demonstration of how the new USDT probe might >>> be used >>> going forward. >>> >>> Signed-off-by: Kevin Sprague >>> Co-authored-by: Aaron Conole >>> Signed-off-by: Aaron Conole >> >> Thanks for following this up Aaron! See comments on this patch >> below. I have no additional comments on patch 2. >> >> Cheers, >> >> Eelco >> >> >>> --- >>> Documentation/topics/usdt-probes.rst | 1 + >>> ofproto/ofproto-dpif-upcall.c| 42 +- >>> utilities/automake.mk| 3 + >>> utilities/usdt-scripts/flow_reval_monitor.py | 653 >>> +++ >>> 4 files changed, 693 insertions(+), 6 deletions(-) >>> create mode 100755 >>> utilities/usdt-scripts/flow_reval_monitor.py >>> >>> diff --git a/Documentation/topics/usdt-probes.rst >>> b/Documentation/topics/usdt-probes.rst >>> index e527f43bab..a8da9bb1f7 100644 >>> --- a/Documentation/topics/usdt-probes.rst >>> +++ b/Documentation/topics/usdt-probes.rst >>> @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >>> - dpif_recv:recv_upcall >>> - main:poll_block >>> - main:run_start >>> +- revalidate:flow_result >>> - revalidate_ukey\_\_:entry >>> - revalidate_ukey\_\_:exit >>> - udpif_revalidator:start_dump >> >> You are missing the specific flow_result result >>> section. This is from the previous patch: > > D'oh! Thanks for catching it. I'll re-add it. > >> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe >> above. >> - ``utilities/usdt-scripts/bridge_loop.bt`` >> >> >> +probe revalidate:flow_result >> +~ >> + >> +**Description**: >> +This probe is triggered when the revalidator decides whether or
Re: [ovs-dev] [PATCH 3/3] ofproto-dpif-xlate: Fix ignoring IPv6 local_ip for native tunnels.
On 20 Feb 2024, at 23:35, Ilya Maximets wrote:
> Local IP is taken into account only in case of IPv4 address, IPv6
> source is not checked. That leads to source being ignored during the
> route lookup and ultimately packets encapsulated with a source IP
> found during a route lookup, which is likely the wrong one.
>
> Even worse, after encapsulation we have a difference between the
> tunnel metadata that contains a correct source IP and the generated
> actions that used a wrong source IP. This means that if there are
> OpenFlow rules in a bridge where packet goes after encapsulation,
> we may match on rules that do not correspond to the actual packet
> we have.
>
> Add the check for IPv6 source address before the route lookup.
>
> Tests added to check that we're actually using the configured local_ip
> as a source address in the packet. Also adding the same test for IPv4,
> since apparently we don't have any tests covering this functionality
> for userspace tunnels.
>
> This issue also affects the case where source address is set via
> OpenFlow, e.g. 'set_filed:2001:beef::88->tun_ipv6_src', but it's just
> a different way of populating the tunnel metadata that doesn't depend
> on a tunnel to be native or kernel one. So, not adding extra tests
> for this case for now.
>
> Fixes: 8e4e45887ec3 ("ofproto-dpif-xlate: makes OVS native tunneling honor
> tunnel-specified source addresses")
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052938.html
> Reported-by: Derrick Lim
> Signed-off-by: Ilya Maximets
Thanks for the patch, the changes look good to me and the tests are passing.
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-dummy: Add local route entries for IP addresses.
On 20 Feb 2024, at 23:35, Ilya Maximets wrote: > From: Ihar Hrachyshka > > To mimic what kernel routing subsystem does [1], add a local route > entry for every dummy IP address. This helps with OVN testing multiple > chassis on a single host and allows to run better unit tests for > userspace tunnels without adding route entries manually. This is also > the only way to add 'local' route entries that are required for testing > 'local_ip' functionality with native tunnels in userspace datapath > because route lookup will reject non-local source IPs. > > There seems to be no way to explicitly remove an IP address from > netdev-dummy, hence no code path to handle route entry cleanup. > The port itself can be removed, but our tests do not normally do that. > Removal can be implemented later if necessary. > > [1]: http://linux-ip.net/html/routing-tables.html#routing-table-local > > "If the machine has several IP addresses on one Ethernet interface, > there will be a route to each locally hosted IP in the local routing > table. This is a normal side effect of bringing up an IP address on > an interface under linux." > > Signed-off-by: Ihar Hrachyshka > Co-authored-by: Ilya Maximets > Signed-of-by: Ilya Maximets Thanks for the patch, the changes look good to me and the tests are passing. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] tests: Move the non-local port as tunnel endpoint test.
On 20 Feb 2024, at 23:35, Ilya Maximets wrote: > It's not a system test as it runs with dummy datapath and ports > and it has nothing to do with layer 3 tunnels. > > It should be with other userspace tunnel tests. > > While moving also making it a little nicer visually and less error > prone by requesting port numbers for all the ports. > > Signed-off-by: Ilya Maximets Thanks for the patch, the changes look good to me and the tests are passing. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/21/24 15:49, Aaron Conole wrote:
Adrian Moreno writes:
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result
section. This is from the previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have
updated the counters. i.e. it all depends on ‘bool need_revalidate
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Adrian Moreno writes: > On 2/20/24 19:06, Aaron Conole wrote: >> Eelco Chaudron writes: >> >>> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >>> Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > During normal operations, it is useful to understand when a > particular flow > gets removed from the system. This can be useful when debugging > performance > issues tied to ofproto flow changes, trying to determine deployed > traffic > patterns, or while debugging dynamic systems where ports come and > go. > > Prior to this change, there was a lack of visibility around flow > expiration. > The existing debugging infrastructure could tell us when a flow > was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the > revalidator > determines that the flow should be removed. Additionally, we > track the > reason for the flow eviction and provide that information as > well. With > this change, we can track the complete flow lifecycle for the > netlink > datapath by hooking the upcall tracepoint in kernel, the flow put > USDT, and > the revaldiator USDT, letting us watch as flows are added and > removed from > the kernel datapath. > > This change only enables this information via USDT probe, so it > won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script > (utilities/usdt-scripts/flow_reval_monitor.py) > which serves as a demonstration of how the new USDT probe might > be used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- >Documentation/topics/usdt-probes.rst | 1 + >ofproto/ofproto-dpif-upcall.c| 42 +- >utilities/automake.mk| 3 + >utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ >4 files changed, 693 insertions(+), 6 deletions(-) >create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: >- dpif_recv:recv_upcall >- main:poll_block >- main:run_start > +- revalidate:flow_result >- revalidate_ukey\_\_:entry >- revalidate_ukey\_\_:exit >- udpif_revalidator:start_dump You are missing the specific flow_result result > section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. - ``utilities/usdt-scripts/bridge_loop.bt`` +probe revalidate:flow_result +~ + +**Description**: +This probe is triggered when the revalidator decides whether or not to +revalidate a flow. ``reason`` is an enum that denotes that either the flow +is being kept, or the reason why the flow is being deleted. The +``flow_reval_monitor.py`` script uses this probe to notify users when flows +matching user-provided criteria are deleted.
Re: [ovs-dev] [PATCH] ovs-thread: Log pthread failures.
On 15 Feb 2024, at 13:00, Ilya Maximets wrote: > Currently, failures of pthread_* functions are printed to stderr > only and then OVS aborts. These error messages are hard to find > and may be even just lost. > > Use VLOG_ABORT() instead. It will do the same thing, but will try > to log the error to the log file and syslog first, if configured. > > Using VLOG_ABORT() instead of VLOG_FATAL() to preserve the abort() > logic and not just exit with a failure code, because it's likely > we want a core dump if one of these function failed. For example, > we would like to have a stack trace in a core dump in case a mutex > lock failed with 'deadlock avoided'. > > Signed-off-by: Ilya Maximets Changes look good to me! Acked-by: Eelco Chaudron //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ovs-tcpdump: Cleanup mirror failed with twice fatal signals
Daniel Ding writes: > After running ovs-tcpdump and inputs multiple CTRL+C, the program will > raise the following exception. > > Error in atexit._run_exitfuncs: > Traceback (most recent call last): > File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror > ovsdb = OVSDB(db_sock) > File "/usr/bin/ovs-tcpdump", line 168, in __init__ > OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB > File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change > while idl.change_seqno == seq and not idl.run(): > > Signed-off-by: Daniel Ding > --- LGTM for the linux side - maybe Alin might check the windows side. When you post v2 you can keep my Reviewed-by: Aaron Conole > utilities/ovs-tcpdump.in | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in > index 4cbd9a5d3..eec2262d6 100755 > --- a/utilities/ovs-tcpdump.in > +++ b/utilities/ovs-tcpdump.in > @@ -17,6 +17,7 @@ > import os > import pwd > from random import randint > +import signal > import subprocess > import sys > import time > @@ -417,8 +418,22 @@ def py_which(executable): > for path in os.environ["PATH"].split(os.pathsep)) > > > +def ignore_fatal_signals(): > +if sys.platform == "win32": > +signals = [signal.SIGTERM, signal.SIGINT] > +else: > +signals = [signal.SIGTERM, signal.SIGINT, signal.SIGHUP, > + signal.SIGALRM] > + > +for signr in signals: > +signal.signal(signr, signal.SIG_IGN) > + > + Just a nit, but it might be clearer to put the ignore_fatal_signals as a function scoped in teardown below. That way it is a bit clearer that this will only be called to turn off these in the double failure case. > def teardown(db_sock, interface, mirror_interface, tap_created): > def cleanup_mirror(): > +# Ignore fatal signals, avoid it to break OVSDB operations. > +# So that cleanup mirror and ports be finished. > +ignore_fatal_signals() > try: > ovsdb = OVSDB(db_sock) > ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface)) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Allow UDP zero checksum with IPv6 tunnels.
Mike Pattrick writes: > On Tue, Feb 20, 2024 at 8:56 PM Mike Pattrick wrote: >> >> This patch adopts the proposed RFC 6935 by allowing null UDP checksums >> even if the tunnel protocol is IPv6. This is already supported by Linux >> through the udp6zerocsumtx tunnel option. It is disabled by default and >> IPv6 tunnels are flagged as requiring a checksum, but this patch enables >> the user to set csum=false on IPv6 tunnels. >> >> Signed-off-by: Mike Pattrick > > One of the github CI runners failed this in test "bfd - bfd decay". I > believe this is a false negative. In the future, you can send a recheck request (I included one with my previous comment) to re-run and clear these "flaky" tests. > -M > > ___ > 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] userspace: Allow UDP zero checksum with IPv6 tunnels.
Mike Pattrick writes:
> This patch adopts the proposed RFC 6935 by allowing null UDP checksums
> even if the tunnel protocol is IPv6. This is already supported by Linux
> through the udp6zerocsumtx tunnel option. It is disabled by default and
> IPv6 tunnels are flagged as requiring a checksum, but this patch enables
> the user to set csum=false on IPv6 tunnels.
>
> Signed-off-by: Mike Pattrick
> ---
Recheck-request: github-robot
> lib/netdev-native-tnl.c | 2 +-
> lib/netdev-vport.c | 13 +++--
> lib/netdev.h| 9 -
> ofproto/tunnel.c| 11 +--
> tests/tunnel.at | 6 +++---
> vswitchd/vswitch.xml| 8 +---
> 6 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index dee9ab344..e8258bc4e 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -424,7 +424,7 @@ udp_build_header(const struct netdev_tunnel_config
> *tnl_cfg,
> udp = netdev_tnl_ip_build_header(data, params, IPPROTO_UDP, 0);
> udp->udp_dst = tnl_cfg->dst_port;
>
> -if (params->is_ipv6 || params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
> +if (params->flow->tunnel.flags & FLOW_TNL_F_CSUM) {
Previously, this encap csum flag would be ignored for IPv6 tunnels, but
now it will be honored for them. It should be noted in the NEWS file,
since it is user impacting.
> /* Write a value in now to mark that we should compute the checksum
> * later. 0x is handy because it is transparent to the
> * calculation. */
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 60caa02fb..f9a778988 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -702,7 +702,9 @@ set_tunnel_config(struct netdev *dev_, const struct smap
> *args, char **errp)
> tnl_cfg.dst_port = htons(atoi(node->value));
> } else if (!strcmp(node->key, "csum") && has_csum) {
> if (!strcmp(node->value, "true")) {
> -tnl_cfg.csum = true;
> +tnl_cfg.csum = NETDEV_TNL_CSUM_ENABLED;
> +} else if (!strcmp(node->value, "false")) {
> +tnl_cfg.csum = NETDEV_TNL_CSUM_DISABLED;
> }
> } else if (!strcmp(node->key, "seq") && has_seq) {
> if (!strcmp(node->value, "true")) {
> @@ -850,6 +852,11 @@ set_tunnel_config(struct netdev *dev_, const struct smap
> *args, char **errp)
> }
> }
>
> +/* The default csum state for GRE is special. */
> +if (tnl_cfg.csum == NETDEV_TNL_CSUM_DEFAULT && strstr(type, "gre")) {
> +tnl_cfg.csum = NETDEV_TNL_CSUM_DEFAULT_GRE;
> +}
> +
> enum tunnel_layers layers = tunnel_supported_layers(type, _cfg);
> const char *full_type = (strcmp(type, "vxlan") ? type
> : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE)
> @@ -1026,8 +1033,10 @@ get_tunnel_config(const struct netdev *dev, struct
> smap *args)
> }
> }
>
> -if (tnl_cfg->csum) {
> +if (tnl_cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
> smap_add(args, "csum", "true");
> +} else if (tnl_cfg->csum == NETDEV_TNL_CSUM_DISABLED) {
> +smap_add(args, "csum", "false");
> }
>
> if (tnl_cfg->set_seq) {
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 67a8486bd..a79531e6d 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -111,6 +111,13 @@ enum netdev_srv6_flowlabel {
> SRV6_FLOWLABEL_COMPUTE,
> };
>
> +enum netdev_tnl_csum {
> +NETDEV_TNL_CSUM_DEFAULT,
> +NETDEV_TNL_CSUM_ENABLED,
> +NETDEV_TNL_CSUM_DISABLED,
> +NETDEV_TNL_CSUM_DEFAULT_GRE,
> +};
> +
> /* Configuration specific to tunnels. */
> struct netdev_tunnel_config {
> ovs_be64 in_key;
> @@ -139,7 +146,7 @@ struct netdev_tunnel_config {
> uint8_t tos;
> bool tos_inherit;
>
> -bool csum;
> +enum netdev_tnl_csum csum;
> bool dont_fragment;
> enum netdev_pt_mode pt_mode;
>
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 80ddee78a..6f462874e 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -465,9 +465,14 @@ tnl_port_send(const struct ofport_dpif *ofport, struct
> flow *flow,
>
> flow->tunnel.flags &= ~(FLOW_TNL_F_MASK & ~FLOW_TNL_PUB_F_MASK);
> flow->tunnel.flags |= (cfg->dont_fragment ? FLOW_TNL_F_DONT_FRAGMENT : 0)
> -| (cfg->csum ? FLOW_TNL_F_CSUM : 0)
> | (cfg->out_key_present ? FLOW_TNL_F_KEY : 0);
>
> +if (cfg->csum == NETDEV_TNL_CSUM_ENABLED) {
> +flow->tunnel.flags |= FLOW_TNL_F_CSUM;
> +} else if (cfg->csum == NETDEV_TNL_CSUM_DEFAULT && !flow->tunnel.ip_dst)
> {
> +flow->tunnel.flags |= FLOW_TNL_F_CSUM;
> +}
> +
> if (cfg->set_egress_pkt_mark) {
> flow->pkt_mark = cfg->egress_pkt_mark;
> wc->masks.pkt_mark = UINT32_MAX;
> @@ -706,8 +711,10 @@ tnl_port_format(const struct tnl_port *tnl_port, struct
> ds *ds)
> ds_put_cstr(ds,
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 2/20/24 19:06, Aaron Conole wrote:
Eelco Chaudron writes:
On 19 Feb 2024, at 19:57, Aaron Conole wrote:
Eelco Chaudron writes:
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
Aaron Conole writes:
Eelco Chaudron writes:
On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
On 2/1/24 10:02, Eelco Chaudron wrote:
On 31 Jan 2024, at 18:03, Aaron Conole wrote:
Eelco Chaudron writes:
On 25 Jan 2024, at 21:55, Aaron Conole wrote:
From: Kevin Sprague
During normal operations, it is useful to understand when a particular flow
gets removed from the system. This can be useful when debugging performance
issues tied to ofproto flow changes, trying to determine deployed traffic
patterns, or while debugging dynamic systems where ports come and go.
Prior to this change, there was a lack of visibility around flow expiration.
The existing debugging infrastructure could tell us when a flow was added to
the datapath, but not when it was removed or why.
This change introduces a USDT probe at the point where the revalidator
determines that the flow should be removed. Additionally, we track the
reason for the flow eviction and provide that information as well. With
this change, we can track the complete flow lifecycle for the netlink
datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and
the revaldiator USDT, letting us watch as flows are added and removed from
the kernel datapath.
This change only enables this information via USDT probe, so it won't be
possible to access this information any other way (see:
Documentation/topics/usdt-probes.rst).
Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
Thanks for following this up Aaron! See comments on this patch
below. I have no additional comments on patch 2.
Cheers,
Eelco
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653 +++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
You are missing the specific flow_result result section. This is from the
previous patch:
D'oh! Thanks for catching it. I'll re-add it.
@@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
- ``utilities/usdt-scripts/bridge_loop.bt``
+probe revalidate:flow_result
+~
+
+**Description**:
+This probe is triggered when the revalidator decides whether or not to
+revalidate a flow. ``reason`` is an enum that denotes that either the flow
+is being kept, or the reason why the flow is being deleted. The
+``flow_reval_monitor.py`` script uses this probe to notify users when flows
+matching user-provided criteria are deleted.
+
+**Arguments**:
+
+- *arg0*: ``(enum flow_del_reason) reason``
+- *arg1*: ``(struct udpif *) udpif``
+- *arg2*: ``(struct udpif_key *) ukey``
+
+**Script references**:
+
+- ``utilities/usdt-scripts/flow_reval_monitor.py``
+
+
Adding your own probes
--
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE = 0, /* The flow was revalidated. */
It was called FDR_FLOW_LIVE before, which might make more
sense. As the flow is just NOT deleted. It might or might not have
been revalidated. Thoughts?
I think it had to have been revalidated if we emit the reason, because
we only emit the reason code after revalidation. IE: there are many
places where we skip revalidation but the flow stays live - and we don't
emit reasons in those cases.
So at least for this patch, it MUST have been revalidated. But maybe in
the future, we would want to catch cases where the flow hasn't been. In
that case, it makes sense to add the FDR_FLOW_LIVE at that time - I
think.
Maybe you disagree?
Well, it depends on how you define revalidation, it might only have
updated the counters. i.e. it all depends on ‘bool need_revalidate =
ukey->reval_seq != reval_seq;’ in revalidate_ukey(). That was why I
opted for a
Re: [ovs-dev] [PATCH v3 2/2] conntrack: Handle persistent selection for IP addresses.
On Mon, Feb 19, 2024 at 01:27:23PM +, Simon Horman wrote: > On Fri, Feb 16, 2024 at 06:19:14PM +0100, Paolo Valerio wrote: > > The patch, when 'persistent' flag is specified, makes the IP selection > > in a range persistent across reboots. > > > > Signed-off-by: Paolo Valerio > > Acked-by: Simon Horman > > --- > > v3: > > - rearranged branches in nat_get_unique_tuple() (Simon) > > Thanks Paolo, > > For the record I'm (still) happy with this patch. > > I'll plan to apply this series unless there is feedback > to the contrary in the next few days. Thanks Paolo and Aaron, Applied with Aaron's Acks. - conntrack: Handle persistent selection for IP addresses. https://github.com/openvswitch/ovs/commit/afdc1171a8f1 - conntrack: Handle random selection for port ranges. https://github.com/openvswitch/ovs/commit/99413ec2610f ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 20 Feb 2024, at 19:10, Adrian Moreno wrote: > On 2/20/24 13:43, Eelco Chaudron wrote: >> >> >> On 19 Feb 2024, at 19:57, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 12 Feb 2024, at 15:15, Aaron Conole wrote: > Aaron Conole writes: > >> Eelco Chaudron writes: >> >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: >>> On 2/1/24 10:02, Eelco Chaudron wrote: > > > On 31 Jan 2024, at 18:03, Aaron Conole wrote: > >> Eelco Chaudron writes: >> >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: >>> From: Kevin Sprague During normal operations, it is useful to understand when a particular flow gets removed from the system. This can be useful when debugging performance issues tied to ofproto flow changes, trying to determine deployed traffic patterns, or while debugging dynamic systems where ports come and go. Prior to this change, there was a lack of visibility around flow expiration. The existing debugging infrastructure could tell us when a flow was added to the datapath, but not when it was removed or why. This change introduces a USDT probe at the point where the revalidator determines that the flow should be removed. Additionally, we track the reason for the flow eviction and provide that information as well. With this change, we can track the complete flow lifecycle for the netlink datapath by hooking the upcall tracepoint in kernel, the flow put USDT, and the revaldiator USDT, letting us watch as flows are added and removed from the kernel datapath. This change only enables this information via USDT probe, so it won't be possible to access this information any other way (see: Documentation/topics/usdt-probes.rst). Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py) which serves as a demonstration of how the new USDT probe might be used going forward. Signed-off-by: Kevin Sprague Co-authored-by: Aaron Conole Signed-off-by: Aaron Conole >>> >>> Thanks for following this up Aaron! See comments on this patch >>> below. I have no additional comments on patch 2. >>> >>> Cheers, >>> >>> Eelco >>> >>> --- Documentation/topics/usdt-probes.rst | 1 + ofproto/ofproto-dpif-upcall.c| 42 +- utilities/automake.mk| 3 + utilities/usdt-scripts/flow_reval_monitor.py | 653 +++ 4 files changed, 693 insertions(+), 6 deletions(-) create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py diff --git a/Documentation/topics/usdt-probes.rst b/Documentation/topics/usdt-probes.rst index e527f43bab..a8da9bb1f7 100644 --- a/Documentation/topics/usdt-probes.rst +++ b/Documentation/topics/usdt-probes.rst @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: - dpif_recv:recv_upcall - main:poll_block - main:run_start +- revalidate:flow_result - revalidate_ukey\_\_:entry - revalidate_ukey\_\_:exit - udpif_revalidator:start_dump >>> >>> You are missing the specific flow_result result section. This is >>> from the previous patch: >> >> D'oh! Thanks for catching it. I'll re-add it. >> >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. >>>- ``utilities/usdt-scripts/bridge_loop.bt`` >>> >>> >>> +probe revalidate:flow_result >>> +~ >>> + >>> +**Description**: >>> +This probe is triggered when the revalidator decides whether or >>> not to >>> +revalidate a flow. ``reason`` is an enum that denotes that either >>> the flow >>> +is being kept, or the reason why the flow is being deleted. The >>> +``flow_reval_monitor.py`` script uses this probe to notify users >>> when flows >>> +matching user-provided criteria are deleted. >>> + >>> +**Arguments**: >>> + >>> +- *arg0*: ``(enum
