[ovs-dev] [PATCH net-next] openvswitch: Pass on secpath details for internal port rx.
Clearing the secpath for internal ports will cause packet drops when ipsec offload or early SW ipsec decrypt are used. Systems that rely on these will not be able to actually pass traffic via openvswitch. There is still an open issue for a flow miss packet - this is because we drop the extensions during upcall and there is no facility to restore such data (and it is non-trivial to add such functionality to the upcall interface). That means that when a flow miss occurs, there will still be packet drops. With this patch, when a flow is found then traffic which has an associated xfrm extension will properly flow. Signed-off-by: Aaron Conole --- net/openvswitch/vport-internal_dev.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c index 5858d65ea1a9..2412d7813d24 100644 --- a/net/openvswitch/vport-internal_dev.c +++ b/net/openvswitch/vport-internal_dev.c @@ -195,7 +195,6 @@ static int internal_dev_recv(struct sk_buff *skb) skb_dst_drop(skb); nf_reset_ct(skb); - secpath_reset(skb); skb->pkt_type = PACKET_HOST; skb->protocol = eth_type_trans(skb, netdev); -- 2.46.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto/ofproto: Initialize learn add rule flag.
Mike Ovsiannikov writes:
> Hi Aron,
>
> Here is the stack trace of the failure / segmentation fault:
>
> #0 0x7f33b423c1c1 in ofproto_flow_mod_learn_finish (ofm=0x230db00,
> orig_ofproto=orig_ofproto@entry=0x1d39220) at ofproto/ofproto-provider.h:509
> #1 0x7f33b4241e9a in ofproto_dpif_xcache_execute (ofproto=0x1d39200,
> xcache=, stats=0x7fff8e76eb10) at ofproto/ofproto-dpif.c:4966
> #2 0x7f33b4247723 in nxt_resume (ofproto_=0x1d39220, pin=0x7fff8e76f7b0)
> at
> ofproto/ofproto-dpif.c:5811
> #3 0x7f33b42308dd in handle_nxt_resume (ofconn=ofconn@entry=0x20689a0,
> oh=) at ofproto/ofproto.c:3851
> #4 0x7f33b423f429 in handle_single_part_openflow
> (type=OFPTYPE_NXT_RESUME,
> oh=, ofconn=0x20689a0) at ofproto/ofproto.c:8823
> #5 handle_openflow (ofconn=0x20689a0, msgs=0x7fff8e7707a0) at
> ofproto/ofproto.c:8959
> #6 0x7f33b422cce0 in ofconn_run (handle_openflow=0x7f33b423e560
> , ofconn=0x20689a0) at ofproto/connmgr.c:1329
> #7 connmgr_run (mgr=0x1cfc140,
> handle_openflow=handle_openflow@entry=0x7f33b423e560 ) at
> ofproto/connmgr.c:356
> #8 0x7f33b4236f70 in ofproto_run (p=0x1d39220) at ofproto/ofproto.c:1990
> #9 0x004101fc in bridge_run__ () at vswitchd/bridge.c:3287
> #10 0x0041615e in bridge_run () at vswitchd/bridge.c:3346
> #11 0x0040be05 in main (argc=, argv=) at
> vswitchd/ovs-vswitchd.c:132
>
> The learn.ofm structure appears to be valid at this point, except that
> learn_adds_rule and
> old_rules new_rules fields does not appear to be initialized:
>
> (gdb) p entry->learn.ofm[0]
> $3 = {
> temp_rule = 0x15fb520,
> criteria = {
> table_id = 1 '\001',
> cr = {
> node = {
> prev = 0x15bd570,
> next = {
> p = 0x15bd570
> }
> },
> priority = 31,
> cls_match = {
> p = 0x0
> },
> match = {
> {
> {
> flow = 0x21b77a0,
> mask = 0x21b77d0
> },
> flows = {0x21b77a0, 0x21b77d0}
> },
> tun_md = 0x0
> }
> },
> version = 18446744073709551614,
> cookie = 0,
> cookie_mask = 0,
> out_port = 65535,
> out_group = 4294967295,
> include_hidden = false,
> include_readonly = false
> },
> conjs = 0x0,
> n_conjs = 0,
> command = 2,
> modify_cookie = true,
> modify_may_add_flow = true,
> modify_keep_counts = true,
> event = 492535614,
> table_id = 1 '\001',
> version = 492535619,
> learn_adds_rule = 2,
> old_rules = {
> collection = {
> objs = 0x1,
> n = 492535617,
> capacity = 5,
> stub = {0x1d5b7f41, 0x30, 0x0, 0x0, 0x0}
> }
> },
> new_rules = {
> collection = {
> objs = 0x0,
> n = 0,
> capacity = 0,
> stub = {0x0, 0x0, 0x0, 0x6, 0x0}
> }
> }
> }
>
> In this particular case the following line in ofproto_flow_mod_learn_finish()
> results in NULL
> pointer that later is de-referenced in subsequent if statement.
> struct rule *rule = rule_collection_rules(&ofm->new_rules)[0];
>
> As far as I can tell, presently the flag initialization only exists in one
> place: inside packet_xlate
> (). Though it does not appear to me that packet_xlate() is guaranteed be
> invoked after the
> structure is allocated inside xlate_learn_action().
> The old and new rules collections appears to be initialized by the following
> call chain, but it is
> not guaranteed that this call chain is always / unconditionally executed.
> ofproto_flow_mod_learn() /
> ofproto_flow_mod_learn_start() /
>ofproto_flow_mod_start() /
> rule_collection_init()
>
> I did notice that the learn_adds_rule flag was introduced by 1f4a89336682
> change that
> you’ve mentioned, though I’m not convinced that the flag is guaranteed to be
> always
> initialized.
Thanks for the details above. You're correct - it isn't being
initialized properly. However, this looks like it is happening with a
specific flow controller setup using some kind of continuation chain.
If you have that flow chain, maybe we can also emulate it in a test.
I'm actually concerned that we may have other uninitialized (or
incorrectly initialized) details.
> Thank you,
> — Mike.
>
> On Oct 29, 2024, at 6:59 AM, Aaron Conole wrote:
>
> !---|
> CAUTION: External Email
>
> |---!
>
> Mike Ovsiannikov writes:
>
> In
Re: [ovs-dev] [PATCH] ofproto/ofproto: Initialize learn add rule flag.
Mike Ovsiannikov writes:
> Initialize learn_adds_rule in order to prevent
> crash in ofproto_flow_mod_learn_finish() due to
> being invoked with uninitialized rules
> collections.
>
> Signed-off-by: Mike Ovsiannikov
>
> ---
Hi Mike,
I think this should have:
Fixes: 1f4a89336682 ("ofproto: Refactor packet_out handling.")
Also, I don't see many details on how we have uninitialized rules
collections. Is it possible to include a stack trace / steps to
reproduce the problem? Could it be that there is a race somewhere?
Is it during add/remove of some group? We shouldn't get into this case
without at least a valid rule, I think (at least it seems like all paths
need to cross the XC_LEARN / or a learn xlate test).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] meta-flow: Fix nw_frag mask while parsing from string.
Ilya Maximets writes:
> mf_from_frag_string() sets all the upper bits of the nw_frag to make
> sure the exact match check will pass. This was not taken into account
> while splitting nw_frag and IP TOS bits into separate fields and the
> mask clean up was removed from the cls_rule_set_frag_masked() which is
> now called match_set_nw_frag_masked(). This leads to the case where
> the match parsed from the string is not considered equal to the
> match parsed from the OpenFlow, due to difference in masks. And that
> is causing ovs-ofctl replace-flows to always replace flows that match
> on nw_frag, even if they are exactly the same triggering unnecessary
> flow table updates and revalidation.
>
> $ cat frag_flows.txt
> ip,in_port=1,nw_frag=yes actions=drop
>
> $ ovs-ofctl dump-flows --no-stat --no-names br0
> ip,in_port=1,nw_frag=yes actions=drop
>
> $ ovs-ofctl -vvconn replace-flows br0 frag_flows.txt 2>&1 | grep MOD
> NXT_FLOW_MOD: DEL_STRICT ip,in_port=1,nw_frag=yes actions=drop
> NXT_FLOW_MOD: ADDip,in_port=1,nw_frag=yes actions=drop
>
> Clear the extra mask bits while setting match/flow structure from the
> field to avoid the issue.
>
> The issue mainly affects non-exact matches 'non_later' and 'yes', since
> exact matches are special-handled in many places / considered equal to
> not having a mask at all.
>
> Note: ideally we would not use byte-sized is_all_ones() for exact match
> checking, but use field-specific checks instead. However, this leads
> to a much larger change throughout OVS code base and would be much
> harder to backport. For now, fixing the issue in the way the code was
> originally implemented.
>
> Fixes: 9e44d715638a ("Don't overload IP TOS with the frag matching bits.")
> Signed-off-by: Ilya Maximets
> ---
Clearly no one noticed it for a while :)
Also, thanks for fixing it in the `mf_set` rather than in
`match_set_nw_frag_masked`. The original `cls_rule_set_frag_masked` was
a bit messy due to the overload. Your fix continues to look clean to me.
Acked-by: Aaron Conole
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/6] conntrack: Correctly annotate conntrack member.
Ilya Maximets writes:
> On 10/9/24 22:31, Aaron Conole wrote:
>> Paolo Valerio writes:
>>
>>> While at it update no longer valid comment.
>>>
>>> Signed-off-by: Paolo Valerio
>>> ---
>>
>> Thanks for the work on this, Paolo. I've merged the series.
>>
>> Given there is something of a bugfix for this series, if you think it
>> should be backported to the other branches please chime in. There is
>> some churn in here that should be okay, but I'm hesitant to just do the
>> backport.
>
>
> Hi, Paolo and Aaron.
>
> There is a syntax error in this set that breaks Windows build:
>
> https://ci.appveyor.com/project/blp/ovs/builds/50766118/job/0r2xcuqvyb76x69b#L3172
>
> lib/conntrack.c(316): error C2059: syntax error: ':'
>
> Could you, please, take a look?
Hrrm... I guess it would prefer something like below. For some reason
both my appveyor and cirrus support got disabled in the repo I was using
so I didn't catch it.
---
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 0061a56364..f4b150bee5 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -313,7 +313,7 @@ zone_limit_get_limit(struct conntrack *ct, struct
conntrack_zone_limit *czl)
if (limit == ZONE_LIMIT_CONN_DEFAULT) {
atomic_read_relaxed(&ct->default_zone_limit, &limit);
-limit = limit ? : -1;
+limit = limit ? limit : -1;
}
return limit;
---
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/2] system-traffic: Do not rely on conncount for already tracked packets.
Ilya Maximets writes: > On 10/9/24 22:28, Aaron Conole wrote: >> Paolo Valerio writes: >> >>> As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT >>> result in the unexpected failure of the following tests: >>> >>> conntrack - multiple zones, local >>> conntrack - multi-stage pipeline, local >>> conntrack - can match and clear ct_state from outside OVS >>> >>> this happens because the nf_conncount turns on connection tracking and >>> the above tests rely on this side effect. However, this behavior may >>> be corrected in the kernel, which could, in turn, cause the tests to >>> fail. >>> >>> The patch removes the assumption by adding iptables rules to attach >>> an nf_conn template to the skb resulting tracked once hit the OvS >>> pipeline. >>> >>> While at it, introduce $HAVE_IPTABLES and skip tests if iptables >>> binary is not present. >>> >>> Reported-by: Xin Long >>> Reported-at: https://issues.redhat.com/browse/FDP-708 >>> Signed-off-by: Paolo Valerio >>> Acked-by: Eelco Chaudron >>> --- >> >> Thanks to Paolo, Simon, and Eelco - merged. > > Thanks, Aaron! AFAIU, we need these changes on all supported branches. > Could you, please, check and backport? Will do. > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-ctl: Add run_sb_relay_ovsdb subcommand.
Michał Nasiadka writes:
> It was just my first patch and didn’t know what I should do with those
> robot posted failures (basically missing signed-off-by).
> What should I do now to get that moved forward please?
It would be best to repost (v2) with the correct signed-off-by line, I
think. Replying with a new signoff will have patchwork include both.
But an OVN maintainer may have a different opinion.
> Best regards,
>
> Michal
>
>> On 27 Sep 2024, at 21:49, Aaron Conole wrote:
>>
>> Michał Nasiadka writes:
>>
>>> This patch adds missing subcommand for running SB Relay in foreground.
>>>
>>> Signed-off-by: Michał Nasiadka
>>> ---
>>
>> Seems these were done as top-post replied to the robot. Was there any
>> reason why?
>>
>>> utilities/ovn-ctl | 8
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
>>> index d7ca9e758..c0b797273 100755
>>> --- a/utilities/ovn-ctl
>>> +++ b/utilities/ovn-ctl
>>> @@ -499,6 +499,11 @@ run_sb_ovsdb() {
>>>start_sb_ovsdb
>>> }
>>>
>>> +run_sb_relay_ovsdb() {
>>> +DB_SB_RELAY_DETACH=no
>>> +start_sb_relay_ovsdb
>>> +}
>>> +
>>> run_ic_nb_ovsdb() {
>>>DB_IC_NB_DETACH=no
>>>start_ic_nb_ovsdb
>>> @@ -1469,6 +1474,9 @@ case $command in
>>>run_sb_ovsdb)
>>>run_sb_ovsdb
>>>;;
>>> +run_sb_relay_ovsdb)
>>> +run_sb_relay_ovsdb
>>> +;;
>>>run_ic_nb_ovsdb)
>>>run_ic_nb_ovsdb
>>>;;
>>> --
>>> 2.34.1
>>>
>>> On 11 Sep 2024, at 14:51, Michał Nasiadka wrote:
>>>
>>> This patch adds missing subcommand for running SB Relay in foreground.
>>>
>>> Signed-off-by: Michał Nasiadka
>>> ---
>>> utilities/ovn-ctl | 8
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
>>> index d7ca9e758..c0b797273 100755
>>> --- a/utilities/ovn-ctl
>>> +++ b/utilities/ovn-ctl
>>> @@ -499,6 +499,11 @@ run_sb_ovsdb() {
>>> start_sb_ovsdb
>>> }
>>>
>>> +run_sb_relay_ovsdb() {
>>> +DB_SB_RELAY_DETACH=no
>>> +start_sb_relay_ovsdb
>>> +}
>>> +
>>> run_ic_nb_ovsdb() {
>>> DB_IC_NB_DETACH=no
>>> start_ic_nb_ovsdb
>>> @@ -1469,6 +1474,9 @@ case $command in
>>> run_sb_ovsdb)
>>> run_sb_ovsdb
>>> ;;
>>> +run_sb_relay_ovsdb)
>>> +run_sb_relay_ovsdb
>>> +;;
>>> run_ic_nb_ovsdb)
>>> run_ic_nb_ovsdb
>>> ;;
>>> --
>>> 2.34.1
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/6] conntrack: Correctly annotate conntrack member.
Paolo Valerio writes: > While at it update no longer valid comment. > > Signed-off-by: Paolo Valerio > --- Thanks for the work on this, Paolo. I've merged the series. Given there is something of a bugfix for this series, if you think it should be backported to the other branches please chime in. There is some churn in here that should be okay, but I'm hesitant to just do the backport. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 1/2] system-traffic: Do not rely on conncount for already tracked packets.
Paolo Valerio writes: > As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT > result in the unexpected failure of the following tests: > > conntrack - multiple zones, local > conntrack - multi-stage pipeline, local > conntrack - can match and clear ct_state from outside OVS > > this happens because the nf_conncount turns on connection tracking and > the above tests rely on this side effect. However, this behavior may > be corrected in the kernel, which could, in turn, cause the tests to > fail. > > The patch removes the assumption by adding iptables rules to attach > an nf_conn template to the skb resulting tracked once hit the OvS > pipeline. > > While at it, introduce $HAVE_IPTABLES and skip tests if iptables > binary is not present. > > Reported-by: Xin Long > Reported-at: https://issues.redhat.com/browse/FDP-708 > Signed-off-by: Paolo Valerio > Acked-by: Eelco Chaudron > --- Thanks to Paolo, Simon, and Eelco - merged. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] checkpatch: Add new check-authors-file option to checkpatch.py.
Eelco Chaudron writes: > This patch adds a new option, --check-authors-file, to the checkpatch > tool to help OVS maintainers check for missing authors in the > AUTHORS.rst file. > > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole > Signed-off-by: Eelco Chaudron > --- Thanks Eelco - applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] selinux: Add missing permissions for netlink_rdma_socket.
Roi Dayan via dev writes: > After testing with DPDK found netlink_rdma_socket missing > permissions 'getattr' and 'getopt' in the audit logs. > > Signed-off-by: Roi Dayan > --- Thanks for the patch - applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Improve load balancing in dp_hash select groups.
ber of backends. The standard
> deviation also doesn't go down that much with higher coefficient.
>
> The data is aggregated for up to 64 backends because with higher
> numbers we're getting close to the cap of 256 hashes, so deviation
> increases. However, the load difference with so many backends should
> have lower impact on a cluster in general. The change improves the
> cases with 64+ backends, but to get very good numbers we'll need to
> consider moving the upper limit for the hash space, which is a separate
> change to think about.
>
> Added test checks that standard deviation of the load distribution
> between buckets is below 12.5% for all cases up to 64 buckets. It's
> just a pretty number that I've chosen that should be IMO good enough.
>
> Note: when number of buckets is a power of 2, then we have an ideal
> distribution with zero deviation, because hashes can be evenly mapped
> to buckets.
>
> Note 2: The change doesn't affect distribution for 4 or less backends,
> since there are still minimum 16 hashes for those groups.
>
> Signed-off-by: Ilya Maximets
> ---
>
> Not sure if this should be considered as a bug fix and be backported.
> Thoughts on this are welcome.
I think it should be okay to backport. It isn't a new feature.
Acked-by: Aaron Conole
> ofproto/ofproto-dpif.c | 6 +++--
> tests/ofproto-dpif.at | 54 ++
> 2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4659d8a3b..bf43d5d4b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5341,8 +5341,10 @@ group_setup_dp_hash_table(struct group_dpif *group,
> size_t max_hash)
> min_weight, total_weight);
>
> uint64_t min_slots = DIV_ROUND_UP(total_weight, min_weight);
> -uint64_t min_slots2 = ROUND_UP_POW2(min_slots);
> -uint64_t n_hash = MAX(16, min_slots2);
> +uint64_t min_slots2 =
> +MAX(min_slots, MIN(n_buckets * 4, MAX_SELECT_GROUP_HASH_VALUES));
> +uint64_t min_slots3 = ROUND_UP_POW2(min_slots2);
> +uint64_t n_hash = MAX(16, min_slots3);
> if (n_hash > MAX_SELECT_GROUP_HASH_VALUES ||
> (max_hash != 0 && n_hash > max_hash)) {
> VLOG_DBG(" Too many hash values required: %"PRIu64, n_hash);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 12cb7f7a6..1c99dc3bb 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1238,6 +1238,60 @@ bucket3 >= 500
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - select group with dp_hash and equal weights])
> +
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 10
> +
> +AT_CHECK([ovs-appctl vlog/set ofproto_dpif:file:dbg vconn:file:info])
> +
> +AT_DATA([stddev.awk], [
> + {
> +# $1 (target) is a mean value, because all weights are the same.
> +# $2 (hits) is an actual number of hashes assigned to this bucket.
> +n_hashes += $2
> +n_buckets++
> +sum_sq_diff += ($2 - $1) * ($2 - $1)
> + }
> + END {
> +mean = n_hashes / n_buckets
> +stddev = sqrt(sum_sq_diff / n_buckets)
> +stddevp = stddev * 100 / mean
> +
> +print "hashes:", n_hashes, "buckets:", n_buckets
> +print "mean:", mean, "stddev:", stddev, "(", stddevp, "% )"
> +
> +# Make sure that standard deviation of load between buckets is below
> 12.5%.
> +# Note: it's not a strict requirement, but a good number that passes
> tests.
> +if (stddevp <= 12.5) { print "PASS" }
> +else { print "FAIL" }
> + }
> +])
> +
> +m4_define([CHECK_DISTRIBUTION], [
> + AT_CHECK([tail -n $1 ovs-vswitchd.log | grep 'ofproto_dpif|DBG|.*Bucket' \
> +| sed 's/.*target=\([[0-9\.]]*\) hits=\([[0-9]]*\)/\1 \2/' \
> +| awk -f stddev.awk], [0], [stdout])
> + AT_CHECK([grep -q "buckets: $2" stdout])
> + AT_CHECK([grep -q 'PASS' stdout])
> +])
> +
> +m4_define([OF_GROUP], [group_id=$1,type=select,selection_method=dp_hash])
> +m4_define([OFG_BUCKET], [bucket=weight=$1,output:10])
> +
> +dnl Test load distribution in groups with up to 64 equally weighted buckets.
> +m4_define([OFG_BUCKETS], [OFG_BUCKET(100)])
> +m4_for([id], [1], [64], [1], [
> + get_log_next_line_num
> + AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
> +"OF_GROUP(id),OFG_BUCKETS()"])
> + CHECK_DISTRIBUTION([+$LINENUM], [id])
> + m4_append([OFG_BUCKETS], [,OFG_BUCKET(100)])
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto-dpif - select group with explicit dp_hash selection
> method])
>
> OVS_VSWITCHD_START
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/6] conntrack: Add zone limit coverage counter.
Paolo Valerio writes:
> Similarly to what it's done for conntrack_full, add
> conntrack_zone_full increased when new entries are not added due to
> reaching the zone limit.
>
> Signed-off-by: Paolo Valerio
> ---
Acked-by: Aaron Conole
> lib/conntrack.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e96779e68..8cf200e06 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_zone_full);
>
> struct conn_lookup_ctx {
> struct conn_key key;
> @@ -918,6 +919,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> struct zone_limit *zl = zone_limit_lookup_or_default(ct,
> ctx->key.zone);
> if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
> +COVERAGE_INC(conntrack_zone_full);
> return nc;
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/6] conntrack: Correctly annotate conntrack member.
Paolo Valerio writes:
> While at it update no longer valid comment.
>
> Signed-off-by: Paolo Valerio
> ---
Acked-by: Aaron Conole
> lib/conntrack-private.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 71367f211..6c65caa07 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -199,11 +199,12 @@ enum ct_ephemeral_range {
> FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_JOIN(idx, __COUNTER__))
>
> struct conntrack {
> -struct ovs_mutex ct_lock; /* Protects 2 following fields. */
> +struct ovs_mutex ct_lock; /* Protects the following fields. */
> struct cmap conns[UINT16_MAX + 1] OVS_GUARDED;
> -struct rculist exp_lists[N_EXP_LISTS];
> +struct rculist exp_lists[N_EXP_LISTS] OVS_GUARDED;
> struct cmap zone_limits OVS_GUARDED;
> struct cmap timeout_policies OVS_GUARDED;
> +
> uint32_t hash_basis; /* Salt for hashing a connection key. */
> pthread_t clean_thread; /* Periodically cleans up connection tracker. */
> struct latch clean_thread_exit; /* To destroy the 'clean_thread'. */
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] checkpatch: Add new check-authors-file option to checkpatch.py.
Eelco Chaudron writes:
> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron
> ---
> v3: - Also include co-authors in the check.
> - Only report the end, when all patches are checked.
> - Fixed spelling mistake.
> - Determine git root directory for AUTHORS.rst location.
>
> v4: - Added a test case.
> ---
Thanks for the quick follow up Eelco.
> tests/checkpatch.at | 21 +
> utilities/checkpatch.py | 68 +++--
> 2 files changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index 34971c514..fa179c707 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -634,3 +634,24 @@ try_checkpatch \
> "--skip-committer-signoff"
>
> AT_CLEANUP
> +
> +AT_SETUP([checkpatch - AUTHORS.rst existence])
> +
> +try_checkpatch \
> + "Author: A
> +Commit: A
> +Subject: netdev: Subject.
> +
> +Signed-off-by: A " \
> +""
> +
> +try_checkpatch \
> + "Author: A
> +Commit: A
> +Subject: netdev: Subject.
> +
> +Signed-off-by: A " \
> +"WARNING: Author 'A ' is not in the AUTHORS.rst file!" \
> +"-a"
> +
> +AT_CLEANUP
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..73c20f804 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,12 +28,14 @@ __errors = 0
> __warnings = 0
> empty_return_check_state = 0
> print_file_name = None
> +check_authors_file = False
> checking_file = False
> total_line = 0
> colors = False
> spellcheck = False
> quiet = False
> spell_check_dict = None
> +missing_authors = []
>
>
> def open_spell_check_dict():
> @@ -860,9 +862,41 @@ def run_subject_checks(subject, spellcheck=False):
> return warnings
>
>
> +def get_top_directory():
> +with os.popen('git rev-parse --show-toplevel') as pipe:
> +path = pipe.read()
> +
> +if path:
> +return path.strip()
> +
> +return "."
> +
> +
> +def do_authors_exist(authors):
> +authors = list(set(authors))
> +missing_authors = []
> +
> +try:
> +with open(get_top_directory() + "/AUTHORS.rst", "r") as file:
> +file_content = file.read()
> +for author in authors:
> +m = re.search(r'<(.*?)>', author)
> +if not m:
> +continue
> +pattern = r'\b' + re.escape(m.group(1)) + r'\b'
> +if re.search(pattern, file_content) is None:
> +missing_authors.append(author)
> +
> +except FileNotFoundError:
> +print_error("Could not open AUTHORS.rst in '%s/'!" %
> +get_top_directory())
> +
> +return missing_authors
> +
> +
> def ovs_checkpatch_parse(text, filename, author=None, committer=None):
> global print_file_name, total_line, checking_file, \
> -empty_return_check_state
> +empty_return_check_state, missing_authors
>
> PARSE_STATE_HEADING = 0
> PARSE_STATE_DIFF_HEADER = 1
> @@ -977,6 +1011,11 @@ def ovs_checkpatch_parse(text, filename, author=None,
> committer=None):
>"who are not authors or co-authors or "
>"committers: %s"
>% ", ".join(extra_sigs))
> +
> +if check_authors_file:
> +missing_authors = do_authors_exist(missing_authors +
> + co_authors + [author])
> +
> elif is_committer.match(line):
> committer = is_committer.match(line).group(2)
> elif is_author.match(line):
> @@ -1067,6 +1106,8 @@ Input options:
>
> Check options:
> -h|--help This help message
> +-a|--check-authors-fileCheck AUTHORS file for existence of the
> authors.
> + Should be used by commiters only!
> -b|--skip-block-whitespace Skips the if/while/for whitespace tests
> -l|--skip-leading-whitespace Skips the leading whitespace test
> -q|--quiet Only print error and warning information
> @@ -1089,6 +1130,19 @@ def ovs_checkpatch_print_result():
> print("Lines checked: %d, no obvious problems found\n" %
> (total_line))
>
>
> +def ovs_checkpatch_print_missing_authors():
> +if missing_authors:
> +if len(missing_authors) == 1:
> +print_warning("Author '%s' is not in the AUTHORS.rst file!"
> + % missing_authors[0])
> +else:
> +print_warning("Authors '%s' are not in the AUTHORS.rst file!"
> + % ', '.join(missing_authors))
> +return True
> +
> +return False
> +
> +
> def ovs_checkpatch_file(filename):
> try:
> mail = email.mess
Re: [ovs-dev] [PATCH] Revert "ci: Use sarif-tools v3.0.1 due to issues in earlier versions."
Ilya Maximets writes: > On 9/28/24 14:36, Aaron Conole wrote: >> It seems that the sarif-tools package version 3.0+ cause some kind >> of instability with the build system. Until it has a proper root >> cause, we shouldn't try to apply any fixes. >> >> This reverts commit f2ab45c66e2fe536e98f7f45d107d9f8c3209437. >> >> Signed-off-by: Aaron Conole >> --- >> .ci/linux-prepare.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh >> index b537163b8e..5f8a1db6af 100755 >> --- a/.ci/linux-prepare.sh >> +++ b/.ci/linux-prepare.sh >> @@ -23,7 +23,7 @@ cd .. >> # https://github.com/pypa/pip/issues/10655 >> pip3 install --disable-pip-version-check --user wheel >> pip3 install --disable-pip-version-check --user \ >> -flake8 netaddr pyparsing sarif-tools>=3.0.1 sphinx setuptools >> +flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools >> >> # Install python test dependencies >> pip3 install -r python/test_requirements.txt > > Thanks, Aaron! > > Acked-by: Ilya Maximets > > The fix should be released somewhere soon, but I think we'll need > a bit more testing before bringing 3.0.3 (or whatever it will be > named) into CI. Agreed, and I've applied this patch to the tree. Thanks Eelco, and Ilya. > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] testing: Document the 0-day robot support.
Eelco Chaudron writes: > On 28 Sep 2024, at 15:03, Aaron Conole wrote: > >> The 0-day robot has been testing patches for 6 years, and we've >> had support for other labs to integrate for 3. However, this >> isn't well documented, and has made it difficult for others to >> know how they can contribute to testing. To that end, this >> patch introduces some documentation for the 0-day robot and >> how to integrate into the patch reporting process. > > Thanks for adding this documentation, some small comments below. > > Cheers, > > Eelco > >> Signed-off-by: Aaron Conole >> --- >> .../contributing/submitting-patches.rst | 3 +- >> Documentation/topics/testing.rst | 105 ++ >> 2 files changed, 107 insertions(+), 1 deletion(-) >> >> diff --git >> a/Documentation/internals/contributing/submitting-patches.rst >> b/Documentation/internals/contributing/submitting-patches.rst >> index 8a8bc11b0a..c01ac7bbdc 100644 >> --- a/Documentation/internals/contributing/submitting-patches.rst >> +++ b/Documentation/internals/contributing/submitting-patches.rst >> @@ -70,7 +70,8 @@ Testing is also important: >> >> If you are using GitHub, then you may utilize the GitHub Actions CI build >> systems. They will run some of the above tests automatically >> -when you push changes to your repository. >> +when you push changes to your repository. See the "Continuous Integration" >> +section in :doc:`/topics/testing` for details on continuous integration. >> >> Email Subject >> - >> diff --git a/Documentation/topics/testing.rst >> b/Documentation/topics/testing.rst >> index dcf10a4db2..49ee7d7ffa 100644 >> --- a/Documentation/topics/testing.rst >> +++ b/Documentation/topics/testing.rst >> @@ -545,3 +545,108 @@ Once you are done with experimenting you can >> tear down setup with:: >> >> Sometimes deployment of Proof of Concept may fail, if, for example, VMs >> don't have network reachability to the Internet. >> + >> + >> +Continuous Integration >> +-- >> + >> +The Open vSwitch project can make use of multiple public and hosted >> +CI services to help developers ensure their patches don't introduce >> +additional regressions. Each service requires different forms of >> +configuration, and for the supported services the configuration >> +file(s) to automatically build Open vSwitch with various build >> +configurations and run the testsuites is/are provided in the > > I think test suites are two words. I guess it could be either. From what I see in the documentation / comments, we use 'testsuite' more often than 'test suite'. I can switch it in v2. >> +repostiory. For example, the GitHub Actions builds will be performed >> +with gcc, clang, sparse, including DPDK, etc. with the -Werror >> +compiler flag included. Therefore, the build will fail if a new >> +warning gets introduced by a change. >> + >> +Each ci system uses a different method of enablement, but most are available > > CI with captials as in the rest of the text? Ack. >> +from the GitHub settings page. By default, Open vSwitch includes a GitHub >> +actions running configuration, and this will automatically email >> the repository >> +owner. >> + >> +Currently, Open vSwitch project enables the following public CI >> services along >> +with the appropriate configuration settings:: >> + >> + - AppVeyor: appveyor.yml >> + - Cirrus-CI: .cirrus.yml >> + - GitHub Actions: .github/workflows >> + >> +GitHub Actions is available without any additional configuration. >> + >> +Additionally, as each patch is posted to the mailing list, the public CI >> +machinery will run additional tests on the patches and report results. >> + >> +Public report / Private lab hybrid testing >> +-- >> + >> +The Open vSwitch project makes use of the ozlabs patchwork instance > > Maybe put in a link to the instance? Good idea. >> +to track patch status and management. This patch tracking tool >> +provides information to maintainers on the state of patches proposed >> +for Open vSwitch. The CI process for Open vSwitch makes use of the >> +checks feature of the ozlabs patchwork instance. These allow developers > > OzLabs > >> +and maintainers to see what tests have been run, and report pass / fail >> +criteria. >> + >> +In order to know that a patch or series is ready for tes
[ovs-dev] [PATCH] testing: Document the 0-day robot support.
The 0-day robot has been testing patches for 6 years, and we've had support for other labs to integrate for 3. However, this isn't well documented, and has made it difficult for others to know how they can contribute to testing. To that end, this patch introduces some documentation for the 0-day robot and how to integrate into the patch reporting process. Signed-off-by: Aaron Conole --- .../contributing/submitting-patches.rst | 3 +- Documentation/topics/testing.rst | 105 ++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/Documentation/internals/contributing/submitting-patches.rst b/Documentation/internals/contributing/submitting-patches.rst index 8a8bc11b0a..c01ac7bbdc 100644 --- a/Documentation/internals/contributing/submitting-patches.rst +++ b/Documentation/internals/contributing/submitting-patches.rst @@ -70,7 +70,8 @@ Testing is also important: If you are using GitHub, then you may utilize the GitHub Actions CI build systems. They will run some of the above tests automatically -when you push changes to your repository. +when you push changes to your repository. See the "Continuous Integration" +section in :doc:`/topics/testing` for details on continuous integration. Email Subject - diff --git a/Documentation/topics/testing.rst b/Documentation/topics/testing.rst index dcf10a4db2..49ee7d7ffa 100644 --- a/Documentation/topics/testing.rst +++ b/Documentation/topics/testing.rst @@ -545,3 +545,108 @@ Once you are done with experimenting you can tear down setup with:: Sometimes deployment of Proof of Concept may fail, if, for example, VMs don't have network reachability to the Internet. + + +Continuous Integration +-- + +The Open vSwitch project can make use of multiple public and hosted +CI services to help developers ensure their patches don't introduce +additional regressions. Each service requires different forms of +configuration, and for the supported services the configuration +file(s) to automatically build Open vSwitch with various build +configurations and run the testsuites is/are provided in the +repostiory. For example, the GitHub Actions builds will be performed +with gcc, clang, sparse, including DPDK, etc. with the -Werror +compiler flag included. Therefore, the build will fail if a new +warning gets introduced by a change. + +Each ci system uses a different method of enablement, but most are available +from the GitHub settings page. By default, Open vSwitch includes a GitHub +actions running configuration, and this will automatically email the repository +owner. + +Currently, Open vSwitch project enables the following public CI services along +with the appropriate configuration settings:: + + - AppVeyor: appveyor.yml + - Cirrus-CI: .cirrus.yml + - GitHub Actions: .github/workflows + +GitHub Actions is available without any additional configuration. + +Additionally, as each patch is posted to the mailing list, the public CI +machinery will run additional tests on the patches and report results. + +Public report / Private lab hybrid testing +-- + +The Open vSwitch project makes use of the ozlabs patchwork instance +to track patch status and management. This patch tracking tool +provides information to maintainers on the state of patches proposed +for Open vSwitch. The CI process for Open vSwitch makes use of the +checks feature of the ozlabs patchwork instance. These allow developers +and maintainers to see what tests have been run, and report pass / fail +criteria. + +In order to know that a patch or series is ready for testing, the +Open vSwitch project makes use of the "0-day Robot", which is a hosted +jenkins instance running the pw-ci_ scripts. These can monitor a +running patchwork instance for new patches and submit the patch details +to other build systems, like jenkins. + +Once a patch is tested, it would be good to report the results. To this +end, the Open vSwitch "0-day Robot" will accept emails sent to +ovs-bu...@openvswitch.org formatted in the correct way to be reflected +on this page. This allows any lab to contribute to the testing and +validation of patches. Note that the ovs-build list participation +requires subscribing the reporting email account to the list. + +To report a test status to a particular patch, send exactly one email to +the mailing formatted as such:: + + From: your email + To: ovs-bu...@openvswitch.org + Date: Mon, 28 Jun 2021 00:00:00 + + Subject: |STATUS| pwPATCHID commit subject + + Test-Label: your-robot-or-test-name + Test-Status: STATUS + http://patchwork.ozlabs.org/api/patches/PATCHID/ + + _ONE LINE DESCRIPTION_ + + Addtional details + +In the above example, the STATUS is one of "success" "warning" "fail" depending +on the outcome of the test. This value is case-sensitive. PATCHID should be +the pat
[ovs-dev] [PATCH] Revert "ci: Use sarif-tools v3.0.1 due to issues in earlier versions."
It seems that the sarif-tools package version 3.0+ cause some kind of instability with the build system. Until it has a proper root cause, we shouldn't try to apply any fixes. This reverts commit f2ab45c66e2fe536e98f7f45d107d9f8c3209437. Signed-off-by: Aaron Conole --- .ci/linux-prepare.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh index b537163b8e..5f8a1db6af 100755 --- a/.ci/linux-prepare.sh +++ b/.ci/linux-prepare.sh @@ -23,7 +23,7 @@ cd .. # https://github.com/pypa/pip/issues/10655 pip3 install --disable-pip-version-check --user wheel pip3 install --disable-pip-version-check --user \ -flake8 netaddr pyparsing sarif-tools>=3.0.1 sphinx setuptools +flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools # Install python test dependencies pip3 install -r python/test_requirements.txt -- 2.46.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Use sarif-tools v3.0.1 due to issues in earlier versions.
Ilya Maximets writes:
> On 9/27/24 21:32, Ilya Maximets wrote:
>> On 9/27/24 21:31, Ilya Maximets wrote:
>>> On 9/27/24 21:27, Aaron Conole wrote:
>>>> Ilya Maximets writes:
>>>>
>>>>> On 9/27/24 15:41, Eelco Chaudron wrote:
>>>>>>
>>>>>>
>>>>>> On 27 Sep 2024, at 14:51, Aaron Conole wrote:
>>>>>>
>>>>>>> Eelco Chaudron writes:
>>>>>>>
>>>>>>>> Sarif-tools v3.0 introduced an issue that has been resolved in v3.0.1.
>>>>>>>> Ensure that v3.0.1 or higher is installed via pip.
>>>>>>>>
>>>>>>>> Fixes: 234e626198a4 ("ci: Use previous sarif-tools release due to
>>>>>>>> issue in latest release.")
>>>>>>>> Signed-off-by: Eelco Chaudron
>>>>>>>> ---
>>>>>>>
>>>>>>> Thanks, Eelco. Applied and backported to branch-3.4 and branch-3.3.
>>>>>>
>>>>>> Thanks Aaron for applying and doing the backport.
>>>>>
>>>>> Hi, Eelco and Aaron. There is something wrong with it still.
>>>>>
>>>>> The check failed on both main and branch-3.4 (3.3 is fine):
>>>>
>>>> O_O
>>>>
>>>> Well, I applied based on:
>>>> https://github.com/ovsrobot/ovs/actions/runs/11048312854
>>>>
>>>> So looks like Sarif recent versions could be unstable, maybe?
>>>
>>> Yeah, I did a couple re-runs in my fork and it seem to fail randomly,
>>> i.e. one re-run without any code changes can fail or succeed.
>>>
>>> Should the change be reverted back to 2.0.0 ?
>>>
>>> Also, sarif-tools>=3.0.1 in a shell command means "re-direct the output
>>> to the file named '=3.0.1'", so the patch itself is not correct.
>>
>> Note: it installs the latest 3.0.2, so it's not the cause of the failures.
>
> I opened an issue for now: https://github.com/microsoft/sarif-tools/issues/68
Thanks. I think a revert is appropriate. I will send one out.
>>
>>>
>>> Best regards, Ilya Maximets.
>>>
>>>>
>>>>> Check: exiting with return code 2 due to increase in issues at or above
>>>>> note severity
>>>>> error level: +0 -0 no changes
>>>>> warning level: +2 -2
>>>>> New issue "core.NullDereference ..." (36 occurrences)
>>>>> file:///home/runner/work/ovs/ovs/include/openvswitch/hmap.h:319
>>>>> file:///home/runner/work/ovs/ovs/include/openvswitch/list.h:262
>>>>> file:///home/runner/work/ovs/ovs/include/openvswitch/nsh.h:287
>>>>> ...
>>>>> New issue "core.UndefinedBinaryOperatorResult The left operand of '
>>>>> ..." (4 occurrences)
>>>>> file:///home/runner/work/ovs/ovs/lib/dpif-netdev.c:946
>>>>> file:///home/runner/work/ovs/ovs/lib/dpif-netdev.c:963
>>>>> file:///home/runner/work/ovs/ovs/lib/netlink-socket.c:240
>>>>> ...
>>>>> Eliminated issue "core.NullDereference Access to field ' ..."
>>>>> Eliminated issue "core.UndefinedBinaryOperatorResult The ..."
>>>>> note level: +0 -0 no changes
>>>>> all levels: +2 -2
>>>>>
>>>>> Best regards, Ilya Maximets.
>>>>
>>>
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Use sarif-tools v3.0.1 due to issues in earlier versions.
Eelco Chaudron writes:
> Sarif-tools v3.0 introduced an issue that has been resolved in v3.0.1.
> Ensure that v3.0.1 or higher is installed via pip.
>
> Fixes: 234e626198a4 ("ci: Use previous sarif-tools release due to
> issue in latest release.")
> Signed-off-by: Eelco Chaudron
> ---
Thanks, Eelco. Applied and backported to branch-3.4 and branch-3.3.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-ctl: Add run_sb_relay_ovsdb subcommand.
Michał Nasiadka writes:
> This patch adds missing subcommand for running SB Relay in foreground.
>
> Signed-off-by: Michał Nasiadka
> ---
Seems these were done as top-post replied to the robot. Was there any
reason why?
> utilities/ovn-ctl | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> index d7ca9e758..c0b797273 100755
> --- a/utilities/ovn-ctl
> +++ b/utilities/ovn-ctl
> @@ -499,6 +499,11 @@ run_sb_ovsdb() {
> start_sb_ovsdb
> }
>
> +run_sb_relay_ovsdb() {
> +DB_SB_RELAY_DETACH=no
> +start_sb_relay_ovsdb
> +}
> +
> run_ic_nb_ovsdb() {
> DB_IC_NB_DETACH=no
> start_ic_nb_ovsdb
> @@ -1469,6 +1474,9 @@ case $command in
> run_sb_ovsdb)
> run_sb_ovsdb
> ;;
> +run_sb_relay_ovsdb)
> +run_sb_relay_ovsdb
> +;;
> run_ic_nb_ovsdb)
> run_ic_nb_ovsdb
> ;;
> --
> 2.34.1
>
> On 11 Sep 2024, at 14:51, Michał Nasiadka wrote:
>
> This patch adds missing subcommand for running SB Relay in foreground.
>
> Signed-off-by: Michał Nasiadka
> ---
> utilities/ovn-ctl | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> index d7ca9e758..c0b797273 100755
> --- a/utilities/ovn-ctl
> +++ b/utilities/ovn-ctl
> @@ -499,6 +499,11 @@ run_sb_ovsdb() {
> start_sb_ovsdb
> }
>
> +run_sb_relay_ovsdb() {
> +DB_SB_RELAY_DETACH=no
> +start_sb_relay_ovsdb
> +}
> +
> run_ic_nb_ovsdb() {
> DB_IC_NB_DETACH=no
> start_ic_nb_ovsdb
> @@ -1469,6 +1474,9 @@ case $command in
> run_sb_ovsdb)
> run_sb_ovsdb
> ;;
> +run_sb_relay_ovsdb)
> +run_sb_relay_ovsdb
> +;;
> run_ic_nb_ovsdb)
> run_ic_nb_ovsdb
> ;;
> --
> 2.34.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Use sarif-tools v3.0.1 due to issues in earlier versions.
Ilya Maximets writes:
> On 9/27/24 15:41, Eelco Chaudron wrote:
>>
>>
>> On 27 Sep 2024, at 14:51, Aaron Conole wrote:
>>
>>> Eelco Chaudron writes:
>>>
>>>> Sarif-tools v3.0 introduced an issue that has been resolved in v3.0.1.
>>>> Ensure that v3.0.1 or higher is installed via pip.
>>>>
>>>> Fixes: 234e626198a4 ("ci: Use previous sarif-tools release due to
>>>> issue in latest release.")
>>>> Signed-off-by: Eelco Chaudron
>>>> ---
>>>
>>> Thanks, Eelco. Applied and backported to branch-3.4 and branch-3.3.
>>
>> Thanks Aaron for applying and doing the backport.
>
> Hi, Eelco and Aaron. There is something wrong with it still.
>
> The check failed on both main and branch-3.4 (3.3 is fine):
O_O
Well, I applied based on:
https://github.com/ovsrobot/ovs/actions/runs/11048312854
So looks like Sarif recent versions could be unstable, maybe?
> Check: exiting with return code 2 due to increase in issues at or above note
> severity
> error level: +0 -0 no changes
> warning level: +2 -2
> New issue "core.NullDereference ..." (36 occurrences)
> file:///home/runner/work/ovs/ovs/include/openvswitch/hmap.h:319
> file:///home/runner/work/ovs/ovs/include/openvswitch/list.h:262
> file:///home/runner/work/ovs/ovs/include/openvswitch/nsh.h:287
> ...
> New issue "core.UndefinedBinaryOperatorResult The left operand of ' ..." (4
> occurrences)
> file:///home/runner/work/ovs/ovs/lib/dpif-netdev.c:946
> file:///home/runner/work/ovs/ovs/lib/dpif-netdev.c:963
> file:///home/runner/work/ovs/ovs/lib/netlink-socket.c:240
> ...
> Eliminated issue "core.NullDereference Access to field ' ..."
> Eliminated issue "core.UndefinedBinaryOperatorResult The ..."
> note level: +0 -0 no changes
> all levels: +2 -2
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.
Eelco Chaudron writes:
> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron
> ---
> v2: Fixed partial match, and long argument check.
Hi Eelco,
During the review I had some other thoughts about the way this feature
would behave, and I think it would be good to add a test to the
checkpatch tests for it to cover the behavior.
> ---
> utilities/checkpatch.py | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..636634472 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,6 +28,7 @@ __errors = 0
> __warnings = 0
> empty_return_check_state = 0
> print_file_name = None
> +check_authors_file = False
> checking_file = False
> total_line = 0
> colors = False
> @@ -977,6 +978,21 @@ def ovs_checkpatch_parse(text, filename, author=None,
> committer=None):
>"who are not authors or co-authors or "
>"committers: %s"
>% ", ".join(extra_sigs))
> +
> +if check_authors_file or author:
Should this be 'and' instead of 'or'? All patches we receive will have
an author so from what I can tell this check is always running.
Also, we probably want to do the below for all the co_authors as well.
> +m = re.search(r'<(.*?)>', author)
> +if m:
> +try:
> +with open("AUTHORS.rst", "r") as file:
> +file_content = file.read()
> +pattern = r'\b' + re.escape(m.group(1)) +
> r'\b'
> +if re.search(pattern, file_content) is None:
> +print_error("Author '%s' is not in the "
> +"AUTHORS.rst file!" % author)
Maybe print_warn instead. I can imagine someone running this and adding
a patch where they add themselves to the AUTHORS.rst file in another
commit.
> +except FileNotFoundError:
> +print_error("Can't open AUTHORS.rst file in "
> +"current path!")
> +
This one I don't know - maybe we should make it so that we can use git
to find the AUTHORS.rst file. See the code snippet I give at the end
(it's not fully tested).
Also, maybe we want to delay printing these until the end of the patch.
For example, if someone does run this and adds themselves to AUTHORS.rst
as part of the patch the scan may not pick it up. None of the other
checks behave so differently between patch scan and file scan mode,
iiuc. If we delay scanning until the end we can avoid printing this in
the case that the AUTHORS.rst file gets modified in a later hunk.
WDYT?
> elif is_committer.match(line):
> committer = is_committer.match(line).group(2)
> elif is_author.match(line):
> @@ -1067,6 +1083,7 @@ Input options:
>
> Check options:
> -h|--help This help message
> +-a|--check-authors-fileCheck AUTHORS file for existense of author
s/existense/existence/
Maybe we should also flag somehow that this check should generally be
run by a committer.
> -b|--skip-block-whitespace Skips the if/while/for whitespace tests
> -l|--skip-leading-whitespace Skips the leading whitespace test
> -q|--quiet Only print error and warning information
> @@ -1138,9 +1155,10 @@ if __name__ == '__main__':
>sys.argv[1:])
> n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>
> -optlist, args = getopt.getopt(args, 'bhlstfSq',
> +optlist, args = getopt.getopt(args, 'abhlstfSq',
>["check-file",
> "help",
> + "check-authors-file",
> "skip-block-whitespace",
> "skip-leading-whitespace",
> "skip-signoff-lines",
> @@ -1157,6 +1175,8 @@ if __name__ == '__main__':
> if o in ("-h", "--help"):
> usage()
> sys.exit(0)
> +elif o in ("-a", "--check-authors-file"):
> +check_authors_file = True
> elif o in ("-b", "--skip-block-whitespace"):
> skip_block_whitespace_check = True
> elif o in ("-l", "--skip-leading-whitespace"):
--- 8<
def get_git_topdir(start_dir='.'):
"""Find the .git directory in the upper path"""
current_dir = os.path.abspath(start_dir)
while True:
git_dir = os.path.join(current_dir, '.git')
Re: [ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.
Simon Horman writes: > On Wed, Sep 25, 2024 at 10:39:04PM +0200, Eelco Chaudron wrote: >> >> >> On 25 Sep 2024, at 21:02, Aaron Conole wrote: >> >> > Eelco Chaudron writes: >> > >> >> This patch adds a new option, --check-authors-file, to the checkpatch >> >> tool to help OVS maintainers check for missing authors in the >> >> AUTHORS.rst file. >> >> >> >> Signed-off-by: Eelco Chaudron >> >> --- >> >> v2: Fixed partial match, and long argument check. >> > >> > Not sure about it. For example, is it really a problem with the patch >> > if the author doesn't appear? Maybe this could be a different checking >> > utility? I certainly don't think it would be an error in the patch. >> >> It’s not an error in the patch itself, but an error to be resolved >> before we apply it. >> >> As we have to duplicate a lot of infra for a stand-alone tool, and >> this option is not being enabled by default, I feel like it’s ok to >> enhance checkpatch. It also simplifies commiters the workflow, as >> now I can just run ‘checkpatch.py -3 -S -a’. >> >> Anyone else thoughts, feedback on this? > > The approach taken here seems reasonable to me. > Those who have a use for the feature can use checkpatch with > the option enabled, while those who don't won't be effected. > And the implementation can make use of existing infrastructure > in checkpatch.py. Okay, that makes sense. I'll reply to the original patch with one nit. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Add new check-authors-file option to checkpatch.py.
Eelco Chaudron writes:
> This patch adds a new option, --check-authors-file, to the checkpatch
> tool to help OVS maintainers check for missing authors in the
> AUTHORS.rst file.
>
> Signed-off-by: Eelco Chaudron
> ---
> v2: Fixed partial match, and long argument check.
Not sure about it. For example, is it really a problem with the patch
if the author doesn't appear? Maybe this could be a different checking
utility? I certainly don't think it would be an error in the patch.
> ---
> utilities/checkpatch.py | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 742a0bc47..636634472 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -28,6 +28,7 @@ __errors = 0
> __warnings = 0
> empty_return_check_state = 0
> print_file_name = None
> +check_authors_file = False
> checking_file = False
> total_line = 0
> colors = False
> @@ -977,6 +978,21 @@ def ovs_checkpatch_parse(text, filename, author=None,
> committer=None):
>"who are not authors or co-authors or "
>"committers: %s"
>% ", ".join(extra_sigs))
> +
> +if check_authors_file or author:
> +m = re.search(r'<(.*?)>', author)
> +if m:
> +try:
> +with open("AUTHORS.rst", "r") as file:
> +file_content = file.read()
> +pattern = r'\b' + re.escape(m.group(1)) +
> r'\b'
> +if re.search(pattern, file_content) is None:
> +print_error("Author '%s' is not in the "
> +"AUTHORS.rst file!" % author)
> +except FileNotFoundError:
> +print_error("Can't open AUTHORS.rst file in "
> +"current path!")
> +
> elif is_committer.match(line):
> committer = is_committer.match(line).group(2)
> elif is_author.match(line):
> @@ -1067,6 +1083,7 @@ Input options:
>
> Check options:
> -h|--help This help message
> +-a|--check-authors-fileCheck AUTHORS file for existense of author
> -b|--skip-block-whitespace Skips the if/while/for whitespace tests
> -l|--skip-leading-whitespace Skips the leading whitespace test
> -q|--quiet Only print error and warning information
> @@ -1138,9 +1155,10 @@ if __name__ == '__main__':
>sys.argv[1:])
> n_patches = int(numeric_options[-1][1:]) if numeric_options else 0
>
> -optlist, args = getopt.getopt(args, 'bhlstfSq',
> +optlist, args = getopt.getopt(args, 'abhlstfSq',
>["check-file",
> "help",
> + "check-authors-file",
> "skip-block-whitespace",
> "skip-leading-whitespace",
> "skip-signoff-lines",
> @@ -1157,6 +1175,8 @@ if __name__ == '__main__':
> if o in ("-h", "--help"):
> usage()
> sys.exit(0)
> +elif o in ("-a", "--check-authors-file"):
> +check_authors_file = True
> elif o in ("-b", "--skip-block-whitespace"):
> skip_block_whitespace_check = True
> elif o in ("-l", "--skip-leading-whitespace"):
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] selinux: Update policy file.
Roi Dayan via dev writes: > Failing to install the selinux policy file under RHEL9.1 with > error "Failed to resolve permission audit_write". > Checking online SELinux permissions, I found that those classes > don't support those permissions. So not sure how it's passing on > other distributions like RHEL8.2, maybe being ignored. > With this change I can install the policy file in RHEL8.2 and RHEL9.1. > > Signed-off-by: Roi Dayan > --- Also, applied and backported down to branch 2.17 Thanks Roi! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] selinux: Update policy file.
Roi Dayan via dev writes:
> Failing to install the selinux policy file under RHEL9.1 with
> error "Failed to resolve permission audit_write".
> Checking online SELinux permissions, I found that those classes
> don't support those permissions. So not sure how it's passing on
> other distributions like RHEL8.2, maybe being ignored.
> With this change I can install the policy file in RHEL8.2 and RHEL9.1.
>
> Signed-off-by: Roi Dayan
> ---
Fixes: 84d272330506 ("selinux: update policy to reflect non-root and dpdk
support")
It has been working for a while, but probably we can consider that the
`audit_write` should only have been applied to the capabilities class.
Thanks for the fix.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential NULL pointer access in log_flow_message().
"Phelan, Michael" writes: >> -Original Message- >> From: Aaron Conole >> Sent: Thursday, September 12, 2024 10:01 PM >> To: Phelan, Michael >> Cc: d...@openvswitch.org; Chaudron, Eelco ; Ilya >> Maximets >> Subject: Re: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential >> NULL pointer access in log_flow_message(). >> >> "Phelan, Michael" writes: >> >> >> -Original Message- >> >> From: Aaron Conole >> >> Sent: Tuesday, September 3, 2024 1:15 PM >> >> To: Phelan, Michael >> >> Cc: d...@openvswitch.org; Chaudron, Eelco ; Ilya >> >> Maximets >> >> Subject: Re: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix >> >> potential NULL pointer access in log_flow_message(). >> >> >> >> "Phelan, Michael" writes: >> >> >> >> > Hi Aaron, >> >> > I have looked into the failures on the CI and ran some tests >> >> > manually and tests 1010, 1015, 1020, 1025, 1030 and 1035 are >> >> > failing in make check even on the main branch. >> >> > >> >> > They fail in this block of calls from ofproto.at: >> >> > CHECK_FLOW_MONITORING([1.0], [OpenFlow10], [], >> >> NXST_FLOW_MONITOR) >> >> > CHECK_FLOW_MONITORING([1.1], [OpenFlow11], [ (OF1.1)], >> >> > NXST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.2], >> >> [OpenFlow12], [ >> >> > (OF1.2)], NXST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.3], >> >> > [OpenFlow13], [ (OF1.3)], ONFST_FLOW_MONITOR) >> >> > CHECK_FLOW_MONITORING([1.4], [OpenFlow14], [ (OF1.4)], >> >> > OFPST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.5], >> >> [OpenFlow15], [ >> >> > (OF1.5)], OFPST_FLOW_MONITOR) >> >> >> >> Thanks for narrowing it down. >> >> >> >> > The error in the log files is: >> >> > cat: ovs-ofctl.pid: No such file or directory >> >> >> >> Weird. I wonder if there is some issue with the way the ovs-ofctl >> >> pidfile is being referenced. That error is likely from (ofproto.at): >> >> >> >> on_exit 'kill `cat ovs-ofctl.pid`' >> >> >> >> Just a guess - could it be the way the autotools are expanding the >> >> on_exit line and it is trying to evaluate the `cat ovs-ofctl.pid` >> >> before ofctl has started? Can you try moving those lines in ofproto.at >> >> just >> to check if that could be it? >> > Hi Aaron, >> > Sorry for the delay in replying, I was OOO yesterday. >> > >> > I think ovs-ofctl is stopped before these checks are performed which is >> causing the failure. >> > >> > The section above these checks is: >> > OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) >> > OVS_VSWITCHD_STOP >> > AT_CLEANUP >> > ]) >> > >> > When I moved the CHECK_FLOW_MONITORING commands to before this >> block, the tests passed. >> >> Hrrm... That seems strange. Those are part of the define. I'm surprised >> that >> worked and didn't end in forever recursion. >> >> This might need some deeper looking. I wonder if moving those simply >> stopped running the checks. > > Sorry, yes looks like they're not checked after being moved. > >> >> Maybe there are some permissions or resource issues on your build system >> that are preventing the needed files from being generated? > > I don't think they are any permissions restricting these files from being > generated. > > What resources would you suggest I check? Is it possible someone can get access to debug it? There could be some issue with selinux(?), or with the /var/lib/jenkins/... stuff, or perhaps an rlimit that is being cascaded to the ovs build system children? >> >> > Thanks, >> > Michael. >> >> >> >> > Do you have any idea what might be the problem? >> >> > >> >> > Thanks, >> >> > Michael. >> >> >> -Original Message- >> >> >> From: Phelan, Michael >> >> >> Sent: Friday, August 30, 2024 4:12 PM >> >> >> To: Aaron Conole ; d...@openvswitch.org >> >> >> Cc: Chaudron, Eelco >> >> >> Subject: RE: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix >> >> >> potential NULL pointer access in log_flow_message(). &g
Re: [ovs-dev] [PATCH branch-2.17] ofproto-dpif-upcall: Avoid stale ukeys leaks.
Eelco Chaudron writes:
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan
> Co-authored-by: Han Zhou
> Co-authored-by: Roi Dayan
> Signed-off-by: Han Zhou
> Signed-off-by: Roi Dayan
> Signed-off-by: Eelco Chaudron
>
> NOTE: Backported a portion of
> 600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to
> OpenFlow.")
> to provide `strip_dp_hash` macro.
> ---
Acked-by: Aaron Conole
Thanks for doing this backport, Eelco!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential NULL pointer access in log_flow_message().
"Phelan, Michael" writes: >> -Original Message- >> From: Aaron Conole >> Sent: Tuesday, September 3, 2024 1:15 PM >> To: Phelan, Michael >> Cc: d...@openvswitch.org; Chaudron, Eelco ; Ilya >> Maximets >> Subject: Re: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential >> NULL pointer access in log_flow_message(). >> >> "Phelan, Michael" writes: >> >> > Hi Aaron, >> > I have looked into the failures on the CI and ran some tests manually >> > and tests 1010, 1015, 1020, 1025, 1030 and 1035 are failing in make >> > check even on the main branch. >> > >> > They fail in this block of calls from ofproto.at: >> > CHECK_FLOW_MONITORING([1.0], [OpenFlow10], [], >> NXST_FLOW_MONITOR) >> > CHECK_FLOW_MONITORING([1.1], [OpenFlow11], [ (OF1.1)], >> > NXST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.2], >> [OpenFlow12], [ >> > (OF1.2)], NXST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.3], >> > [OpenFlow13], [ (OF1.3)], ONFST_FLOW_MONITOR) >> > CHECK_FLOW_MONITORING([1.4], [OpenFlow14], [ (OF1.4)], >> > OFPST_FLOW_MONITOR) CHECK_FLOW_MONITORING([1.5], >> [OpenFlow15], [ >> > (OF1.5)], OFPST_FLOW_MONITOR) >> >> Thanks for narrowing it down. >> >> > The error in the log files is: >> > cat: ovs-ofctl.pid: No such file or directory >> >> Weird. I wonder if there is some issue with the way the ovs-ofctl pidfile is >> being referenced. That error is likely from (ofproto.at): >> >> on_exit 'kill `cat ovs-ofctl.pid`' >> >> Just a guess - could it be the way the autotools are expanding the on_exit >> line >> and it is trying to evaluate the `cat ovs-ofctl.pid` before ofctl has >> started? Can >> you try moving those lines in ofproto.at just to check if that could be it? > Hi Aaron, > Sorry for the delay in replying, I was OOO yesterday. > > I think ovs-ofctl is stopped before these checks are performed which is > causing the failure. > > The section above these checks is: > OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) > OVS_VSWITCHD_STOP > AT_CLEANUP > ]) > > When I moved the CHECK_FLOW_MONITORING commands to before this block, the > tests passed. Hrrm... That seems strange. Those are part of the define. I'm surprised that worked and didn't end in forever recursion. This might need some deeper looking. I wonder if moving those simply stopped running the checks. Maybe there are some permissions or resource issues on your build system that are preventing the needed files from being generated? > Thanks, > Michael. >> >> > Do you have any idea what might be the problem? >> > >> > Thanks, >> > Michael. >> >> -Original Message- >> >> From: Phelan, Michael >> >> Sent: Friday, August 30, 2024 4:12 PM >> >> To: Aaron Conole ; d...@openvswitch.org >> >> Cc: Chaudron, Eelco >> >> Subject: RE: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix >> >> potential NULL pointer access in log_flow_message(). >> >> >> >> Hi Aaron, >> >> Yes I've noticed that jobs have been failing quite a lot since make >> >> check was added to the tests. >> >> >> >> I will try to spend some time on this to debug and get back to you. >> >> >> >> Kind regards, >> >> Michael. >> >> >> >> > -Original Message- >> >> > From: Aaron Conole >> >> > Sent: Thursday, August 29, 2024 8:39 PM >> >> > To: d...@openvswitch.org >> >> > Cc: Chaudron, Eelco ; Phelan, Michael >> >> > >> >> > Subject: Re: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix >> >> > potential NULL pointer access in log_flow_message(). >> >> > >> >> > >> >> > Hi Michael, >> >> > >> >> > I'm seeing the most recent jobs failing with this error, and it's >> >> > been there for a while. Is there someone from Intel side who can >> >> > help debug the Intel CI? >> >> > >> >> > -Aaron >> >> > >> >> > ovs_jenk...@intel.com writes: >> >> > >> >> > > Test-Label: intel-ovs-compilation >> >> > > Test-Status: fail >> >> > > http://patchwork.ozlabs.org/api/patches/1977882/ >> >> > > >> >> > > AVX-512_compilation: failed >> >> > > DPLCS Test: fail >> >> > > DPIF Test: fail >> >> > > MFEX Test: fail >> >> > > Actions Test: fail >> >> > > Errors in DPCLS test: >> >> > > make -j CFLAGS=-g -O0 -march=native make all-am >> >>>> ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Use previous sarif-tools release due to issue in latest release.
Eelco Chaudron writes: > The just released v3.0 of the sarif tools do not work as > expected when comparing results. Temporarily force pip > to install the 2.0 release until this is fixes. > > Signed-off-by: Eelco Chaudron > --- Ugh.... LGTM Acked-by: Aaron Conole > .ci/linux-prepare.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh > index 2a191b57f..5f8a1db6a 100755 > --- a/.ci/linux-prepare.sh > +++ b/.ci/linux-prepare.sh > @@ -23,7 +23,7 @@ cd .. > # https://github.com/pypa/pip/issues/10655 > pip3 install --disable-pip-version-check --user wheel > pip3 install --disable-pip-version-check --user \ > -flake8 netaddr pyparsing sarif-tools sphinx setuptools > +flake8 netaddr pyparsing sarif-tools==2.0.0 sphinx setuptools > > # Install python test dependencies > pip3 install -r python/test_requirements.txt ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] conntrack: Disambiguate the cleaned count log.
After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with
rculists.")
the conntrack cleanup log reports the number of connections it checked
rather than the number of connections it cleaned. This patch includes the
count of connections cleaned during expiration sweeping.
Reported-by: Cheng Li
Suggested-by: Cheng Li
Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with
rculists.")
Acked-by: Simon Horman
Signed-off-by: Aaron Conole
---
v2: Clarify the log message a bit further per Eelco's request.
lib/conntrack.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index db44f82374..e96779e683 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1443,20 +1443,27 @@ conntrack_get_sweep_interval(struct conntrack *ct)
}
static size_t
-ct_sweep(struct conntrack *ct, struct rculist *list, long long now)
+ct_sweep(struct conntrack *ct, struct rculist *list, long long now,
+ size_t *cleaned_count)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct conn *conn;
+size_t cleaned = 0;
size_t count = 0;
RCULIST_FOR_EACH (conn, node, list) {
if (conn_expired(conn, now)) {
conn_clean(ct, conn);
+cleaned++;
}
count++;
}
+if (cleaned_count) {
+*cleaned_count = cleaned;
+}
+
return count;
}
@@ -1469,22 +1476,27 @@ conntrack_clean(struct conntrack *ct, long long now)
long long next_wakeup = now + conntrack_get_sweep_interval(ct);
unsigned int n_conn_limit, i;
size_t clean_end, count = 0;
+size_t total_cleaned = 0;
atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
clean_end = n_conn_limit / 64;
for (i = ct->next_sweep; i < N_EXP_LISTS; i++) {
+size_t cleaned;
+
if (count > clean_end) {
next_wakeup = 0;
break;
}
-count += ct_sweep(ct, &ct->exp_lists[i], now);
+count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned);
+total_cleaned += cleaned;
}
ct->next_sweep = (i < N_EXP_LISTS) ? i : 0;
-VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count,
+VLOG_DBG("conntrack cleaned %"PRIuSIZE" entries out of %"PRIuSIZE
+ " entries in %lld msec", total_cleaned, count,
time_msec() - now);
return next_wakeup;
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH branch-3.3] ofproto-dpif-upcall: Avoid stale ukeys leaks.
From: Eelco Chaudron
It is observed in some environments that there are much more ukeys than
actual DP flows. For example:
$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true
23: (keys 3612)
24: (keys 3625)
25: (keys 3485)
The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.
This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.
Reported-by: Roi Dayan
Co-authored-by: Han Zhou
Co-authored-by: Roi Dayan
Signed-off-by: Han Zhou
Signed-off-by: Roi Dayan
Signed-off-by: Eelco Chaudron
Signed-off-by: Aaron Conole
NOTE: Backported a portion of
600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow.")
to provide `strip_dp_hash` macro.
---
ofproto/ofproto-dpif-upcall.c | 16 +
tests/ofproto-dpif.at | 45 +++
tests/ofproto-macros.at | 5
3 files changed, 66 insertions(+)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index a046f8a339..f122b47f1c 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -57,6 +57,7 @@ COVERAGE_DEFINE(dumped_inconsistent_flow);
COVERAGE_DEFINE(dumped_new_flow);
COVERAGE_DEFINE(handler_duplicate_upcall);
COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
COVERAGE_DEFINE(ukey_dp_change);
COVERAGE_DEFINE(ukey_invalid_stat_reset);
COVERAGE_DEFINE(ukey_replace_contention);
@@ -302,6 +303,7 @@ struct udpif_key {
uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
/* 'state' debug information. */
unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -2995,6 +2997,20 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs);
}
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
if (result != UKEY_KEEP) {
/* Clears 'recircs' if filled by revalidate_ukey(). */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 0b23fd6c5e..d5aa9145c7 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -12136,3 +12136,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap |
wc -l`])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+ strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapath flows, and make sure they are gone.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names ], [0], [])
+
+dnl Move forward in time and make sure we have at least 4 * 500ms.
+AT_CHECK([ovs-appctl time/warp 3000 300], [0], [ignore])
+
+dnl Make sure no more ukeys exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk &#x
[ovs-dev] [PATCH branch-3.2] ofproto-dpif-upcall: Avoid stale ukeys leaks.
From: Eelco Chaudron
It is observed in some environments that there are much more ukeys than
actual DP flows. For example:
$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true
23: (keys 3612)
24: (keys 3625)
25: (keys 3485)
The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.
This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.
Reported-by: Roi Dayan
Co-authored-by: Han Zhou
Co-authored-by: Roi Dayan
Signed-off-by: Han Zhou
Signed-off-by: Roi Dayan
Signed-off-by: Eelco Chaudron
Signed-off-by: Aaron Conole
NOTE: Backported a portion of
600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow.")
to provide `strip_dp_hash` macro.
---
ofproto/ofproto-dpif-upcall.c | 16 +
tests/ofproto-dpif.at | 45 +++
tests/ofproto-macros.at | 5
3 files changed, 66 insertions(+)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 7d324e7e0a..f78f3331c8 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -56,6 +56,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow);
COVERAGE_DEFINE(dumped_new_flow);
COVERAGE_DEFINE(handler_duplicate_upcall);
COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
COVERAGE_DEFINE(ukey_dp_change);
COVERAGE_DEFINE(ukey_invalid_stat_reset);
COVERAGE_DEFINE(ukey_replace_contention);
@@ -297,6 +298,7 @@ struct udpif_key {
uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
/* 'state' debug information. */
unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -2967,6 +2969,20 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs);
}
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
if (result != UKEY_KEEP) {
/* Clears 'recircs' if filled by revalidate_ukey(). */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9edb743792..9ce515656b 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -12136,3 +12136,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap |
wc -l`])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+ strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapath flows, and make sure they are gone.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names ], [0], [])
+
+dnl Move forward in time and make sure we have at least 4 * 500ms.
+AT_CHECK([ovs-appctl time/warp 3000 300], [0], [ignore])
+
+dnl Make sure no more ukeys exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk &#x
[ovs-dev] [PATCH branch-3.4] ofproto-dpif-upcall: Avoid stale ukeys leaks.
From: Eelco Chaudron
It is observed in some environments that there are much more ukeys than
actual DP flows. For example:
$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true
23: (keys 3612)
24: (keys 3625)
25: (keys 3485)
The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.
This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.
Reported-by: Roi Dayan
Co-authored-by: Han Zhou
Co-authored-by: Roi Dayan
Signed-off-by: Han Zhou
Signed-off-by: Roi Dayan
Signed-off-by: Eelco Chaudron
Signed-off-by: Aaron Conole
(cherry picked from commit 180ab2fd635e03ffab5381bb1c22dda3f13aea7c)
---
ofproto/ofproto-dpif-upcall.c| 18
tests/ofproto-dpif.at| 45
utilities/usdt-scripts/flow_reval_monitor.py | 4 +-
3 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4d39bc5a71..e7d4c2b2c3 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -57,6 +57,7 @@ COVERAGE_DEFINE(dumped_inconsistent_flow);
COVERAGE_DEFINE(dumped_new_flow);
COVERAGE_DEFINE(handler_duplicate_upcall);
COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
COVERAGE_DEFINE(ukey_dp_change);
COVERAGE_DEFINE(ukey_invalid_stat_reset);
COVERAGE_DEFINE(ukey_replace_contention);
@@ -284,6 +285,7 @@ enum flow_del_reason {
FDR_TOO_EXPENSIVE, /* Too expensive to revalidate. */
FDR_UPDATE_FAIL,/* Datapath update failed. */
FDR_XLATION_ERROR, /* Flow translation error. */
+FDR_FLOW_MISSING_DP,/* Flow is missing from the datapath. */
};
/* 'udpif_key's are responsible for tracking the little bit of state udpif
@@ -318,6 +320,7 @@ struct udpif_key {
uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
/* 'state' debug information. */
unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -3040,6 +3043,21 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs, &del_reason);
}
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+del_reason = FDR_FLOW_MISSING_DP;
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
if (result != UKEY_KEEP) {
/* Clears 'recircs' if filled by revalidate_ukey(). */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 42fb66de68..1df944ef85 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -12661,3 +12661,48 @@ AT_CHECK([ovs-appctl revalidator/resume])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+ strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapa
[ovs-dev] [PATCH branch-3.1] ofproto-dpif-upcall: Avoid stale ukeys leaks.
From: Eelco Chaudron
It is observed in some environments that there are much more ukeys than
actual DP flows. For example:
$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true
23: (keys 3612)
24: (keys 3625)
25: (keys 3485)
The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.
This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.
Reported-by: Roi Dayan
Co-authored-by: Han Zhou
Co-authored-by: Roi Dayan
Signed-off-by: Han Zhou
Signed-off-by: Roi Dayan
Signed-off-by: Eelco Chaudron
Signed-off-by: Aaron Conole
NOTE: Backported a portion of
600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow.")
to provide `strip_dp_hash` macro.
---
ofproto/ofproto-dpif-upcall.c | 16 +
tests/ofproto-dpif.at | 45 +++
tests/ofproto-macros.at | 5
3 files changed, 66 insertions(+)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 9c6a63d7fb..dccbc8208a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -55,6 +55,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow);
COVERAGE_DEFINE(dumped_new_flow);
COVERAGE_DEFINE(handler_duplicate_upcall);
COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
COVERAGE_DEFINE(ukey_dp_change);
COVERAGE_DEFINE(ukey_invalid_stat_reset);
COVERAGE_DEFINE(ukey_replace_contention);
@@ -296,6 +297,7 @@ struct udpif_key {
uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
/* 'state' debug information. */
unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -2954,6 +2956,20 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs);
}
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
if (result != UKEY_KEEP) {
/* Clears 'recircs' if filled by revalidate_ukey(). */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index beb33713c7..513816ef94 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -12114,3 +12114,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap |
wc -l`])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+ strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapath flows, and make sure they are gone.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names ], [0], [])
+
+dnl Move forward in time and make sure we have at least 4 * 500ms.
+AT_CHECK([ovs-appctl time/warp 3000 300], [0], [ignore])
+
+dnl Make sure no more ukeys exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk &#x
[ovs-dev] [PATCH branch-3.0] ofproto-dpif-upcall: Avoid stale ukeys leaks.
From: Eelco Chaudron
It is observed in some environments that there are much more ukeys than
actual DP flows. For example:
$ ovs-appctl upcall/show
system@ovs-system:
flows : (current 7) (avg 6) (max 117) (limit 2125)
offloaded flows : 525
dump duration : 1063ms
ufid enabled : true
23: (keys 3612)
24: (keys 3625)
25: (keys 3485)
The revalidator threads are busy revalidating the stale ukeys leading to
high CPU and long dump duration.
This patch tracks the number of consecutive missed dumps. If four dumps
are missed in a row, it is assumed that the datapath flow no longer
exists, and the ukey can be deleted.
Reported-by: Roi Dayan
Co-authored-by: Han Zhou
Co-authored-by: Roi Dayan
Signed-off-by: Han Zhou
Signed-off-by: Roi Dayan
Signed-off-by: Eelco Chaudron
Signed-off-by: Aaron Conole
NOTE: Backported a portion of
600125b2c380 ("ofproto: Add ofproto/detrace command to map UFIDs to OpenFlow.")
to provide `strip_dp_hash` macro.
---
ofproto/ofproto-dpif-upcall.c | 16 +
tests/ofproto-dpif.at | 45 +++
tests/ofproto-macros.at | 5
3 files changed, 66 insertions(+)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 047f684e1b..5c06beb16a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -55,6 +55,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow);
COVERAGE_DEFINE(dumped_new_flow);
COVERAGE_DEFINE(handler_duplicate_upcall);
COVERAGE_DEFINE(revalidate_missed_dp_flow);
+COVERAGE_DEFINE(revalidate_missing_dp_flow);
COVERAGE_DEFINE(ukey_dp_change);
COVERAGE_DEFINE(ukey_invalid_stat_reset);
COVERAGE_DEFINE(ukey_replace_contention);
@@ -296,6 +297,7 @@ struct udpif_key {
uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
+uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
/* 'state' debug information. */
unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
@@ -2924,6 +2926,20 @@ revalidator_sweep__(struct revalidator *revalidator,
bool purge)
result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
reval_seq, &recircs);
}
+
+if (ukey->dump_seq != dump_seq) {
+ukey->missed_dumps++;
+if (ukey->missed_dumps >= 4) {
+/* If the flow was not dumped for 4 revalidator rounds,
+ * we can assume the datapath flow no longer exists
+ * and the ukey should be deleted. */
+COVERAGE_INC(revalidate_missing_dp_flow);
+result = UKEY_DELETE;
+}
+} else {
+ukey->missed_dumps = 0;
+}
+
if (result != UKEY_KEEP) {
/* Clears 'recircs' if filled by revalidate_ukey(). */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ead49f6567..3db1db0ac8 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -12102,3 +12102,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap |
wc -l`])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
+
+OVS_VSWITCHD_START
+add_of_ports br0 1 2
+
+m4_define([ICMP_PKT], [m4_join([,],
+[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
+[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
+[icmp(type=8,code=0)])])
+
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats | dnl
+ strip_duration | strip_dp_hash | sort], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(frag=no),
packets:0, bytes:0, used:never, actions:br0,p2
+])
+
+dnl Make sure the ukey exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk '{print $3}' | \
+grep -q '1)'], [0])
+
+dnl Delete all datapath flows, and make sure they are gone.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names ], [0], [])
+
+dnl Move forward in time and make sure we have at least 4 * 500ms.
+AT_CHECK([ovs-appctl time/warp 3000 300], [0], [ignore])
+
+dnl Make sure no more ukeys exists.
+AT_CHECK([ovs-appctl upcall/show | grep '(keys' | awk &#x
Re: [ovs-dev] [PATCH v5] ofproto-dpif-upcall: Avoid stale ukeys leaks.
Roi Dayan writes: > On 8/30/24 16:00, Aaron Conole wrote: >> Eelco Chaudron writes: >> >>> It is observed in some environments that there are much more ukeys than >>> actual DP flows. For example: >>> >>> $ ovs-appctl upcall/show >>> system@ovs-system: >>> flows : (current 7) (avg 6) (max 117) (limit 2125) >>> offloaded flows : 525 >>> dump duration : 1063ms >>> ufid enabled : true >>> >>> 23: (keys 3612) >>> 24: (keys 3625) >>> 25: (keys 3485) >>> >>> The revalidator threads are busy revalidating the stale ukeys leading to >>> high CPU and long dump duration. >>> >>> This patch tracks the number of consecutive missed dumps. If four dumps >>> are missed in a row, it is assumed that the datapath flow no longer >>> exists, and the ukey can be deleted. >>> >>> Reported-by: Roi Dayan >>> Co-authored-by: Han Zhou >>> Co-authored-by: Roi Dayan >>> Signed-off-by: Han Zhou >>> Signed-off-by: Roi Dayan >>> Signed-off-by: Eelco Chaudron >>> --- >> Thanks all, applied. >> > > Hi, > > Can we get this merged into 3.x branches maybe ? Yes, I think it makes sense there. While there isn't a specific 'fixes' for this, it does seem more like a fix rather than a feature. > From 3.0 to 3.3 the python script and flow_del_reason enum > doesn't exists. With removing the diff and removing > "del_reason = FDR_FLOW_MISSING_DP;" should apply cleanly. It will fail because it depends on some other support (for example strip_dp_hash in the tests). I'm working on a backport for it, and will post. > This patch applies cleanly without modification on branch 3.4. > > Tell me if needed to send a back-ported patch that can be applied > to branches 3.0-3.3. I will be posting it for each branch when it is ready - I think I should be happy with it all by COB tomorrow. > Thanks, > Roi > >> ___ >> 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] conntrack: Disambiguate the cleaned count log.
After 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with
rculists.")
the conntrack cleanup log reports the number of connections it checked
rather than the number of connections it cleaned. This patch includes the
count of connections cleaned during expiration sweeping.
Reported-by: Cheng Li
Suggested-by: Cheng Li
Fixes: 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with
rculists.")
Signed-off-by: Aaron Conole
---
lib/conntrack.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index db44f82374..e90c2ac19f 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1443,20 +1443,27 @@ conntrack_get_sweep_interval(struct conntrack *ct)
}
static size_t
-ct_sweep(struct conntrack *ct, struct rculist *list, long long now)
+ct_sweep(struct conntrack *ct, struct rculist *list, long long now,
+ size_t *cleaned_count)
OVS_NO_THREAD_SAFETY_ANALYSIS
{
struct conn *conn;
+size_t cleaned = 0;
size_t count = 0;
RCULIST_FOR_EACH (conn, node, list) {
if (conn_expired(conn, now)) {
conn_clean(ct, conn);
+cleaned++;
}
count++;
}
+if (cleaned_count) {
+*cleaned_count = cleaned;
+}
+
return count;
}
@@ -1469,22 +1476,27 @@ conntrack_clean(struct conntrack *ct, long long now)
long long next_wakeup = now + conntrack_get_sweep_interval(ct);
unsigned int n_conn_limit, i;
size_t clean_end, count = 0;
+size_t total_cleaned = 0;
atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
clean_end = n_conn_limit / 64;
for (i = ct->next_sweep; i < N_EXP_LISTS; i++) {
+size_t cleaned;
+
if (count > clean_end) {
next_wakeup = 0;
break;
}
-count += ct_sweep(ct, &ct->exp_lists[i], now);
+count += ct_sweep(ct, &ct->exp_lists[i], now, &cleaned);
+total_cleaned += cleaned;
}
ct->next_sweep = (i < N_EXP_LISTS) ? i : 0;
-VLOG_DBG("conntrack cleanup %"PRIuSIZE" entries in %lld msec", count,
+VLOG_DBG("conntrack cleaned %"PRIuSIZE" / %"PRIuSIZE
+ " entries in %lld msec", total_cleaned, count,
time_msec() - now);
return next_wakeup;
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] system-traffic: Do not rely on conncount for already tracked packets.
Paolo Valerio writes: > Paolo Valerio writes: > >> Simon Horman writes: >> >>> On Wed, Aug 28, 2024 at 07:14:06PM +0200, Paolo Valerio wrote: As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT result in the unexpected failure of the following tests: conntrack - multiple zones, local conntrack - multi-stage pipeline, local conntrack - can match and clear ct_state from outside OVS this happens because the nf_conncount turns on connection tracking and the above tests rely on this side effect. However, this behavior may be corrected in the kernel, which could, in turn, cause the tests to fail. The patch removes the assumption by adding explicit iptables rules to attach an nf_conn template to the skb resulting tracked once hit the OvS pipeline. While at it, introduce $HAVE_IPTABLES and skip tests if iptables binary is not present. Reported-by: Xin Long Reported-at: https://issues.redhat.com/browse/FDP-708 Signed-off-by: Paolo Valerio >>> >>> Hi Paolo, >>> >>> I exercised this using vng with net-next compiled using >>> tools/testing/selftests/net/config from the upstream kernel tree [1]. >>> >>> [1] >>> https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style >>> >>> The resulting config does not have CONFIG_NETFILTER_CONNCOUNT set. >>> >> >> Hi Simon, >> >> I used vng with net-next as well, but with a different config. In my >> case I simply reused a previously used config which is essentially my >> local one updated with olddefconfig and manually (to remove/add things). >> >>> Some observations: >>> >>> * CONFIG_NETFILTER_XT_TARGET_CT is required for -j CT >>> >>> I don't think this is a problem (other than my own problem >>> of it taking me a long time to figure that out). But it seems >>> worth noting (see parentheses in previous sentence:). >>> >> >> It's something I was thinking about right after the patch submission. >> Although extensions are fairly common in the main distros, maybe we may >> consider checking they are present. >> >> The only ways that comes to mind to reliably check whether the >> extensions (both kernel and user space) are present is simply by >> applying a rule. >> >> I guess we can (added a diff at the bottom), given the nft discussion >> and the potential follow-up, change things a bit in order to better >> accomodate a potential migration/follow-up. >> >> WDYT? >> >>> * Of the tests that are updated by this patch, >>> I only observed that the last one, >>> "conntrack - can match and clear ct_state from outside OVS", >>> fails without this patch applied. >>> >>> I am unsure if that is something that warrants updating this >>> patch or not. Or if, rather, there is an error in my testing. >> >> Thanks for testing it. >> >> Interesting. >> I tried the following config against net-next and it seems I can't >> reproduce that behaviour: >> >> vng --build --config tools/testing/selftests/net/config \ >> --configitem CONFIG_NF_CONNTRACK_ZONES=y \ >> --configitem CONFIG_NETFILTER_XT_TARGET_CT=m -v >> >> --- >> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >> index 5203b1df8..6b5eb32fc 100644 >> --- a/tests/system-kmod-macros.at >> +++ b/tests/system-kmod-macros.at >> @@ -267,3 +267,22 @@ m4_define([OVS_CHECK_BAREUDP], >> AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635 >> ethertype mpls_uc 2>&1 >/dev/null]) >> AT_CHECK([ip link del dev ovs_bareudp0]) >> ]) >> + >> +# CHECK_EXTERNAL_CT() >> +# >> +# Checks if packets can be tracked outside OvS. >> +m4_define([CHECK_EXTERNAL_CT], >> +[ >> +dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT) >> +dnl and user space extensions need to be present. >> +AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT]) >> +AT_CHECK([iptables -t raw -D OUTPUT 1]) >> +]) >> + >> +# ADD_EXTERNAL_CT() >> +# >> +# Let conntrack start tracking the packets outside OvS. >> +m4_define([ADD_EXTERNAL_CT], >> +[ >> +AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) >> +]) > > on_exit here got lost for some reason. > Below the corrected diff. > > --- > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > index 5203b1df8..ab89ea24b 100644 > --- a/tests/system-kmod-macros.at > +++ b/tests/system-kmod-macros.at > @@ -267,3 +267,23 @@ m4_define([OVS_CHECK_BAREUDP], > AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635 > ethertype mpls_uc 2>&1 >/dev/null]) > AT_CHECK([ip link del dev ovs_bareudp0]) > ]) > + > +# CHECK_EXTERNAL_CT() > +# > +# Checks if packets can be tracked outside OvS. > +m4_define([CHECK_EXTERNAL_CT], > +[ > +dnl Kernel config (CONFIG_NETFILTER_XT_TARGET_CT) > +dnl and user space extensions need to be present. > +AT_SKIP_IF([! iptables -t raw -I OUTPUT 1 -j CT]) > +AT_CHECK([iptables -t raw -D OUTPUT 1]) > +]) > + > +# ADD_EXTERNAL_CT() > +# > +# Le
Re: [ovs-dev] [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential NULL pointer access in log_flow_message().
"Phelan, Michael" writes: > Hi Aaron, > I have looked into the failures on the CI and ran some tests manually > and tests 1010, 1015, 1020, 1025, 1030 and 1035 are failing in make > check even on the main branch. > > They fail in this block of calls from ofproto.at: > CHECK_FLOW_MONITORING([1.0], [OpenFlow10], [], NXST_FLOW_MONITOR) > CHECK_FLOW_MONITORING([1.1], [OpenFlow11], [ (OF1.1)], NXST_FLOW_MONITOR) > CHECK_FLOW_MONITORING([1.2], [OpenFlow12], [ (OF1.2)], NXST_FLOW_MONITOR) > CHECK_FLOW_MONITORING([1.3], [OpenFlow13], [ (OF1.3)], ONFST_FLOW_MONITOR) > CHECK_FLOW_MONITORING([1.4], [OpenFlow14], [ (OF1.4)], OFPST_FLOW_MONITOR) > CHECK_FLOW_MONITORING([1.5], [OpenFlow15], [ (OF1.5)], OFPST_FLOW_MONITOR) Thanks for narrowing it down. > The error in the log files is: > cat: ovs-ofctl.pid: No such file or directory Weird. I wonder if there is some issue with the way the ovs-ofctl pidfile is being referenced. That error is likely from (ofproto.at): on_exit 'kill `cat ovs-ofctl.pid`' Just a guess - could it be the way the autotools are expanding the on_exit line and it is trying to evaluate the `cat ovs-ofctl.pid` before ofctl has started? Can you try moving those lines in ofproto.at just to check if that could be it? > Do you have any idea what might be the problem? > > Thanks, > Michael. >> -Original Message- >> From: Phelan, Michael >> Sent: Friday, August 30, 2024 4:12 PM >> To: Aaron Conole ; d...@openvswitch.org >> Cc: Chaudron, Eelco >> Subject: RE: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix potential >> NULL pointer access in log_flow_message(). >> >> Hi Aaron, >> Yes I've noticed that jobs have been failing quite a lot since make check was >> added to the tests. >> >> I will try to spend some time on this to debug and get back to you. >> >> Kind regards, >> Michael. >> >> > -Original Message- >> > From: Aaron Conole >> > Sent: Thursday, August 29, 2024 8:39 PM >> > To: d...@openvswitch.org >> > Cc: Chaudron, Eelco ; Phelan, Michael >> > >> > Subject: Re: [ovs-build] |fail| pw1977882 [ovs-dev, 3/7] dpif: Fix >> > potential >> > NULL pointer access in log_flow_message(). >> > >> > >> > Hi Michael, >> > >> > I'm seeing the most recent jobs failing with this error, and it's been >> > there for a while. Is there someone from Intel side who can help debug >> > the Intel CI? >> > >> > -Aaron >> > >> > ovs_jenk...@intel.com writes: >> > >> > > Test-Label: intel-ovs-compilation >> > > Test-Status: fail >> > > http://patchwork.ozlabs.org/api/patches/1977882/ >> > > >> > > AVX-512_compilation: failed >> > > DPLCS Test: fail >> > > DPIF Test: fail >> > > MFEX Test: fail >> > > Actions Test: fail >> > > Errors in DPCLS test: >> > > make -j CFLAGS=-g -O0 -march=native >> > > make all-am >>>> ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-dpctl-top: Fix RuntimeError with resizing flow dict during iteration.
Eelco Chaudron writes:
> On 30 Aug 2024, at 17:21, Aaron Conole wrote:
>
>> Python does not allow modifying a dictionary or list while iterating
>> over the elements. The common idom to work around this is to create
>> a copy of the keys or elements and then use that information to delete
>> the corresponding entries from the original dictionary.
>>
>> This patch changes the iteration to use an intermediate copy and then
>> modify the original dictionary. This avoids the associated
>> RuntimeError.
>>
>> Reported-by: Eelco Chaudron
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-August/417000.html
>> Fixes: 14b4c575c284 ("utilities: a top like tool for ovs-dpctl dump-flows.")
>> Signed-off-by: Aaron Conole
>
> Thanks Aaron for fixing this.
>
> Acked-by: Eelco Chaudron
Applied. Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovs-dpctl-top: Fix RuntimeError with resizing flow dict during iteration.
Python does not allow modifying a dictionary or list while iterating
over the elements. The common idom to work around this is to create
a copy of the keys or elements and then use that information to delete
the corresponding entries from the original dictionary.
This patch changes the iteration to use an intermediate copy and then
modify the original dictionary. This avoids the associated
RuntimeError.
Reported-by: Eelco Chaudron
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2024-August/417000.html
Fixes: 14b4c575c284 ("utilities: a top like tool for ovs-dpctl dump-flows.")
Signed-off-by: Aaron Conole
---
utilities/ovs-dpctl-top.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utilities/ovs-dpctl-top.in b/utilities/ovs-dpctl-top.in
index 1708c2f374..f2d5bae6a6 100755
--- a/utilities/ovs-dpctl-top.in
+++ b/utilities/ovs-dpctl-top.in
@@ -1007,7 +1007,7 @@ class FlowDB:
def decay(self, decayTimeInSeconds):
""" Decay content. """
now = datetime.datetime.now()
-for (key, value) in self._flows.items():
+for (key, value) in list(self._flows.items()):
(stats_dict, updateTime) = value
delta = now - updateTime
--
2.45.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-dpctl-top: Fix Python3.12 invalid syntax warning
Eelco Chaudron writes:
> On 30 Aug 2024, at 12:23, Eelco Chaudron wrote:
>
>> On 30 Aug 2024, at 11:15, Simon Horman wrote:
>>
>>> On Thu, Aug 29, 2024 at 12:04:33PM -0400, Michael Santana wrote:
Python3.12 is throwing syntax warnings in ovs-dpctl-top
[root@localhost ~]# python --version
Python 3.12.5
[root@localhost ~]# ovs-dpctl-top --help > /dev/null
/usr/bin/ovs-dpctl-top:392: SyntaxWarning: invalid escape sequence '\w'
FIELDS_CMPND = re.compile("([\w]+)\((.+)\)")
/usr/bin/ovs-dpctl-top:394: SyntaxWarning: invalid escape sequence '\w'
FIELDS_CMPND_ELEMENT = re.compile("([\w:]+)=([/\.\w:]+)")
/usr/bin/ovs-dpctl-top:395: SyntaxWarning: invalid escape sequence '\w'
FIELDS_ELEMENT = re.compile("([\w]+):([-\.\w]+)")
The warning seems to be new to python3.12
Python 3.12.5 (main, Aug 23 2024, 00:00:00)
>>> import re
>>> re.compile("([\w]+)\((.+)\)")
:1: SyntaxWarning: invalid escape sequence '\w'
re.compile('([\\w]+)\\((.+)\\)')
>>> re.compile(r"([\w]+)\((.+)\)")
re.compile('([\\w]+)\\((.+)\\)')
>>>
>>> Thanks Michael,
>>>
>>> I also see this.
>>>
Python 3.11.4 (main, Jun 7 2023, 00:00:00)
>>> import re
>>> re.compile("([\w]+)\((.+)\)")
re.compile('([\\w]+)\\((.+)\\)')
Prepending the string with r tells python treat the string as a raw string
literal and to not try to scape \w, \(, etc, and gets rid of the warning
Signed-off-by: Michael Santana
>>>
>>> The checkpatch warning flagged by the robot aside, which I think can be
>>> addressed when the patch is applied, this looks good to me.
>>>
>>> Acked-by: Simon Horman
>>
>> Tested the change, and it looks good to me.
>>
>> Acked-by: Eelco Chaudron
>
> I wanted to add that I have a general problem with the tool, I assume it’s
> related to my machine, but it might not be.
> I get these errors for flows added:
>
> in_port(2)in thread Thread-1:
> Traceback (most recent call last):
> File "/usr/lib64/python3.9/threading.py", line 980, in _bootstrap_inner
> self.run()
> File "/root/Documents/scratch/ovs/./utilities/ovs-dpctl-top", line 1126, in
> run
> self._flow_db.decay(self._interval)
> File "/root/Documents/scratch/ovs/./utilities/ovs-dpctl-top", line 1010, in
> decay
> for (key, value) in self._flows.items():
> RuntimeError: dictionary changed size during iteration
>
> Can you confirm the tool works for you with active traffic?
I think this is an issue in the tool. Can you try the following
snippet and see if it resolves your issue?
diff --git a/utilities/ovs-dpctl-top.in b/utilities/ovs-dpctl-top.in
index 1708c2f374..f2d5bae6a6 100755
--- a/utilities/ovs-dpctl-top.in
+++ b/utilities/ovs-dpctl-top.in
@@ -1007,7 +1007,7 @@ class FlowDB:
def decay(self, decayTimeInSeconds):
""" Decay content. """
now = datetime.datetime.now()
-for (key, value) in self._flows.items():
+for (key, value) in list(self._flows.items()):
(stats_dict, updateTime) = value
delta = now - updateTime
> //Eelco
>
> ___
> 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] ovs-dpctl-top: Fix Python3.12 invalid syntax warning
Simon Horman writes:
> On Thu, Aug 29, 2024 at 12:04:33PM -0400, Michael Santana wrote:
>> Python3.12 is throwing syntax warnings in ovs-dpctl-top
>>
>> [root@localhost ~]# python --version
>> Python 3.12.5
>> [root@localhost ~]# ovs-dpctl-top --help > /dev/null
>> /usr/bin/ovs-dpctl-top:392: SyntaxWarning: invalid escape sequence '\w'
>> FIELDS_CMPND = re.compile("([\w]+)\((.+)\)")
>> /usr/bin/ovs-dpctl-top:394: SyntaxWarning: invalid escape sequence '\w'
>> FIELDS_CMPND_ELEMENT = re.compile("([\w:]+)=([/\.\w:]+)")
>> /usr/bin/ovs-dpctl-top:395: SyntaxWarning: invalid escape sequence '\w'
>> FIELDS_ELEMENT = re.compile("([\w]+):([-\.\w]+)")
>>
>>
>> The warning seems to be new to python3.12
>>
>> Python 3.12.5 (main, Aug 23 2024, 00:00:00)
>> >>> import re
>> >>> re.compile("([\w]+)\((.+)\)")
>> :1: SyntaxWarning: invalid escape sequence '\w'
>> re.compile('([\\w]+)\\((.+)\\)')
>> >>> re.compile(r"([\w]+)\((.+)\)")
>> re.compile('([\\w]+)\\((.+)\\)')
>
> Thanks Michael,
>
> I also see this.
>
>>
>> Python 3.11.4 (main, Jun 7 2023, 00:00:00)
>> >>> import re
>> >>> re.compile("([\w]+)\((.+)\)")
>> re.compile('([\\w]+)\\((.+)\\)')
>>
>>
>> Prepending the string with r tells python treat the string as a raw string
>> literal and to not try to scape \w, \(, etc, and gets rid of the warning
>>
>> Signed-off-by: Michael Santana
>
> The checkpatch warning flagged by the robot aside, which I think can be
> addressed when the patch is applied, this looks good to me.
I fixed that and the spelling mistake in the commit message (scape => escape)
> Acked-by: Simon Horman
> ___
> 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] ovs-dpctl-top: Fix Python3.12 invalid syntax warning
Michael Santana writes:
> Python3.12 is throwing syntax warnings in ovs-dpctl-top
>
> [root@localhost ~]# python --version
> Python 3.12.5
> [root@localhost ~]# ovs-dpctl-top --help > /dev/null
> /usr/bin/ovs-dpctl-top:392: SyntaxWarning: invalid escape sequence '\w'
> FIELDS_CMPND = re.compile("([\w]+)\((.+)\)")
> /usr/bin/ovs-dpctl-top:394: SyntaxWarning: invalid escape sequence '\w'
> FIELDS_CMPND_ELEMENT = re.compile("([\w:]+)=([/\.\w:]+)")
> /usr/bin/ovs-dpctl-top:395: SyntaxWarning: invalid escape sequence '\w'
> FIELDS_ELEMENT = re.compile("([\w]+):([-\.\w]+)")
>
>
> The warning seems to be new to python3.12
>
> Python 3.12.5 (main, Aug 23 2024, 00:00:00)
import re
re.compile("([\w]+)\((.+)\)")
> :1: SyntaxWarning: invalid escape sequence '\w'
> re.compile('([\\w]+)\\((.+)\\)')
re.compile(r"([\w]+)\((.+)\)")
> re.compile('([\\w]+)\\((.+)\\)')
>
> Python 3.11.4 (main, Jun 7 2023, 00:00:00)
import re
re.compile("([\w]+)\((.+)\)")
> re.compile('([\\w]+)\\((.+)\\)')
>
>
> Prepending the string with r tells python treat the string as a raw string
> literal and to not try to scape \w, \(, etc, and gets rid of the warning
>
> Signed-off-by: Michael Santana
> ---
Thanks Michael, Simon, and Eelco. Applied.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/2] Update and rename dpif_nl_exec_monitor.py.
Eelco Chaudron writes: > Updated the dpif_nl_exec_monitor.py utility to also optionally monitor > the other DPIF netlink operations, i.e., DPIF_OP_FLOW_PUT, > DPIF_OP_FLOW_DEL and DPIF_OP_FLOW_GET. > > Due to the nature of the change, also renamed the script to > dpif_op_nl_monitor.py. > > v2: - Renamed the script. > - Fixed some general comments from Ilya. > - Updated the documentation and build files due to the > script name change. > v3: - Fixed header comments/help text. > - Fixed spelling mistake > - Also made the execute operational optional. > > Eelco Chaudron (2): > utilities: Update dpif_nl_exec_monitor.py to include new actions. > utilities: Updated dpif_nl_exec_monitor.py to debug all operations. > Thanks for this series, Eelco! Applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5] ofproto-dpif-upcall: Avoid stale ukeys leaks.
Eelco Chaudron writes: > It is observed in some environments that there are much more ukeys than > actual DP flows. For example: > > $ ovs-appctl upcall/show > system@ovs-system: > flows : (current 7) (avg 6) (max 117) (limit 2125) > offloaded flows : 525 > dump duration : 1063ms > ufid enabled : true > > 23: (keys 3612) > 24: (keys 3625) > 25: (keys 3485) > > The revalidator threads are busy revalidating the stale ukeys leading to > high CPU and long dump duration. > > This patch tracks the number of consecutive missed dumps. If four dumps > are missed in a row, it is assumed that the datapath flow no longer > exists, and the ukey can be deleted. > > Reported-by: Roi Dayan > Co-authored-by: Han Zhou > Co-authored-by: Roi Dayan > Signed-off-by: Han Zhou > Signed-off-by: Roi Dayan > Signed-off-by: Eelco Chaudron > --- Thanks all, applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Correctly set ip offload flag in native tunneling.
Simon Horman writes:
> On Wed, Aug 28, 2024 at 10:32:01AM -0400, Aaron Conole wrote:
>> Eelco Chaudron writes:
>>
>> > On 27 Aug 2024, at 18:01, Mike Pattrick wrote:
>> >
>> >> Coverity identified the following issue
>> >>
>> >> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN)
>> >> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking
>> >> return value (as is done elsewhere 9 out of 11 times).
>> >>
>> >> This appears to be a true positive, the fields getter was called instead
>> >> of its setter.
>> >>
>> >> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>> >> Reported-by: Eelco Chaudron
>> >> Signed-off-by: Mike Pattrick
>> >
>> > Thanks for the patch! The fix looks good to me. Is there any reason
>> > why none of the CI tests picked this up? I guess we need real hardware
>> > to verify this.
>>
>> I guess we have identified a coverage gap. :)
>
> I'm inclined to think that can be addressed as a follow-up.
Yes - if it truly requires some real hardware to address we will need to
add such a test, but that wouldn't hold up applying a fix.
>> > Cheers,
>> >
>> > Eelco
>> >
>> > Acked-by: Eelco Chaudron
>
> Acked-by: Simon Horman
>
> Eelco, will you will handle applying this (or not)?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 net-next] net: openvswitch: Use ERR_CAST() to return
Yan Zhen writes: > Using ERR_CAST() is more reasonable and safer, When it is necessary > to convert the type of an error pointer and return it. > > Signed-off-by: Yan Zhen > --- LGTM, and seems like the only remaining place where we are open coding the error return conversion in the OVS module (at least, where we do a check with IS_ERR). At least, I tried to visit them all while looking at this. Reviewed-by: Aaron Conole > net/openvswitch/flow_netlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index c92bdc4dfe19..729ef582a3a8 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -2491,7 +2491,7 @@ static struct nlattr *reserve_sfa_size(struct > sw_flow_actions **sfa, > > acts = nla_alloc_flow_actions(new_acts_size); > if (IS_ERR(acts)) > - return (void *)acts; > + return ERR_CAST(acts); > > memcpy(acts->actions, (*sfa)->actions, (*sfa)->actions_len); > acts->actions_len = (*sfa)->actions_len; > -- > 2.34.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] system-traffic: Do not rely on conncount for already tracked packets.
Paolo Valerio writes: > As Long reported, kernels built without CONFIG_NETFILTER_CONNCOUNT > result in the unexpected failure of the following tests: > > conntrack - multiple zones, local > conntrack - multi-stage pipeline, local > conntrack - can match and clear ct_state from outside OVS > > this happens because the nf_conncount turns on connection tracking and > the above tests rely on this side effect. However, this behavior may > be corrected in the kernel, which could, in turn, cause the tests to > fail. > > The patch removes the assumption by adding explicit iptables rules to > attach an nf_conn template to the skb resulting tracked once hit the > OvS pipeline. > > While at it, introduce $HAVE_IPTABLES and skip tests if iptables > binary is not present. > > Reported-by: Xin Long > Reported-at: https://issues.redhat.com/browse/FDP-708 > Signed-off-by: Paolo Valerio > --- Makes sense to me. Reviewed-by: Aaron Conole NOTE: not for this series, we do need to also think about systems where 'iptables' is not available but 'nft' is available. Right now there is the iptables compat layer - maybe we just rely on that existing for now. > V2: > - add $HAVE_IPTABLES > - reduced subject length (0-day Robot) > > Signed-off-by: Paolo Valerio > --- > tests/atlocal.in| 3 +++ > tests/ovs-macros.at | 5 + > tests/system-traffic.at | 8 > 3 files changed, 16 insertions(+) > > diff --git a/tests/atlocal.in b/tests/atlocal.in > index 8565a0bae..d6b87f8ec 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -185,6 +185,9 @@ find_command lftp > # Set HAVE_ETHTOOL > find_command ethtool > > +# Set HAVE_IPTABLES > +find_command iptables > + > CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout 1" > > # Determine whether "diff" supports "normal" diffs. (busybox diff does not.) > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > index 06c978555..df2835747 100644 > --- a/tests/ovs-macros.at > +++ b/tests/ovs-macros.at > @@ -366,3 +366,8 @@ dnl Add a rule to always accept the traffic. > m4_define([IPTABLES_ACCEPT], >[AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT]) > on_exit 'iptables -D INPUT 1 -i $1']) > + > +dnl Required to let conntrack start tracking the packets outside ovs > +m4_define([IPTABLES_CT], > + [AT_CHECK([iptables -t raw -I OUTPUT 1 -o $1 -j CT]) > + on_exit 'iptables -t raw -D OUTPUT 1']) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 202ff0492..46a9414d4 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -1094,6 +1094,7 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/Invalid Geneve tunnel > metadata on bridge br0 while > AT_CLEANUP > > AT_SETUP([datapath - ping over gre tunnel by simulated packets]) > +AT_SKIP_IF([test $HAVE_IPTABLES = no]) > OVS_CHECK_MIN_KERNEL(3, 10) > > OVS_TRAFFIC_VSWITCHD_START() > @@ -1140,6 +1141,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > AT_SETUP([datapath - ping over erspan v1 tunnel by simulated packets]) > +AT_SKIP_IF([test $HAVE_IPTABLES = no]) > OVS_CHECK_MIN_KERNEL(3, 10) > > OVS_TRAFFIC_VSWITCHD_START() > @@ -5456,10 +5458,12 @@ OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > AT_SETUP([conntrack - multiple zones, local]) > +AT_SKIP_IF([test $HAVE_IPTABLES = no]) > CHECK_CONNTRACK() > CHECK_CONNTRACK_LOCAL_STACK() > OVS_TRAFFIC_VSWITCHD_START() > > +IPTABLES_CT([br0]) > ADD_NAMESPACES(at_ns0) > > AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) > @@ -5505,10 +5509,12 @@ OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > AT_SETUP([conntrack - multi-stage pipeline, local]) > +AT_SKIP_IF([test $HAVE_IPTABLES = no]) > CHECK_CONNTRACK() > CHECK_CONNTRACK_LOCAL_STACK() > OVS_TRAFFIC_VSWITCHD_START() > > +IPTABLES_CT([br0]) > ADD_NAMESPACES(at_ns0) > > AT_CHECK([ip addr add dev br0 "10.1.1.1/24"]) > @@ -8386,6 +8392,7 @@ OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > AT_SETUP([conntrack - can match and clear ct_state from outside OVS]) > +AT_SKIP_IF([test $HAVE_IPTABLES = no]) > CHECK_CONNTRACK_LOCAL_STACK() > OVS_CHECK_GENEVE() > > @@ -8396,6 +8403,7 @@ AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > AT_CHECK([ovs-ofctl add-flow br-underlay > "priority=100,ct_state=+trk,actions=ct_clear,resubmit(,0)"]) > AT_CHECK([ovs-ofctl add-flow br-underlay "priority=10,actions=normal"]) > > +IPTABLES_CT([br0]) > ADD_NAMESPACES(at_ns0) > > dnl Set up underlay link from host into the namespace using veth pair. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/2] ovs-macros.at: Correctly delete iptables rule on_exit.
Paolo Valerio writes:
> Currently, at every call of IPTABLES_ACCEPT() an iptables rule gets
> added. Such rule is supposed to be removed on exit, but the current
> syntax for deleting the rule is incorrect, resulting in a leftover
> rule after execution.
>
> Fix it by correcting the deletion command.
>
> Fixes: 5e06e7ac99dc ("tests: Refactor the iptables accept rule.")
> Signed-off-by: Paolo Valerio
> ---
Reviewed-by: Aaron Conole
> tests/ovs-macros.at | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
> index df2835747..4cc8e7bc8 100644
> --- a/tests/ovs-macros.at
> +++ b/tests/ovs-macros.at
> @@ -365,7 +365,7 @@ dnl to reject input traffic from bridges such as
> br-underlay.
> dnl Add a rule to always accept the traffic.
> m4_define([IPTABLES_ACCEPT],
>[AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT])
> - on_exit 'iptables -D INPUT 1 -i $1'])
> + on_exit 'iptables -D INPUT 1'])
>
> dnl Required to let conntrack start tracking the packets outside ovs
> m4_define([IPTABLES_CT],
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/7] vlog: Only close() valid file descriptors.
Eelco Chaudron writes:
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
> lib/vlog.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vlog.c b/lib/vlog.c
> index 59b524b09..876209ee2 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -396,7 +396,9 @@ vlog_set_log_file__(char *new_log_file_name)
>&& old_stat.st_ino == new_stat.st_ino));
> ovs_mutex_unlock(&log_file_mutex);
> if (same_file) {
> -close(new_log_fd);
> +if (new_log_fd >= 0) {
> +close(new_log_fd);
> +}
> free(new_log_file_name);
> return 0;
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 6/7] dpif-netdev-perf: Eliminate dead code.
Eelco Chaudron writes:
> This patch eliminates a small piece of dead code.
>
> Fixes: 79f368756ce8 ("dpif-netdev: Detailed performance stats for PMDs")
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
(I got a little bit spooked with this change at first, but the division
seems properly guarded - as you noted).
> lib/dpif-netdev-perf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
> index 79ea5e3be..1cd4ee084 100644
> --- a/lib/dpif-netdev-perf.c
> +++ b/lib/dpif-netdev-perf.c
> @@ -267,7 +267,7 @@ pmd_perf_format_overall_stats(struct ds *str, struct
> pmd_perf_stats *s,
> " - Lost upcalls: %12"PRIu64" (%5.1f %%)\n",
> rx_packets, (rx_packets / duration) / 1000,
> 1.0 * stats[PMD_CYCLES_ITER_BUSY] / rx_packets,
> -passes, rx_packets ? 1.0 * passes / rx_packets : 0,
> +passes, 1.0 * passes / rx_packets,
> stats[PMD_STAT_PHWOL_HIT],
> 100.0 * stats[PMD_STAT_PHWOL_HIT] / passes,
> stats[PMD_STAT_MFEX_OPT_HIT],
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 7/7] odp-util: Fix dead code warning in format_odp_set_nsh().
Eelco Chaudron writes:
> The 'type' verification is incorrect, leading to a dead code warning.
> The value OVS_NSH_KEY_ATTR_MAX represents the maximum value, not the
> maximum plus one. This fix ensures that the OVS_NSH_KEY_ATTR_MD2 case
> is reachable by changing the condition from 'greater than or equal'
> to 'greater than'."
>
> Fixes: 81fdabb94dd7 ("nsh: fix nested mask for OVS_KEY_ATTR_NSH")
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
> lib/odp-util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index d3245223d..7c80a7e88 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -1056,7 +1056,7 @@ format_odp_set_nsh(struct ds *ds, const struct nlattr
> *attr)
> enum ovs_nsh_key_attr type = nl_attr_type(a);
> size_t len = nl_attr_get_size(a);
>
> -if (type >= OVS_NSH_KEY_ATTR_MAX) {
> +if (type > OVS_NSH_KEY_ATTR_MAX) {
> return;
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/7] dpif: Fix potential NULL pointer access in log_flow_message().
Eelco Chaudron writes:
> When actions is NULL and action_len is not zero, it would
> potentially allow format_odp_actions() to dereference the
> NULL pointer. The fix will check for valid actions pointer
> only, as format_odp_actions() will handle a zero length.
>
> Fixes: cdee00fd635d ("datapath: Replace "struct odp_action" by Netlink
> attributes.")
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
> lib/dpif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index ab633fd27..245c8b5ee 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1810,7 +1810,7 @@ log_flow_message(const struct dpif *dpif, int error,
> ds_put_cstr(&ds, ", ");
> dpif_flow_stats_format(stats, &ds);
> }
> -if (actions || actions_len) {
> +if (actions) {
> ds_put_cstr(&ds, ", actions:");
> format_odp_actions(&ds, actions, actions_len, NULL);
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/7] netdev-native-tnl: Fix Coverity integer overflows report.
Eelco Chaudron writes:
> Fixed potential integer overflow in netdev_srv6_pop_header(),
> by making sure the packet length does at least account for
> the IPv6 header.
>
> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
> lib/netdev-native-tnl.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 16c56608d..92081d5e3 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -990,7 +990,6 @@ struct dp_packet *
> netdev_srv6_pop_header(struct dp_packet *packet)
> {
> const struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet);
> -size_t size = dp_packet_l3_size(packet) - IPV6_HEADER_LEN;
> struct pkt_metadata *md = &packet->md;
> struct flow_tnl *tnl = &md->tunnel;
> const struct ip6_rt_hdr *rt_hdr;
> @@ -998,11 +997,18 @@ netdev_srv6_pop_header(struct dp_packet *packet)
> const void *data = nh + 1;
> uint8_t nw_frag = 0;
> unsigned int hlen;
> +size_t size;
>
> /*
> * Verifies that the routing header is present in the IPv6
> * extension headers and that its type is SRv6.
> */
> +size = dp_packet_l3_size(packet);
> +if (size < IPV6_HEADER_LEN) {
> +goto err;
> +}
> +size -= IPV6_HEADER_LEN;
> +
> if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag,
> NULL, &rt_hdr)) {
> goto err;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/7] hash: Fix integer overflow before widen in hash_finish32().
Eelco Chaudron writes:
> Fix unintentional integer overflow reported by Coverity by adding
> the ULL suffix to the numerical literals used in the multiplication.
>
> Fixes: e85e8a7541cb ("hash: Avoid 64bit crc intrinsics on 32bit aligned
> data.")
> Signed-off-by: Eelco Chaudron
> ---
Acked-by: Aaron Conole
> lib/hash.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/hash.h b/lib/hash.h
> index 307309fd0..850e031ae 100644
> --- a/lib/hash.h
> +++ b/lib/hash.h
> @@ -193,7 +193,7 @@ hash_finish32(uint64_t hash, uint32_t final, uint32_t
> semifinal)
> /* The finishing multiplier 0x805204f3 has been experimentally
> * derived to pass the testsuite hash tests. */
> hash = _mm_crc32_u32(hash, semifinal);
> -hash = _mm_crc32_u32(hash, final) * 0x805204f3;
> +hash = _mm_crc32_u32(hash, final) * 0x805204f3ULL;
> return hash ^ ((uint32_t) hash >> 16); /* Increase entropy in LSBs. */
> }
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] checkpatch: Add check for non-inclusive language.
Mark Michelson writes: > On 8/26/24 08:43, Ilya Maximets wrote: >> On 8/21/24 22:31, Mark Michelson wrote: >>> This takes the approach of hard-coding the words in a text file, rather >>> than trying to use a dynamic list of words. The inclusive naming project >>> provides a way to download their word list as JSON, but this is avoided >>> here for a few reasons: >>> >>> * Only the base form of words are provided. We would need to analyze the >>>part of speech and try to generate the other forms of the words. >>> * Some entries are more "example" than anything. For example, they >>>provide "blackhat-whitehat" as a single entry, even though you're much >>>more likely to come across the individual words, rather than this >>>specific hyphenated variety. Similarly, "whitelist" is provided in the >>>word list but "blacklist" is not. >>> >>> If it turns out that the word list updates frequently, then it may be >>> worth moving to a more dynamic approach, at the expense of accuracy. For >>> now, this seems like a nice approach. >>> >>> We do not consider this an error in checkpatch, but a warning. There are >>> some cases where non-inclusive words are acceptable, (such as >>> ovs_abort(), or referring to the "master" branch of a third-party repo). >>> This is why the warning only suggests to consider an alternative. >> Hi, Mark. I'm not sure this a right thing to do. The point of >> avoiding >> non-inclusive language is to not have instances of it in the repo. But >> this change explicitly adds all the non-inclusive words to the repo and >> that IMO contradicts the goal. >> My 2c. >> Best regards, Ilya Maximets. >> > > Your point is 100% valid. As a follow-up, do you have any ideas for > how we can add automated checks for non-inclusive language without > listing offending words in our repo? I mentioned in the commit message > the shortcomings of attempting a dynamic approach, but maybe there's > some middle ground between fully-dynamic and a static list of words > that I'm not thinking of. Unfortunately, I haven't found any prior art for this. What I mean is, I don't see any projects that have such an inclusive language check being done automatically. There have been some attempts but it doesn't seem that anything got adopted anywhere. Maybe it would be better if we could ask the inclusive language place to publish a list that is suitable for programs like codespell, checkpatch.py, enchant, etc. That might make adoption easier for us since we could pull the list dynamically and not have such language be a part of a repository. After all, if there are a list of words we wish to avoid, we need to know the list. And if we're trying to avoid keeping them in the repository, I think it stands to reason they have to exist somewhere. And having an automated means of pulling and evaluating them makes a lot of sense. An alternative might be to include the option to farm the patch text to an AI model and ask whether it seems to avoid non-inclusive terminology. That is a dynamic approach, but may flag words which aren't part of the inclusive naming list (whether that would be a positive change would need to be evaluated). > Thanks, > Mark Michelson > > ___ > 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: Correctly set ip offload flag in native tunneling.
Eelco Chaudron writes:
> On 27 Aug 2024, at 18:01, Mike Pattrick wrote:
>
>> Coverity identified the following issue
>>
>> CID 425094: (#1 of 1): Unchecked return value (CHECKED_RETURN)
>> 4. check_return: Calling dp_packet_hwol_tx_ip_csum without checking
>> return value (as is done elsewhere 9 out of 11 times).
>>
>> This appears to be a true positive, the fields getter was called instead
>> of its setter.
>>
>> Fixes: 084c8087292c ("userspace: Support VXLAN and GENEVE TSO.")
>> Reported-by: Eelco Chaudron
>> Signed-off-by: Mike Pattrick
>
> Thanks for the patch! The fix looks good to me. Is there any reason
> why none of the CI tests picked this up? I guess we need real hardware
> to verify this.
I guess we have identified a coverage gap. :)
> Cheers,
>
> Eelco
>
> Acked-by: Eelco Chaudron
>
>
>> ---
>> lib/netdev-native-tnl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
>> index 16c56608d..529d64fe1 100644
>> --- a/lib/netdev-native-tnl.c
>> +++ b/lib/netdev-native-tnl.c
>> @@ -254,7 +254,7 @@ dp_packet_tnl_ol_process(struct dp_packet *packet,
>>
>> if (IP_VER(ip->ip_ihl_ver) == 4) {
>> dp_packet_hwol_set_tx_ipv4(packet);
>> -dp_packet_hwol_tx_ip_csum(packet);
>> +dp_packet_hwol_set_tx_ip_csum(packet);
>> } else if (IP_VER(ip->ip_ihl_ver) == 6) {
>> dp_packet_hwol_set_tx_ipv6(packet);
>> }
>> --
>> 2.43.5
>
> ___
> 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 v4] ofproto-dpif-upcall: Avoid stale ukeys leaks.
Eelco Chaudron writes:
> It is observed in some environments that there are much more ukeys than
> actual DP flows. For example:
>
> $ ovs-appctl upcall/show
> system@ovs-system:
> flows : (current 7) (avg 6) (max 117) (limit 2125)
> offloaded flows : 525
> dump duration : 1063ms
> ufid enabled : true
>
> 23: (keys 3612)
> 24: (keys 3625)
> 25: (keys 3485)
>
> The revalidator threads are busy revalidating the stale ukeys leading to
> high CPU and long dump duration.
>
> This patch tracks the number of consecutive missed dumps. If four dumps
> are missed in a row, it is assumed that the datapath flow no longer
> exists, and the ukey can be deleted.
>
> Reported-by: Roi Dayan
> Co-authored-by: Han Zhou
> Co-authored-by: Roi Dayan
> Signed-off-by: Han Zhou
> Signed-off-by: Roi Dayan
> Signed-off-by: Eelco Chaudron
> ---
>
> v3: - Rewrote fix to use actual dump state, and added a tests case.
> v4: - Added reason to end of flow_del_reason.
> - Rather than time based, make it missed dumps based.
> - Changed it from a TC specific test to a generic unit test.
> ---
> ofproto/ofproto-dpif-upcall.c| 18 +
> tests/ofproto-dpif.at| 39
> utilities/usdt-scripts/flow_reval_monitor.py | 4 +-
> 3 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 4d39bc5a7..053cfd863 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -57,6 +57,7 @@ COVERAGE_DEFINE(dumped_inconsistent_flow);
> COVERAGE_DEFINE(dumped_new_flow);
> COVERAGE_DEFINE(handler_duplicate_upcall);
> COVERAGE_DEFINE(revalidate_missed_dp_flow);
> +COVERAGE_DEFINE(revalidate_missing_dp_flow);
> COVERAGE_DEFINE(ukey_dp_change);
> COVERAGE_DEFINE(ukey_invalid_stat_reset);
> COVERAGE_DEFINE(ukey_replace_contention);
> @@ -284,6 +285,7 @@ enum flow_del_reason {
> FDR_TOO_EXPENSIVE, /* Too expensive to revalidate. */
> FDR_UPDATE_FAIL,/* Datapath update failed. */
> FDR_XLATION_ERROR, /* Flow translation error. */
> +FDR_FLOW_MISSING_DP,/* Flow is missing from the datapath. */
> };
>
> /* 'udpif_key's are responsible for tracking the little bit of state udpif
> @@ -318,6 +320,7 @@ struct udpif_key {
> uint64_t dump_seq OVS_GUARDED;/* Tracks udpif->dump_seq. */
> uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */
> enum ukey_state state OVS_GUARDED;/* Tracks ukey lifetime. */
> +uint32_t missed_dumps OVS_GUARDED;/* Missed consecutive dumps. */
>
> /* 'state' debug information. */
> unsigned int state_thread OVS_GUARDED;/* Thread that transitions. */
> @@ -3040,6 +3043,21 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> result = revalidate_ukey(udpif, ukey, &stats,
> &odp_actions,
> reval_seq, &recircs,
> &del_reason);
> }
> +
> +if (ukey->dump_seq != dump_seq) {
> +ukey->missed_dumps++;
> +if (ukey->missed_dumps >= 4) {
> +/* If the flow was not dumped for 4 revalidator
> rounds,
> + * we can assume the datapath flow now longer exists
'now' => 'no'
> + * and the ukey should be deleted. */
> +COVERAGE_INC(revalidate_missing_dp_flow);
> +del_reason = FDR_FLOW_MISSING_DP;
> +result = UKEY_DELETE;
> +}
> +} else {
> +ukey->missed_dumps = 0;
> +}
> +
> if (result != UKEY_KEEP) {
> /* Clears 'recircs' if filled by revalidate_ukey(). */
> reval_op_init(&ops[n_ops++], result, udpif, ukey,
> &recircs,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 42fb66de6..9702b889a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12661,3 +12661,42 @@ AT_CHECK([ovs-appctl revalidator/resume])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Cleanup missing datapath flows])
> +
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +m4_define([ICMP_PKT], [m4_join([,],
> +[eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800)],
> +[ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> +[icmp(type=8,code=0)])])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 'actions=normal' ])
> +
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'ICMP_PKT'])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows --names | strip_used | strip_stats |
> dnl
> + strip_duration | strip_dp_hash | sort], [0], [dnl
> +flow-dump from the main thread:
> +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(src=50:54:00:00
Re: [ovs-dev] [PATCH v2 2/2] utilities: Updated dpif_nl_exec_monitor.py to debug all operations.
Eelco Chaudron writes:
> Updated the dpif_nl_exec_monitor.py utility to also optionally monitor
> the other DPIF netlink operations, i.e., DPIF_OP_FLOW_PUT,
> DPIF_OP_FLOW_DEL and DPIF_OP_FLOW_GET
>
> Signed-off-by: Eelco Chaudron
> ---
> Documentation/topics/usdt-probes.rst | 7 +-
> utilities/automake.mk | 6 +-
> ..._exec_monitor.py => dpif_op_nl_monitor.py} | 279 +-
> 3 files changed, 207 insertions(+), 85 deletions(-)
> rename utilities/usdt-scripts/{dpif_nl_exec_monitor.py =>
> dpif_op_nl_monitor.py} (72%)
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index b9a6c54b2..93bfaec12 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -239,7 +239,7 @@ DPIF_OP_FLOW_DEL operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- *None*
>
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
>
>
> dpif_netlink_operate\_\_:op_flow_execute
> @@ -260,7 +260,7 @@ DPIF_OP_FLOW_EXECUTE operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- ``utilities/usdt-scripts/dpif_nl_exec_monitor.py``
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
> - ``utilities/usdt-scripts/upcall_cost.py``
>
>
> @@ -281,7 +281,7 @@ DPIF_OP_FLOW_GET operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- *None*
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
>
>
> dpif_netlink_operate\_\_:op_flow_put
> @@ -301,6 +301,7 @@ DPIF_OP_FLOW_PUT operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
> - ``utilities/usdt-scripts/upcall_cost.py``
>
>
>
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 146b8c37f..acc1af4e0 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -22,7 +22,7 @@ scripts_SCRIPTS += \
> scripts_DATA += utilities/ovs-lib
> usdt_SCRIPTS += \
> utilities/usdt-scripts/bridge_loop.bt \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/kernel_delay.py \
> utilities/usdt-scripts/kernel_delay.rst \
> @@ -72,7 +72,7 @@ EXTRA_DIST += \
> utilities/docker/debian/Dockerfile \
> utilities/docker/debian/build-kernel-modules.sh \
> utilities/usdt-scripts/bridge_loop.bt \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/kernel_delay.py \
> utilities/usdt-scripts/kernel_delay.rst \
> @@ -147,7 +147,7 @@ FLAKE8_PYFILES += utilities/ovs-pcap.in \
> utilities/ovs-check-dead-ifs.in \
> utilities/ovs-tcpdump.in \
> utilities/ovs-pipegen.py \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/upcall_monitor.py \
> utilities/usdt-scripts/upcall_cost.py
> diff --git a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> b/utilities/usdt-scripts/dpif_op_nl_monitor.py
> similarity index 72%
> rename from utilities/usdt-scripts/dpif_nl_exec_monitor.py
> rename to utilities/usdt-scripts/dpif_op_nl_monitor.py
> index a5cf1a35a..c0e0a9fae 100755
> --- a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> +++ b/utilities/usdt-scripts/dpif_op_nl_monitor.py
> @@ -19,7 +19,8 @@
> # dpif_nl_exec_monitor.py uses the dpif_netlink_operate__:op_flow_execute
> USDT
> # probe to receive all DPIF_OP_EXECUTE operations that are queued for
> # transmission over the netlink socket. It will do some basic decoding, and
> if
> -# requested a packet dump.
> +# requested a packet dump. Note that there are also options to obtain
> +# additional information for the DPIF_OP_FLOW_* operations.
> #
> # Here is an example:
> #
> @@ -75,31 +76,9 @@
> #seq = 0xc
> #
> # The example above dumps the full netlink and packet decode. However options
> -# exist to disable this. Here is the full list of supported options:
> -#
> -# usage: dpif_nl_exec_monitor.py [-h] [--buffer-page-count NUMBER] [-D
> [DEBUG]]
> -#[-d {none,hex,decode}] [-n {none,hex,nlraw}]
> -#[-p VSWITCHD_PID] [-s [64-2048]]
> -#[-w PCAP_FILE]
> -#
> -# optional arguments:
> -# -h, --helpshow this help message and exit
> -# --buffer-page-count NUMBER
> -# Number of BPF ring buffer pages, default 1024
> -# -D [DEBUG], --debug [DEBUG]
> -# Enable eBPF debugging
> -# -d {none,hex,decode}, --packet-decode {no
Re: [ovs-dev] [PATCH v2 1/2] utilities: Update dpif_nl_exec_monitor.py to include new actions.
Eelco Chaudron writes:
> Added new actions to the get_ovs_action_attr_str() function.
We should also note that the DROP attribute changed from a userspace
only action - it means that there could be different results depending
on which script and OVS version are being used. Might need a NEWS or
other disclaimer to make users aware. I didn't find a documented
requirement that usdt scripts need to be paired with like versions of
the ovs-vswitchd (although, we probably treat it as an implicit
requirement).
> Fixes: 8bb065961e54 ("dpif: Stub out unimplemented action
> OVS_ACTION_ATTR_DEC_TTL.")
> Fixes: 3c8d069b9bc2 ("dpif: Probe support for OVS_ACTION_ATTR_DROP.")
NOTE for MAINTAINERS: if we do backport this fix, we need to be careful
around this change in particular as there was some netlink changes
related to this.
> Fixes: 1a3bd96b4fc4 ("odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.")
> Signed-off-by: Eelco Chaudron
> ---
The string change is fine, but for the future we should consider
switching to pahole to populate the ovs_action_attr string.
ex:
~/git/ovs$ pahole ./vswitchd/ovs-vswitchd -C ovs_action_attr
enum ovs_action_attr {
OVS_ACTION_ATTR_UNSPEC= 0,
OVS_ACTION_ATTR_OUTPUT= 1,
OVS_ACTION_ATTR_USERSPACE = 2,
OVS_ACTION_ATTR_SET = 3,
OVS_ACTION_ATTR_PUSH_VLAN = 4,
OVS_ACTION_ATTR_POP_VLAN = 5,
OVS_ACTION_ATTR_SAMPLE= 6,
OVS_ACTION_ATTR_RECIRC= 7,
OVS_ACTION_ATTR_HASH = 8,
OVS_ACTION_ATTR_PUSH_MPLS = 9,
OVS_ACTION_ATTR_POP_MPLS = 10,
OVS_ACTION_ATTR_SET_MASKED= 11,
OVS_ACTION_ATTR_CT= 12,
OVS_ACTION_ATTR_TRUNC = 13,
OVS_ACTION_ATTR_PUSH_ETH = 14,
OVS_ACTION_ATTR_POP_ETH = 15,
OVS_ACTION_ATTR_CT_CLEAR = 16,
OVS_ACTION_ATTR_PUSH_NSH = 17,
OVS_ACTION_ATTR_POP_NSH = 18,
OVS_ACTION_ATTR_METER = 19,
OVS_ACTION_ATTR_CLONE = 20,
OVS_ACTION_ATTR_CHECK_PKT_LEN = 21,
OVS_ACTION_ATTR_ADD_MPLS = 22,
OVS_ACTION_ATTR_DEC_TTL = 23,
OVS_ACTION_ATTR_DROP = 24,
OVS_ACTION_ATTR_TUNNEL_PUSH = 25,
OVS_ACTION_ATTR_TUNNEL_POP= 26,
OVS_ACTION_ATTR_LB_OUTPUT = 27,
__OVS_ACTION_ATTR_MAX = 28,
};
So we could do something like (untested):
ovs_action_attr = get_ovs_definitions("ovs_action_attr", ...)
Such a change would be a separate patch, as I think we can accept this
to get the enums properly in line, but the end goal should be to
eliminate all the maintenance burden on keeping these enums around.
> utilities/usdt-scripts/dpif_nl_exec_monitor.py | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> b/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> index 0a9ff8123..a5cf1a35a 100755
> --- a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> +++ b/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> @@ -468,9 +468,11 @@ def get_ovs_action_attr_str(attr):
> "OVS_ACTION_ATTR_CLONE",
> "OVS_ACTION_ATTR_CHECK_PKT_LEN",
> "OVS_ACTION_ATTR_ADD_MPLS",
> + "OVS_ACTION_ATTR_DEC_TTL",
> + "OVS_ACTION_ATTR_DROP",
> + "OVS_ACTION_ATTR_PSAMPLE",
> "OVS_ACTION_ATTR_TUNNEL_PUSH",
> "OVS_ACTION_ATTR_TUNNEL_POP",
> - "OVS_ACTION_ATTR_DROP",
> "OVS_ACTION_ATTR_LB_OUTPUT"]
> if attr < 0 or attr >= len(ovs_action_attr):
> return "".format(attr)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] conntrack: Use a per zone default limit.
Paolo Valerio writes:
> Aaron Conole writes:
>
>> Hi Paolo,
>>
>> Paolo Valerio writes:
>>
>>> Before this change the default limit, instead of being considered
>>> per-zone, was considered as a global value that every new entry was
>>> checked against during the creation. This was not the intended
>>> behavior as the default limit should be inherited by each zone instead
>>> of being an aggregate number.
>>>
>>> This change corrects that by removing the default limit from the cmap
>>> and making it global (atomic). Now, whenever a new connection needs to
>>> be committed, if default_zone_limit is set and the entry for the zone
>>> doesn't exist, a new entry for that zone is lazily created, marked as
>>> default. All subsequent packets for that zone will undergo the regular
>>> lookup process.
>>>
>>> Operations such as creation/deletion are modified accordingly taking
>>> into account this new behavior.
>>
>> I think there should be some documentation about this that includes the
>> expected behavior for end users. At least something so that users can
>> plan how they will set their zone limits.
>>
>
> "new behavior" isn't really something new, but more how it was supposed
> to be.
>
> dpctl man page describes how it was supposed to work, with things now
> aligning with kernel dp.
I guess what I mean is, it is possible that users are used to the
current behavior. Even if it is documented as working one way, but if
users are expecting one behavior and it changes, we need to make them
aware. I would say it is best to note that the per-zone limits have
been changed to be in line with the original intent.
> Did you have anything specific in mind when you refer to the
> documentation? i.e. articulating the man page, or something else.
NEWS entry.
>> Some other stuff follows.
>>
>>> Worth noting that OVS_REQUIRES(ct->ct_lock) is not a strict
>>> requirement for zone_limit_lookup_or_default(), however since the
>>> function operates under the lock and it can create an entry in the
>>> slow path, the lock requirement is enforced in order to make thread
>>> safety checks work. The function can still be moved outside the
>>> creation lock or any lock, keeping the fastpath lockless (turning
>>> zone_limit_lookup_protected() to its unprotected version) and locking
>>> only in the slow path (replacing zone_limit_create__() with
>>> zone_limit_create__().
>>>
>>> The patch also extends `conntrack - limit by zone` test in order to
>>> check the behavior, and while at it, update test's packet-out to use
>>> compose-packet function.
>>>
>>> Fixes: a7f33fdbfb67 ("conntrack: Support zone limits.")
>>> Reported-at: https://issues.redhat.com/browse/FDP-122
>>> Reported-by: Ilya Maximets
>>> Signed-off-by: Paolo Valerio
>>> ---
>>> lib/conntrack-private.h | 7 +-
>>> lib/conntrack.c | 233 +++-
>>> tests/system-traffic.at | 64 +++
>>> 3 files changed, 231 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
>>> index 2770470d1..46b212754 100644
>>> --- a/lib/conntrack-private.h
>>> +++ b/lib/conntrack-private.h
>>> @@ -198,10 +198,11 @@ enum ct_ephemeral_range {
>>> #define FOR_EACH_PORT_IN_RANGE(curr, min, max) \
>>> FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_JOIN(idx, __COUNTER__))
>>>
>>> +#define ZONE_LIMIT_CONN_DEFAULT -1
>>>
>>> struct conntrack_zone_limit {
>>> int32_t zone;
>>> -atomic_uint32_t limit;
>>> +atomic_int64_t limit;
>>
>> We change the zone limit max storage here, it seems. Maybe that should
>> be mentioned in the commit message.
>>
>
> Mentioning it makes sense, and I added a note in the commit msg, thanks.
>
> Althought the storage changed here, this will not change the user facing
> admitted values. It is essentially intended to accomodate
> ZONE_LIMIT_CONN_DEFAULT.
>
>>> atomic_count count;
>>> uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
>>> };
>>> @@ -212,6 +213,9 @@ struct conntrack {
>>> struct rculist exp_lists[N_EXP_LISTS] OVS_GUARDED;
>>> struct cmap zone_limits OVS_GUARDED;
>>> struct cmap timeout_policies OVS_GUARDED;
>>> +uint32_t zone_limit_seq OVS_GUARDED; /* Used to disambiguat
Re: [ovs-dev] [PATCH v2 2/2] utilities: Updated dpif_nl_exec_monitor.py to debug all operations.
Eelco Chaudron writes:
> Updated the dpif_nl_exec_monitor.py utility to also optionally monitor
> the other DPIF netlink operations, i.e., DPIF_OP_FLOW_PUT,
> DPIF_OP_FLOW_DEL and DPIF_OP_FLOW_GET
>
> Signed-off-by: Eelco Chaudron
> ---
Just a quick nit below. I haven't checked the enums / spelling.
> Documentation/topics/usdt-probes.rst | 7 +-
> utilities/automake.mk | 6 +-
> ..._exec_monitor.py => dpif_op_nl_monitor.py} | 279 +-
> 3 files changed, 207 insertions(+), 85 deletions(-)
> rename utilities/usdt-scripts/{dpif_nl_exec_monitor.py =>
> dpif_op_nl_monitor.py} (72%)
>
> diff --git a/Documentation/topics/usdt-probes.rst
> b/Documentation/topics/usdt-probes.rst
> index b9a6c54b2..93bfaec12 100644
> --- a/Documentation/topics/usdt-probes.rst
> +++ b/Documentation/topics/usdt-probes.rst
> @@ -239,7 +239,7 @@ DPIF_OP_FLOW_DEL operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- *None*
>
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
>
>
> dpif_netlink_operate\_\_:op_flow_execute
> @@ -260,7 +260,7 @@ DPIF_OP_FLOW_EXECUTE operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- ``utilities/usdt-scripts/dpif_nl_exec_monitor.py``
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
> - ``utilities/usdt-scripts/upcall_cost.py``
>
>
> @@ -281,7 +281,7 @@ DPIF_OP_FLOW_GET operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> -- *None*
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
>
>
> dpif_netlink_operate\_\_:op_flow_put
> @@ -301,6 +301,7 @@ DPIF_OP_FLOW_PUT operation as part of the dpif
> ``operate()`` callback.
>
> **Script references**:
>
> +- ``utilities/usdt-scripts/dpif_op_nl_monitor.py``
> - ``utilities/usdt-scripts/upcall_cost.py``
>
>
>
> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 146b8c37f..acc1af4e0 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -22,7 +22,7 @@ scripts_SCRIPTS += \
> scripts_DATA += utilities/ovs-lib
> usdt_SCRIPTS += \
> utilities/usdt-scripts/bridge_loop.bt \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/kernel_delay.py \
> utilities/usdt-scripts/kernel_delay.rst \
> @@ -72,7 +72,7 @@ EXTRA_DIST += \
> utilities/docker/debian/Dockerfile \
> utilities/docker/debian/build-kernel-modules.sh \
> utilities/usdt-scripts/bridge_loop.bt \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/kernel_delay.py \
> utilities/usdt-scripts/kernel_delay.rst \
> @@ -147,7 +147,7 @@ FLAKE8_PYFILES += utilities/ovs-pcap.in \
> utilities/ovs-check-dead-ifs.in \
> utilities/ovs-tcpdump.in \
> utilities/ovs-pipegen.py \
> - utilities/usdt-scripts/dpif_nl_exec_monitor.py \
> + utilities/usdt-scripts/dpif_op_nl_monitor.py \
> utilities/usdt-scripts/flow_reval_monitor.py \
> utilities/usdt-scripts/upcall_monitor.py \
> utilities/usdt-scripts/upcall_cost.py
> diff --git a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> b/utilities/usdt-scripts/dpif_op_nl_monitor.py
> similarity index 72%
> rename from utilities/usdt-scripts/dpif_nl_exec_monitor.py
> rename to utilities/usdt-scripts/dpif_op_nl_monitor.py
> index a5cf1a35a..c0e0a9fae 100755
> --- a/utilities/usdt-scripts/dpif_nl_exec_monitor.py
> +++ b/utilities/usdt-scripts/dpif_op_nl_monitor.py
> @@ -19,7 +19,8 @@
> # dpif_nl_exec_monitor.py uses the dpif_netlink_operate__:op_flow_execute
> USDT
Probably want to adjust this comment as well.
> # probe to receive all DPIF_OP_EXECUTE operations that are queued for
> # transmission over the netlink socket. It will do some basic decoding, and
> if
> -# requested a packet dump.
> +# requested a packet dump. Note that there are also options to obtain
> +# additional information for the DPIF_OP_FLOW_* operations.
> #
> # Here is an example:
> #
> @@ -75,31 +76,9 @@
> #seq = 0xc
> #
> # The example above dumps the full netlink and packet decode. However options
> -# exist to disable this. Here is the full list of supported options:
> -#
> -# usage: dpif_nl_exec_monitor.py [-h] [--buffer-page-count NUMBER] [-D
> [DEBUG]]
> -#[-d {none,hex,decode}] [-n {none,hex,nlraw}]
> -#[-p VSWITCHD_PID] [-s [64-2048]]
> -#[-w PCAP_FILE]
> -#
> -# optional arguments:
> -# -h, --helpshow this help message and exit
> -# --buffer-page-count NUMBER
> -# Number of BPF ring buffer pages, default 1024
> -# -D [DEBUG],
Re: [ovs-dev] [PATCH branch-3.3 2/2] Prepare for 3.3.3.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 3 +++ > configure.ac | 2 +- > debian/changelog | 6 ++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/NEWS b/NEWS > index 28bcc588d..ab7b3df96 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,3 +1,6 @@ > +v3.3.3 - xx xxx > + > + > v3.3.2 - 27 Aug 2024 > > - Bug fixes > diff --git a/configure.ac b/configure.ac > index 4b00d1878..73e297fa7 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -13,7 +13,7 @@ > # limitations under the License. > > AC_PREREQ(2.63) > -AC_INIT(openvswitch, 3.3.2, b...@openvswitch.org) > +AC_INIT(openvswitch, 3.3.3, b...@openvswitch.org) > AC_CONFIG_SRCDIR([vswitchd/ovs-vswitchd.c]) > AC_CONFIG_MACRO_DIR([m4]) > AC_CONFIG_AUX_DIR([build-aux]) > diff --git a/debian/changelog b/debian/changelog > index 313b2ebe2..0bae589a4 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -1,3 +1,9 @@ > +openvswitch (3.3.3-1) unstable; urgency=low > + [ Open vSwitch team ] > + * New upstream version > + > + -- Open vSwitch team Tue, 27 Aug 2024 14:33:26 +0200 > + > openvswitch (3.3.2-1) unstable; urgency=low > [ Open vSwitch team ] > * New upstream version ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-2.17 2/2] Prepare for 2.17.12.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 3 +++ > configure.ac | 2 +- > debian/changelog | 6 ++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/NEWS b/NEWS > index 4567adce7..4370d0727 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,3 +1,6 @@ > +v2.17.12 - xx xxx > +-- > + > v2.17.11 - 27 Aug 2024 > -- > - Bug fixes > diff --git a/configure.ac b/configure.ac > index 187431e51..bec9db9ec 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -13,7 +13,7 @@ > # limitations under the License. > > AC_PREREQ(2.63) > -AC_INIT(openvswitch, 2.17.11, b...@openvswitch.org) > +AC_INIT(openvswitch, 2.17.12, b...@openvswitch.org) > AC_CONFIG_SRCDIR([datapath/datapath.c]) > AC_CONFIG_MACRO_DIR([m4]) > AC_CONFIG_AUX_DIR([build-aux]) > diff --git a/debian/changelog b/debian/changelog > index 0251611ca..260806a46 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -1,3 +1,9 @@ > +openvswitch (2.17.12-1) unstable; urgency=low > + [ Open vSwitch team ] > + * New upstream version > + > + -- Open vSwitch team Tue, 27 Aug 2024 14:33:20 +0200 > + > openvswitch (2.17.11-1) unstable; urgency=low > [ Open vSwitch team ] > * New upstream version ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.3 1/2] Set release date for 3.3.2.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 5 - > debian/changelog | 2 +- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index f62d06ffb..28bcc588d 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,8 @@ > -v3.3.2 - xx xxx > +v3.3.2 - 27 Aug 2024 > > + - Bug fixes > + - IPsec: > + * Fixed compatibility between ovs-monitor-ipsec daemon and Libreswan 5. > > v3.3.1 - 07 Jun 2024 > > diff --git a/debian/changelog b/debian/changelog > index 82af18415..313b2ebe2 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -2,7 +2,7 @@ openvswitch (3.3.2-1) unstable; urgency=low > [ Open vSwitch team ] > * New upstream version > > - -- Open vSwitch team Fri, 07 Jun 2024 15:58:27 +0200 > + -- Open vSwitch team Tue, 27 Aug 2024 14:33:26 +0200 > > openvswitch (3.3.1-1) unstable; urgency=low > [ Open vSwitch team ] ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-2.17 1/2] Set release date for 2.17.11.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 3 ++- > debian/changelog | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index 8ba8a8d62..4567adce7 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,6 @@ > -v2.17.11 - xx xxx > +v2.17.11 - 27 Aug 2024 > -- > + - Bug fixes > > v2.17.10 - 07 Jun 2024 > -- > diff --git a/debian/changelog b/debian/changelog > index dee876165..0251611ca 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -2,7 +2,7 @@ openvswitch (2.17.11-1) unstable; urgency=low > [ Open vSwitch team ] > * New upstream version > > - -- Open vSwitch team Fri, 07 Jun 2024 15:57:48 +0200 > + -- Open vSwitch team Tue, 27 Aug 2024 14:33:20 +0200 > > openvswitch (2.17.10-1) unstable; urgency=low > [ Open vSwitch team ] ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 6/8] net/openvswitch: Use max() to simplify the code
Hongbo Li via dev writes: > Let's use max() to simplify the code and fix the > Coccinelle/coccicheck warning reported by minmax.cocci. > > Signed-off-by: Hongbo Li > --- Reviewed-by: Aaron Conole > net/openvswitch/vport.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index 8732f6e51ae5..859208df65ea 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -534,7 +534,7 @@ static int packet_length(const struct sk_buff *skb, >* account for 802.1ad. e.g. is_skb_forwardable(). >*/ > > - return length > 0 ? length : 0; > + return max(length, 0); > } > > void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.4 3/3] Prepare for 3.4.1.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 3 +++ > configure.ac | 2 +- > debian/changelog | 6 ++ > 3 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/NEWS b/NEWS > index 944c95a8d..5a1010d93 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,3 +1,6 @@ > +v3.4.1 - xx xxx > + > + > v3.4.0 - 15 Aug 2024 > > - Option '--mlockall' now only locks memory pages on fault, if possible. > diff --git a/configure.ac b/configure.ac > index 3e39120af..f5d9648f0 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -13,7 +13,7 @@ > # limitations under the License. > > AC_PREREQ(2.63) > -AC_INIT(openvswitch, 3.4.0, b...@openvswitch.org) > +AC_INIT(openvswitch, 3.4.1, b...@openvswitch.org) > AC_CONFIG_SRCDIR([vswitchd/ovs-vswitchd.c]) > AC_CONFIG_MACRO_DIR([m4]) > AC_CONFIG_AUX_DIR([build-aux]) > diff --git a/debian/changelog b/debian/changelog > index b9f9b404b..c5396cc09 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -1,3 +1,9 @@ > +openvswitch (3.4.1-1) unstable; urgency=low > + [ Open vSwitch team ] > + * New upstream version > + > + -- Open vSwitch team Thu, 15 Aug 2024 14:59:36 +0200 > + > openvswitch (3.4.0-1) unstable; urgency=low > > * New upstream version ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.4 2/3] Set release date for 3.4.0.
Ilya Maximets writes: > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole > NEWS | 2 +- > debian/changelog | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index 5290696a8..944c95a8d 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,4 +1,4 @@ > -v3.4.0 - xx xxx > +v3.4.0 - 15 Aug 2024 > > - Option '--mlockall' now only locks memory pages on fault, if possible. > This also makes it compatible with vHost Post-copy Live Migration. > diff --git a/debian/changelog b/debian/changelog > index 929b8ecf4..b9f9b404b 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -2,7 +2,7 @@ openvswitch (3.4.0-1) unstable; urgency=low > > * New upstream version > > - -- Open vSwitch team Mon, 15 Jul 2024 13:00:00 +0100 > + -- Open vSwitch team Thu, 15 Aug 2024 14:59:36 +0200 > > openvswitch (3.3.0-1) unstable; urgency=low ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-3.4 1/3] releases: Mark 3.3 as a new LTS release.
Ilya Maximets writes: > With release of OVS v3.4.0, according to our release process, > 3.3.x becomes a new LTS series. > > Signed-off-by: Ilya Maximets > --- Acked-by: Aaron Conole We also need this patch applied to `main` > Documentation/faq/releases.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst > index 9fbee90ed..7c32d7acf 100644 > --- a/Documentation/faq/releases.rst > +++ b/Documentation/faq/releases.rst > @@ -32,7 +32,7 @@ Q: What does it mean for an Open vSwitch release to be LTS > (long-term support)? > If a significant bug is identified in an LTS release, we will provide an > updated release that includes the fix. Releases that are not LTS may not > be fixed and may just be supplanted by the next major release. The > current > -LTS release is 2.17.x. > +LTS release is 3.3.x. > > For more information on the Open vSwitch release process, refer to > :doc:`/internals/release-process`. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCHv2 net-next] openvswitch: switch to per-action label counting in conntrack
Xin Long writes:
> Similar to commit 70f06c115bcc ("sched: act_ct: switch to per-action
> label counting"), we should also switch to per-action label counting
> in openvswitch conntrack, as Florian suggested.
>
> The difference is that nf_connlabels_get() is called unconditionally
> when creating an ct action in ovs_ct_copy_action(). As with these
> flows:
>
> table=0,ip,actions=ct(commit,table=1)
> table=1,ip,actions=ct(commit,exec(set_field:0xac->ct_label),table=2)
>
> it needs to make sure the label ext is created in the 1st flow before
> the ct is committed in ovs_ct_commit(). Otherwise, the warning in
> nf_ct_ext_add() when creating the label ext in the 2nd flow will
> be triggered:
>
>WARN_ON(nf_ct_is_confirmed(ct));
>
> Signed-off-by: Xin Long
> ---
Reviewed-by: Aaron Conole
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 5/5] conntrack: Use a per zone default limit.
Hi Paolo,
Paolo Valerio writes:
> Before this change the default limit, instead of being considered
> per-zone, was considered as a global value that every new entry was
> checked against during the creation. This was not the intended
> behavior as the default limit should be inherited by each zone instead
> of being an aggregate number.
>
> This change corrects that by removing the default limit from the cmap
> and making it global (atomic). Now, whenever a new connection needs to
> be committed, if default_zone_limit is set and the entry for the zone
> doesn't exist, a new entry for that zone is lazily created, marked as
> default. All subsequent packets for that zone will undergo the regular
> lookup process.
>
> Operations such as creation/deletion are modified accordingly taking
> into account this new behavior.
I think there should be some documentation about this that includes the
expected behavior for end users. At least something so that users can
plan how they will set their zone limits.
Some other stuff follows.
> Worth noting that OVS_REQUIRES(ct->ct_lock) is not a strict
> requirement for zone_limit_lookup_or_default(), however since the
> function operates under the lock and it can create an entry in the
> slow path, the lock requirement is enforced in order to make thread
> safety checks work. The function can still be moved outside the
> creation lock or any lock, keeping the fastpath lockless (turning
> zone_limit_lookup_protected() to its unprotected version) and locking
> only in the slow path (replacing zone_limit_create__() with
> zone_limit_create__().
>
> The patch also extends `conntrack - limit by zone` test in order to
> check the behavior, and while at it, update test's packet-out to use
> compose-packet function.
>
> Fixes: a7f33fdbfb67 ("conntrack: Support zone limits.")
> Reported-at: https://issues.redhat.com/browse/FDP-122
> Reported-by: Ilya Maximets
> Signed-off-by: Paolo Valerio
> ---
> lib/conntrack-private.h | 7 +-
> lib/conntrack.c | 233 +++-
> tests/system-traffic.at | 64 +++
> 3 files changed, 231 insertions(+), 73 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 2770470d1..46b212754 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -198,10 +198,11 @@ enum ct_ephemeral_range {
> #define FOR_EACH_PORT_IN_RANGE(curr, min, max) \
> FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_JOIN(idx, __COUNTER__))
>
> +#define ZONE_LIMIT_CONN_DEFAULT -1
>
> struct conntrack_zone_limit {
> int32_t zone;
> -atomic_uint32_t limit;
> +atomic_int64_t limit;
We change the zone limit max storage here, it seems. Maybe that should
be mentioned in the commit message.
> atomic_count count;
> uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
> };
> @@ -212,6 +213,9 @@ struct conntrack {
> struct rculist exp_lists[N_EXP_LISTS] OVS_GUARDED;
> struct cmap zone_limits OVS_GUARDED;
> struct cmap timeout_policies OVS_GUARDED;
> +uint32_t zone_limit_seq OVS_GUARDED; /* Used to disambiguate zone limit
> + * counts. */
> +atomic_uint32_t default_zone_limit;
>
> uint32_t hash_basis; /* Salt for hashing a connection key. */
> pthread_t clean_thread; /* Periodically cleans up connection tracker. */
> @@ -234,7 +238,6 @@ struct conntrack {
> * control context. */
>
> struct ipf *ipf; /* Fragmentation handling context. */
> -uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
> atomic_bool tcp_seq_chk; /* Check TCP sequence numbers. */
> atomic_uint32_t sweep_ms; /* Next sweep interval. */
> };
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index ac0790e11..0e128a0c6 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -270,6 +270,7 @@ conntrack_init(void)
> atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);
> atomic_init(&ct->tcp_seq_chk, true);
> atomic_init(&ct->sweep_ms, 2);
> +atomic_init(&ct->default_zone_limit, 0);
> latch_init(&ct->clean_thread_exit);
> ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct);
> ct->ipf = ipf_init();
> @@ -296,6 +297,28 @@ zone_key_hash(int32_t zone, uint32_t basis)
> return hash;
> }
>
> +static int64_t
> +zone_limit_get_limit__(struct conntrack_zone_limit *czl)
> +{
> +int64_t limit;
> +atomic_read_relaxed(&czl->limit, &limit);
> +
> +return limit;
> +}
> +
> +static int64_t
> +zone_limit_get_limit(struct conntrack *ct, struct conntrack_zone_limit *czl)
> +{
> +int64_t limit = zone_limit_get_limit__(czl);
> +
> +if (limit == ZONE_LIMIT_CONN_DEFAULT) {
> +atomic_read_relaxed(&ct->default_zone_limit, &limit);
> +limit = limit ? : -1;
> +}
> +
> +return limit;
> +}
> +
> static struct zone_limit
Re: [ovs-dev] [PATCH 4/5] conntrack: Turn zl local limit into atomic.
Paolo Valerio writes:
> while at it, changes struct zone_limit initialization in
> zone_limit_create() in order to use atomic init operations instead of
> relying on memset() which, although correctly initializes the struct,
> is semantially not aware of atomics.
semantically
> Signed-off-by: Paolo Valerio
> ---
> lib/conntrack-private.h | 2 +-
> lib/conntrack.c | 19 ---
> 2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 2c625d710..2770470d1 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -201,7 +201,7 @@ enum ct_ephemeral_range {
>
> struct conntrack_zone_limit {
> int32_t zone;
> -uint32_t limit;
> +atomic_uint32_t limit;
> atomic_count count;
> uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */
> };
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 0481a8c8a..ac0790e11 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -341,7 +341,7 @@ zone_limit_get(struct conntrack *ct, int32_t zone)
> struct zone_limit *zl = zone_limit_lookup_or_default(ct, zone);
> if (zl) {
> czl.zone = zl->czl.zone;
> -czl.limit = zl->czl.limit;
> +atomic_read_relaxed(&zl->czl.limit, &czl.limit);
> czl.count = atomic_count_get(&zl->czl.count);
> }
> return czl;
> @@ -358,8 +358,9 @@ zone_limit_create(struct conntrack *ct, int32_t zone,
> uint32_t limit)
> }
>
> if (zone >= DEFAULT_ZONE && zone <= MAX_ZONE) {
> -zl = xzalloc(sizeof *zl);
> -zl->czl.limit = limit;
> +zl = xmalloc(sizeof *zl);
> +atomic_init(&zl->czl.limit, limit);
> +atomic_count_init(&zl->czl.count, 0);
> zl->czl.zone = zone;
> zl->czl.zone_limit_seq = ct->zone_limit_seq++;
> uint32_t hash = zone_key_hash(zone, ct->hash_basis);
> @@ -376,7 +377,7 @@ zone_limit_update(struct conntrack *ct, int32_t zone,
> uint32_t limit)
> int err = 0;
> struct zone_limit *zl = zone_limit_lookup(ct, zone);
> if (zl) {
> -zl->czl.limit = limit;
> +atomic_store_relaxed(&zl->czl.limit, limit);
> VLOG_INFO("Changed zone limit of %u for zone %d", limit, zone);
> } else {
> ovs_mutex_lock(&ct->ct_lock);
> @@ -916,12 +917,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> }
>
> if (commit) {
> +uint32_t czl_limit;
> struct conn_key_node *fwd_key_node, *rev_key_node;
> struct zone_limit *zl = zone_limit_lookup_or_default(ct,
> ctx->key.zone);
> -if (zl && atomic_count_get(&zl->czl.count) >= zl->czl.limit) {
> -COVERAGE_INC(conntrack_zone_full);
> -return nc;
> +if (zl) {
> +atomic_read_relaxed(&zl->czl.limit, &czl_limit);
> +if (atomic_count_get(&zl->czl.count) >= czl_limit) {
> +COVERAGE_INC(conntrack_zone_full);
> +return nc;
> +}
> }
>
> unsigned int n_conn_limit;
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] OVS+OVN '24: Call for Participation
Hello everyone!
We are happy to announce this year's Fall Open vSwitch and OVN
conference! The conference will take place on November 20th and 21st,
2024. This is expected to be a hybrid event located in Prague, Czech
Republic at the Grandior Hotel.
We are seeking long and short ("lightning") talks on topics related to
Open vSwitch and OVN. We expect long talks to last 25 minutes with an
additional 5 minutes for questions, and short talks to last 10 minutes.
All talks will be recorded and made available online.
How to propose a talk
-
We are soliciting proposals for full talks and short ("lightning")
talks in a single round. You may propose a talk as a full talk, a
lightning talk, or for either one at the committee's discretion.
We will also accept proposals for panel discussions. Please submit
them as full talks and make it clear in the description that it is a
panel.
Please submit proposals to ovs...@openvswitch.org by September 27, 2024.
Proposals should include:
* Title and abstract.
* Speaker names, email addresses, and affiliations.
* Whether you are proposing a full talk or a lightning talk or
either one at the committee's discretion.
Speakers will be notified of acceptance by October 14, 2024.
Speakers should plan to attend the event in person. Travel and
accommodations are the responsibility of attendees. The paper review
committee may make exceptions for virtual talks, but the preference is
for talks to be presented in-person.
Topics that may be considered, among others, include:
-
* The future of Open vSwitch (e.g., AF_XDP, P4, eBPF).
* NAT, DPI, and stateful processing with Open vSwitch.
* Deploying and using OVN.
* Testing and scaling OVN.
* NIC acceleration of Open vSwitch.
* Using Open vSwitch to realize NFV and service chaining.
* Porting Open vSwitch to new operating systems, hypervisors,
or container systems.
* Integrating Open vSwitch and OVN into larger systems.
* Troubleshooting and debugging Open vSwitch installations.
* Open vSwitch development and testing practices.
* Performance measurements or approaches to improving
performance.
* End-user or service provider experiences with Open vSwitch
and OVN.
* Hardware ports of Open vSwitch (existing, in progress, or
speculative).
* The relationship between OpenFlow and Open vSwitch.
* Using, developing, or administering OpenFlow controllers in
conjunction with Open vSwitch.
* Comparisons to other implementations of features found in
Open vSwitch (e.g. other OpenFlow implementations, other
software switches, etc.).
* Increasing the size and diversity of the Open vSwitch user
and developer base.
* Tutorials and walkthroughs of use cases.
* Demos.
How to attend
-
General registration is open on Event Brite at the following URL:
https://www.eventbrite.com/e/ovsovn-2024-fall-conference-tickets-970435407427
The event will be hybrid with a fee for both in-person and virtual
attendees. We offer complementary registration to speakers, students,
academics, and anyone for whom the registration fee is burdensome.
Please contact us if you need any help obtaining a complementary
registration.
To book for the Grandior Hotel, please use the following link:
https://www.hotel-grandior.cz/en/reservations/?date_in=2024-11-20&date_out=2024-11-22&promo_code=OVN2024
Alternatively, use the code OVN2024 when registering.
More information
To reach the organizers, email ovs...@openvswitch.org. For general
discussion of the conference, please use the ovs-discuss mailing list
at ovs-disc...@openvswitch.org
As always, you can see the latest developments at http://ovscon.site
Thank you,
Conference Team
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] |fail| pw1959493 [PATCH ovn v2] northd: Fix logical router load-balancer nat rules when using DGP.
Roberto Bartzen Acosta writes: > Hey Aaron, Hi Roberto, > How are you doing? > How can I see the real reason for this error? the log message "echo '*** > ERROR: 255 ***'" > after the "podman cp" doesn't say much to me, sorry. > It seems to me that this has no relation to my patch. Can you help me > understand the > problem so I can fix it? Sure thing - in the link below there is a pointer to a build URL: https://github.com/ovsrobot/ovn/actions/runs/9897498819 And just looking at the logs, for example: main: clean up vswitch 8559 ../../../tests/ovn.at:36697: test -e $OVS_RUNDIR/ovs-vswitchd.pid 8560 ../../../tests/ovn.at:36697: ovs-appctl --timeout=10 -t ovs-vswitchd exit --cleanup 8561 ovn.at:36697: waiting while kill -0 $TMPPID 2>/dev/null... 8562 ovn.at:36697: wait succeeded quickly 8563 ../../../tests/ovn.at:36697: test -e $OVS_RUNDIR/ovsdb-server.pid 8564 ../../../tests/ovn.at:36697: ovs-appctl --timeout=10 -t ovsdb-server exit 8565 ovn.at:36697: waiting while kill -0 $TMPPID 2>/dev/null... 8566 ovn.at:36697: wait succeeded quickly 8567 Undefined Behavior Sanitizer or Address Sanitizer reported errors in: sanitizers.126090 8568 8569 = 8570 ==126090==ERROR: LeakSanitizer: detected memory leaks 8571 8572 Direct leak of 108 byte(s) in 3 object(s) allocated from: 8573 #0 0x55b195ca1110 in realloc (/workspace/ovn-tmp/ovn-24.03.90/_build/sub/northd/ovn-northd+0x377110) (BuildId: 2796c1a70d6ad795706de27129fa1f63838eb036) 8574 #1 0x55b196100888 in xrealloc__ /workspace/ovn-tmp/ovs/lib/util.c:150:9 8575 #2 0x55b196100888 in xrealloc /workspace/ovn-tmp/ovs/lib/util.c:182:12 8576 #3 0x55b1960a4801 in ds_reserve /workspace/ovn-tmp/ovs/lib/dynamic-string.c:63:22 8577 #4 0x55b1960a4801 in ds_put_format_valist /workspace/ovn-tmp/ovs/lib/dynamic-string.c:164:9 8578 #5 0x55b1960a474a in ds_put_format /workspace/ovn-tmp/ovs/lib/dynamic-string.c:142:5 8579 #6 0x55b195d98baf in build_distr_lrouter_nat_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11079:5 8580 #7 0x55b195d98baf in build_lrouter_nat_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11302:17 8581 #8 0x55b195d18f26 in build_lrouter_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11440:9 8582 #9 0x55b195cfb804 in build_lflows_thread /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:16150:21 8583 #10 0x55b1960d88ce in ovsthread_wrapper /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12 8584 #11 0x55b195c9e7dc in asan_thread_start(void*) asan_interceptors.cpp.o 8585 8586 Direct leak of 36 byte(s) in 1 object(s) allocated from: 8587 #0 0x55b195ca1110 in realloc (/workspace/ovn-tmp/ovn-24.03.90/_build/sub/northd/ovn-northd+0x377110) (BuildId: 2796c1a70d6ad795706de27129fa1f63838eb036) 8588 #1 0x55b196100888 in xrealloc__ /workspace/ovn-tmp/ovs/lib/util.c:150:9 8589 #2 0x55b196100888 in xrealloc /workspace/ovn-tmp/ovs/lib/util.c:182:12 8590 #3 0x55b1960a4801 in ds_reserve /workspace/ovn-tmp/ovs/lib/dynamic-string.c:63:22 8591 #4 0x55b1960a4801 in ds_put_format_valist /workspace/ovn-tmp/ovs/lib/dynamic-string.c:164:9 8592 #5 0x55b1960a474a in ds_put_format /workspace/ovn-tmp/ovs/lib/dynamic-string.c:142:5 8593 #6 0x55b195d98baf in build_distr_lrouter_nat_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11079:5 8594 #7 0x55b195d98baf in build_lrouter_nat_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11302:17 8595 #8 0x55b195d18f26 in build_lrouter_flows_for_lb /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:11440:9 8596 #9 0x55b195d16a60 in lflow_handle_northd_lb_changes /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/northd.c:16793:9 8597 #10 0x55b195dced9a in lflow_northd_handler /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/en-lflow.c:144:10 8598 #11 0x55b195e48bef in engine_compute /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../lib/inc-proc-eng.c:437:28 8599 #12 0x55b195e48bef in engine_run_node /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../lib/inc-proc-eng.c:499:14 8600 #13 0x55b195e48bef in engine_run /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../lib/inc-proc-eng.c:524:9 8601 #14 0x55b195e05da5 in inc_proc_northd_run /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/inc-proc-northd.c:414:5 8602 #15 0x55b195dc1bbc in main /workspace/ovn-tmp/ovn-24.03.90/_build/sub/../../northd/ovn-northd.c:970:32 8603 #16 0x7fa4617251c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) 8604 #17 0x7fa46172528a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) 8605 #18 0x55b195c05ea4 in _start (/workspace/ovn-tmp/ovn-24.03.90/_build/sub/
Re: [ovs-dev] [PATCH v1] selftests: openvswitch: retry instead of sleep
Adrian Moreno writes: > There are a couple of places where the test script "sleep"s to wait for > some external condition to be met. > > This is error prone, specially in slow systems (identified in CI by > "KSFT_MACHINE_SLOW=yes"). > > To fix this, add a "ovs_wait" function that tries to execute a command > a few times until it succeeds. The timeout used is set to 5s for > "normal" systems and doubled if a slow CI machine is detected. > > This should make the following work: > > $ vng --build \ > --config tools/testing/selftests/net/config \ > --config kernel/configs/debug.config > > $ vng --run . --user root -- "make -C tools/testing/selftests/ \ > KSFT_MACHINE_SLOW=yes TARGETS=net/openvswitch run_tests" > > Signed-off-by: Adrian Moreno > --- Looks like this does resolve the issue in question on the -dbg environment: https://netdev.bots.linux.dev/contest.html?executor=vmksft-net-dbg&test=openvswitch-sh Thanks Adrian! Also, thanks for including the fractional sleep. Reviewed-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 0/3] selftests: openvswitch: Address some flakes in the CI environment
Jakub Kicinski writes: > On Tue, 2 Jul 2024 09:28:27 -0400 Aaron Conole wrote: >> These patches aim to make using the openvswitch testsuite more reliable. >> These should address the major sources of flakiness in the openvswitch >> test suite allowing the CI infrastructure to exercise the openvswitch >> module for patch series. There should be no change for users who simply >> run the tests (except that patch 3/3 does make some of the debugging a bit >> easier by making some output more verbose). > > Hi Aaron! > > The results look solid on normal builds now, but with a debug kernel > the test is failing consistently: > > https://netdev.bots.linux.dev/contest.html?executor=vmksft-net-dbg&test=openvswitch-sh Yes - it shows a test case issue with the upcall and psample tests. Adrian and I discussed the correct approach would be using a wait_for instead of just sleeping, because it seems the dbg environment might be too racy. I think he is working on a follow up to submit after the psample work gets merged - we were hoping not to hold that patch series up with more potential conflicts or merge issues if that's okay. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] openvswitch: prepare for stolen verdict coming from conntrack and nat engine
Florian Westphal writes: > At this time, conntrack either returns NF_ACCEPT or NF_DROP. > To improve debuging it would be nice to be able to replace NF_DROP > verdict with NF_DROP_REASON() helper, > > This helper releases the skb instantly (so drop_monitor can pinpoint > precise location) and returns NF_STOLEN. > > Prepare call sites to deal with this before introducing such changes > in conntrack and nat core. > > Signed-off-by: Florian Westphal > --- Reviewed-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] openvswitch: prepare for stolen verdict coming from conntrack and nat engine
Florian Westphal writes:
> Aaron Conole wrote:
>> > verdict with NF_DROP_REASON() helper,
>> >
>> > This helper releases the skb instantly (so drop_monitor can pinpoint
>> > precise location) and returns NF_STOLEN.
>> >
>> > Prepare call sites to deal with this before introducing such changes
>> > in conntrack and nat core.
>> >
>> > Signed-off-by: Florian Westphal
>> > ---
>>
>> AFAIU, these changes are only impacting the existing NF_DROP cases, and
>> won't impact how ovs + netfilter communicate about invalid packets. One
>> important thing to note is that we rely on:
>>
>> * Note that if the packet is deemed invalid by conntrack, skb->_nfct will be
>> * set to NULL and 0 will be returned.
>
> Right, this is about how to communicate 'packet dropped'.
>
> NF_DROP means 'please call kfree_skb for me'. Problem from introspection
> point
> of view is that drop monitor will blame nf_hook_slow() (for netfilter)
> and ovs resp. act_ct for the drop.
>
> Plan is to allow conntrack/nat engine to return STOLEN verdict ("skb
> might have been free'd already").
>
> Example change:
> @@ -52,10 +53,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb,
> unsigned int hooknum,
> rt = skb_rtable(skb);
> nh = rt_nexthop(rt, ip_hdr(skb)->daddr);
> newsrc = inet_select_addr(out, nh, RT_SCOPE_UNIVERSE);
> - if (!newsrc) {
> - pr_info("%s ate my IP address\n", out->name);
> - return NF_DROP;
> - }
> + if (!newsrc)
> + return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP,
> EADDRNOTAVAIL);
>
>
> Where NF_DROP_REASON() is:
>
> static __always_inline int
> NF_DROP_REASON(struct sk_buff *skb, enum skb_drop_reason reason, u32 err)
> {
> BUILD_BUG_ON(err > 0x);
>
> kfree_skb_reason(skb, reason);
>
> return ((err << 16) | NF_STOLEN);
> }
>
> So drop monitoring tools will blame
> nf_nat_masquerade.c:nf_nat_masquerade_ipv4 and not
> the consumer of the NF_DROP verdict.
>
> I can't make such changes ATM because ovs and act_ct assume conntrack
> returns only ACCEPT and DROP, so we'd get double-free. Hope that makes
> sense.
>
> Thanks!
Makes sense to me, thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next] openvswitch: prepare for stolen verdict coming from conntrack and nat engine
Hi Florian,
Florian Westphal writes:
> At this time, conntrack either returns NF_ACCEPT or NF_DROP.
> To improve debuging it would be nice to be able to replace NF_DROP
> verdict with NF_DROP_REASON() helper,
>
> This helper releases the skb instantly (so drop_monitor can pinpoint
> precise location) and returns NF_STOLEN.
>
> Prepare call sites to deal with this before introducing such changes
> in conntrack and nat core.
>
> Signed-off-by: Florian Westphal
> ---
AFAIU, these changes are only impacting the existing NF_DROP cases, and
won't impact how ovs + netfilter communicate about invalid packets. One
important thing to note is that we rely on:
* Note that if the packet is deemed invalid by conntrack, skb->_nfct will be
* set to NULL and 0 will be returned.
Based on this, my understanding is if packet isn't part of a valid
connection, skb->_nfct is NULL and NF_ACCEPT is returned.
If this changes, those flow pipelines matching on ct_state(+inv+trk)
will no longer function as expected since we will bail early. I think
this comment will also apply to the act_ct change as well.
> net/openvswitch/conntrack.c | 47 +
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3b980bf2770b..8eb1d644b741 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -679,6 +679,8 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key
> *key,
> action |= BIT(NF_NAT_MANIP_DST);
>
> err = nf_ct_nat(skb, ct, ctinfo, &action, &info->range, info->commit);
> + if (err != NF_ACCEPT)
> + return err;
>
> if (action & BIT(NF_NAT_MANIP_SRC))
> ovs_nat_update_key(key, skb, NF_NAT_MANIP_SRC);
> @@ -697,6 +699,22 @@ static int ovs_ct_nat(struct net *net, struct
> sw_flow_key *key,
> }
> #endif
>
> +static int verdict_to_errno(unsigned int verdict)
> +{
> + switch (verdict & NF_VERDICT_MASK) {
> + case NF_ACCEPT:
> + return 0;
> + case NF_DROP:
> + return -EINVAL;
> + case NF_STOLEN:
> + return -EINPROGRESS;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> /* Pass 'skb' through conntrack in 'net', using zone configured in 'info', if
> * not done already. Update key with new CT state after passing the packet
> * through conntrack.
> @@ -735,7 +753,7 @@ static int __ovs_ct_lookup(struct net *net, struct
> sw_flow_key *key,
>
> err = nf_conntrack_in(skb, &state);
> if (err != NF_ACCEPT)
> - return -ENOENT;
> + return verdict_to_errno(err);
>
> /* Clear CT state NAT flags to mark that we have not yet done
>* NAT after the nf_conntrack_in() call. We can actually clear
> @@ -762,9 +780,12 @@ static int __ovs_ct_lookup(struct net *net, struct
> sw_flow_key *key,
>* the key->ct_state.
>*/
> if (info->nat && !(key->ct_state & OVS_CS_F_NAT_MASK) &&
> - (nf_ct_is_confirmed(ct) || info->commit) &&
> - ovs_ct_nat(net, key, info, skb, ct, ctinfo) != NF_ACCEPT) {
> - return -EINVAL;
> + (nf_ct_is_confirmed(ct) || info->commit)) {
> + int err = ovs_ct_nat(net, key, info, skb, ct, ctinfo);
> +
> + err = verdict_to_errno(err);
> + if (err)
> + return err;
> }
>
> /* Userspace may decide to perform a ct lookup without a helper
> @@ -795,9 +816,12 @@ static int __ovs_ct_lookup(struct net *net, struct
> sw_flow_key *key,
>* - When committing an unconfirmed connection.
>*/
> if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
> - info->commit) &&
> - nf_ct_helper(skb, ct, ctinfo, info->family) != NF_ACCEPT) {
> - return -EINVAL;
> + info->commit)) {
> + int err = nf_ct_helper(skb, ct, ctinfo, info->family);
> +
> + err = verdict_to_errno(err);
> + if (err)
> + return err;
> }
>
> if (nf_ct_protonum(ct) == IPPROTO_TCP &&
> @@ -1001,10 +1025,9 @@ static int ovs_ct_commit(struct net *net, struct
> sw_flow_key *key,
> /* This will take care of sending queued events even if the connection
>* is already confirmed.
>*/
> - if (nf_conntrack_confirm(skb) != NF_ACCEPT)
> - return -EINVAL;
> + err = nf_conntrack_confirm(skb);
>
> - return 0;
> + return verdict_to_errno(err);
> }
>
> /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
> @@ -1039,6 +1062,10 @@
[ovs-dev] [PATCH net-next 3/3] selftests: openvswitch: Be more verbose with selftest debugging.
The openvswitch selftest is difficult to debug for anyone that isn't
directly familiar with the openvswitch module and the specifics of the
test cases. Many times when something fails, the debug log will be
sparsely populated and it takes some time to understand where a failure
occured.
Increase the amount of details logged to the debug log by trapping all
'info' logs, and all 'ovs_sbx' commands.
Signed-off-by: Aaron Conole
---
NOTE: There is a conflict here with a patch on list that adds psample
support, but it should be simple to resolve, since the conflict
would be due to a context change in tests="". I can also respin
if the patches collide.
tools/testing/selftests/net/openvswitch/openvswitch.sh | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 0bd0425848d9..531951086d9c 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -23,7 +23,9 @@ tests="
drop_reason drop: test drop reasons are
emitted"
info() {
-[ $VERBOSE = 0 ] || echo $*
+ [ "${ovs_dir}" != "" ] &&
+ echo "`date +"[%m-%d %H:%M:%S]"` $*" >> ${ovs_dir}/debug.log
+ [ $VERBOSE = 0 ] || echo $*
}
ovs_base=`pwd`
@@ -65,7 +67,8 @@ ovs_setenv() {
ovs_sbx() {
if test "X$2" != X; then
- (ovs_setenv $1; shift; "$@" >> ${ovs_dir}/debug.log)
+ (ovs_setenv $1; shift;
+info "run cmd: $@"; "$@" >> ${ovs_dir}/debug.log)
else
ovs_setenv $1
fi
@@ -139,7 +142,7 @@ ovs_add_flow () {
info "Adding flow to DP: sbx:$1 br:$2 flow:$3 act:$4"
ovs_sbx "$1" python3 $ovs_base/ovs-dpctl.py add-flow "$2" "$3" "$4"
if [ $? -ne 0 ]; then
- echo "Flow [ $3 : $4 ] failed" >> ${ovs_dir}/debug.log
+ info "Flow [ $3 : $4 ] failed"
return 1
fi
return 0
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 2/3] selftests: openvswitch: Attempt to autoload module.
Previously, the openvswitch.sh test suites would not attempt to autoload
the openvswitch module. The idea was that a user who is manually running
tests might not even have the OVS module loaded or configured for their
own development. However, if the kernel module is configured, and the
module can be autoloaded then we should just attempt to load it and run
the tests. This is especially true in the CI environments, where the CI
tests should be able to rely on auto loading to get the test suite running.
Signed-off-by: Aaron Conole
---
.../selftests/net/openvswitch/openvswitch.sh | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index 15bca0708717..0bd0425848d9 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -613,16 +613,20 @@ run_test() {
tname="$1"
tdesc="$2"
- if ! lsmod | grep openvswitch >/dev/null 2>&1; then
- stdbuf -o0 printf "TEST: %-60s [NOMOD]\n" "${tdesc}"
- return $ksft_skip
- fi
-
if python3 ovs-dpctl.py -h 2>&1 | \
grep -E "Need to (install|upgrade) the python" >/dev/null 2>&1;
then
stdbuf -o0 printf "TEST: %-60s [PYLIB]\n" "${tdesc}"
return $ksft_skip
fi
+
+ python3 ovs-dpctl.py show >/dev/null 2>&1 || \
+ echo "[DPCTL] show exception."
+
+ if ! lsmod | grep openvswitch >/dev/null 2>&1; then
+ stdbuf -o0 printf "TEST: %-60s [NOMOD]\n" "${tdesc}"
+ return $ksft_skip
+ fi
+
printf "TEST: %-60s [START]\n" "${tname}"
unset IFS
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 1/3] selftests: openvswitch: Bump timeout to 15 minutes.
We found that since some tests rely on the TCP SYN timeouts to cause flow misses, the default test suite timeout of 45 seconds is quick to be exceeded. Bump the timeout to 15 minutes. Signed-off-by: Aaron Conole --- tools/testing/selftests/net/openvswitch/settings | 1 + 1 file changed, 1 insertion(+) create mode 100644 tools/testing/selftests/net/openvswitch/settings diff --git a/tools/testing/selftests/net/openvswitch/settings b/tools/testing/selftests/net/openvswitch/settings new file mode 100644 index ..e2206265f67c --- /dev/null +++ b/tools/testing/selftests/net/openvswitch/settings @@ -0,0 +1 @@ +timeout=900 -- 2.45.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next 0/3] selftests: openvswitch: Address some flakes in the CI environment
These patches aim to make using the openvswitch testsuite more reliable. These should address the major sources of flakiness in the openvswitch test suite allowing the CI infrastructure to exercise the openvswitch module for patch series. There should be no change for users who simply run the tests (except that patch 3/3 does make some of the debugging a bit easier by making some output more verbose). Aaron Conole (3): selftests: openvswitch: Bump timeout to 15 minutes. selftests: openvswitch: Attempt to autoload module. selftests: openvswitch: Be more verbose with selftest debugging. .../selftests/net/openvswitch/openvswitch.sh | 23 --- .../selftests/net/openvswitch/settings| 1 + 2 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 tools/testing/selftests/net/openvswitch/settings -- 2.45.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v8 05/10] net: openvswitch: add psample action
Adrian Moreno writes: > Add support for a new action: psample. > > This action accepts a u32 group id and a variable-length cookie and uses > the psample multicast group to make the packet available for > observability. > > The maximum length of the user-defined cookie is set to 16, same as > tc_cookie, to discourage using cookies that will not be offloadable. > > Reviewed-by: Ilya Maximets > Acked-by: Eelco Chaudron > Signed-off-by: Adrian Moreno > --- Reviewed-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 10/10] selftests: openvswitch: add psample test
Adrián Moreno writes:
> On Mon, Jul 01, 2024 at 02:38:44PM GMT, Aaron Conole wrote:
>> Adrian Moreno writes:
>>
>> > Add a test to verify sampling packets via psample works.
>> >
>> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
>> > on the psample multicast group and print samples.
>> >
>> > Signed-off-by: Adrian Moreno
>> > ---
>> > .../selftests/net/openvswitch/openvswitch.sh | 115 +-
>> > .../selftests/net/openvswitch/ovs-dpctl.py| 73 ++-
>> > 2 files changed, 182 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > index 15bca0708717..02a366e01004 100755
>> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
>> > @@ -20,7 +20,8 @@ tests="
>> >nat_related_v4 ip4-nat-related: ICMP related
>> > matches work with SNAT
>> >netlink_checks ovsnl: validate netlink attrs
>> > and settings
>> >upcall_interfaces ovs: test the upcall interfaces
>> > - drop_reason drop: test drop reasons are
>> > emitted"
>> > + drop_reason drop: test drop reasons are
>> > emitted
>> > + psample psample: Sampling packets with
>> > psample"
>> >
>> > info() {
>> > [ $VERBOSE = 0 ] || echo $*
>> > @@ -102,12 +103,21 @@ ovs_netns_spawn_daemon() {
>> >shift
>> >netns=$1
>> >shift
>> > - info "spawning cmd: $*"
>> > - ip netns exec $netns $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
>> > + if [ "$netns" == "_default" ]; then
>> > + $* >> $ovs_dir/stdout 2>> $ovs_dir/stderr &
>> > + else
>> > + ip netns exec $netns $* >> $ovs_dir/stdout 2>>
>> > $ovs_dir/stderr &
>> > + fi
>> >pid=$!
>> >ovs_sbx "$sbx" on_exit "kill -TERM $pid 2>/dev/null"
>> > }
>> >
>> > +ovs_spawn_daemon() {
>> > + sbx=$1
>> > + shift
>> > + ovs_netns_spawn_daemon $sbx "_default" $*
>> > +}
>> > +
>> > ovs_add_netns_and_veths () {
>> >info "Adding netns attached: sbx:$1 dp:$2 {$3, $4, $5}"
>> >ovs_sbx "$1" ip netns add "$3" || return 1
>> > @@ -170,6 +180,19 @@ ovs_drop_reason_count()
>> >return `echo "$perf_output" | grep "$pattern" | wc -l`
>> > }
>> >
>> > +ovs_test_flow_fails () {
>> > + ERR_MSG="Flow actions may not be safe on all matching packets"
>> > +
>> > + PRE_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > + ovs_add_flow $@ &> /dev/null $@ && return 1
>> > + POST_TEST=$(dmesg | grep -c "${ERR_MSG}")
>> > +
>> > + if [ "$PRE_TEST" == "$POST_TEST" ]; then
>> > + return 1
>> > + fi
>> > + return 0
>> > +}
>> > +
>> > usage() {
>> >echo
>> >echo "$0 [OPTIONS] [TEST]..."
>> > @@ -184,6 +207,92 @@ usage() {
>> >exit 1
>> > }
>> >
>> > +
>> > +# psample test
>> > +# - use psample to observe packets
>> > +test_psample() {
>> > + sbx_add "test_psample" || return $?
>> > +
>> > + # Add a datapath with per-vport dispatching.
>> > + ovs_add_dp "test_psample" psample -V 2:1 || return 1
>> > +
>> > + info "create namespaces"
>> > + ovs_add_netns_and_veths "test_psample" "psample" \
>> > + client c0 c1 172.31.110.10/24 -u || return 1
>> > + ovs_add_netns_and_veths "test_psample" "psample" \
>> > + server s0 s1 172.31.110.20/24 -u || return 1
>> > +
>> > + # Check if psample actions can be configured.
>> > + ovs_add_flow "test_psample" psample \
>> > + 'in_port(1),eth(),eth_type(0x0806),arp()' 'psample(group=1)'
>>
>> Might be good to redirect t
Re: [ovs-dev] [PATCH net-next v7 07/10] selftests: openvswitch: add psample action
Adrian Moreno writes:
> Add sample and psample action support to ovs-dpctl.py.
>
> Refactor common attribute parsing logic into an external function.
>
> Signed-off-by: Adrian Moreno
> ---
Reviewed-by: Aaron Conole
> .../selftests/net/openvswitch/ovs-dpctl.py| 162 +-
> 1 file changed, 161 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 182a09975975..dcc400a21a22 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -8,6 +8,7 @@ import argparse
> import errno
> import ipaddress
> import logging
> +import math
> import multiprocessing
> import re
> import socket
> @@ -60,6 +61,7 @@ OVS_FLOW_CMD_DEL = 2
> OVS_FLOW_CMD_GET = 3
> OVS_FLOW_CMD_SET = 4
>
> +UINT32_MAX = 0x
>
> def macstr(mac):
> outstr = ":".join(["%02X" % i for i in mac])
> @@ -281,6 +283,75 @@ def parse_extract_field(
> return str_skipped, data
>
>
> +def parse_attrs(actstr, attr_desc):
> +"""Parses the given action string and returns a list of netlink
> +attributes based on a list of attribute descriptions.
> +
> +Each element in the attribute description list is a tuple such as:
> +(name, attr_name, parse_func)
> +where:
> +name: is the string representing the attribute
> +attr_name: is the name of the attribute as defined in the uAPI.
> +parse_func: is a callable accepting a string and returning either
> +a single object (the parsed attribute value) or a tuple of
> +two values (the parsed attribute value and the remaining string)
> +
> +Returns a list of attributes and the remaining string.
> +"""
> +def parse_attr(actstr, key, func):
> +actstr = actstr[len(key) :]
> +
> +if not func:
> +return None, actstr
> +
> +delim = actstr[0]
> +actstr = actstr[1:]
> +
> +if delim == "=":
> +pos = strcspn(actstr, ",)")
> +ret = func(actstr[:pos])
> +else:
> +ret = func(actstr)
> +
> +if isinstance(ret, tuple):
> +(datum, actstr) = ret
> +else:
> +datum = ret
> +actstr = actstr[strcspn(actstr, ",)"):]
> +
> +if delim == "(":
> +if not actstr or actstr[0] != ")":
> +raise ValueError("Action contains unbalanced parentheses")
> +
> +actstr = actstr[1:]
> +
> +actstr = actstr[strspn(actstr, ", ") :]
> +
> +return datum, actstr
> +
> +attrs = []
> +attr_desc = list(attr_desc)
> +while actstr and actstr[0] != ")" and attr_desc:
> +found = False
> +for i, (key, attr, func) in enumerate(attr_desc):
> +if actstr.startswith(key):
> +datum, actstr = parse_attr(actstr, key, func)
> +attrs.append([attr, datum])
> +found = True
> +del attr_desc[i]
> +
> +if not found:
> +raise ValueError("Unknown attribute: '%s'" % actstr)
> +
> +actstr = actstr[strspn(actstr, ", ") :]
> +
> +if actstr[0] != ")":
> +raise ValueError("Action string contains extra garbage or has "
> + "unbalanced parenthesis: '%s'" % actstr)
> +
> +return attrs, actstr[1:]
> +
> +
> class ovs_dp_msg(genlmsg):
> # include the OVS version
> # We need a custom header rather than just being able to rely on
> @@ -299,7 +370,7 @@ class ovsactions(nla):
> ("OVS_ACTION_ATTR_SET", "ovskey"),
> ("OVS_ACTION_ATTR_PUSH_VLAN", "none"),
> ("OVS_ACTION_ATTR_POP_VLAN", "flag"),
> -("OVS_ACTION_ATTR_SAMPLE", "none"),
> +("OVS_ACTION_ATTR_SAMPLE", "sample"),
> ("OVS_ACTION_ATTR_RECIRC", "uint32"),
> ("OVS_ACTION_ATTR_HASH", "none"),
> ("OVS_ACTION_ATTR_PUSH_MPLS", "none"),
> @@ -318,8 +389,85 @@ class ovsactions(nla):
> ("OVS_ACTION_ATTR_ADD_MPLS", "none"),
> ("OVS_ACTION_ATTR_DEC_TTL", "none"),
> ("OVS_ACTION_ATTR_DROP", "uint
Re: [ovs-dev] [PATCH net-next v7 10/10] selftests: openvswitch: add psample test
> + min_key='in_port(1),eth(),eth_type(0x0800),ipv4()'
> + for testcase in \
> + "cookie to
> large"*"psample(group=1,cookie=1615141312111009080706050403020100)" \
> + "no group with cookie"*"psample(cookie=abcd)" \
> + "no group"*"psample()";
> + do
> + set -- $testcase;
> + ovs_test_flow_fails "test_psample" psample $min_key $2
> + if [ $? == 1 ]; then
> + info "failed - $1"
> + return 1
> + fi
> + done
> + IFS=$OLDIFS
> +
> + ovs_del_flows "test_psample" psample
> + # Allow ARP
> + ovs_add_flow "test_psample" psample \
> + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
> + ovs_add_flow "test_psample" psample \
> + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
> +
> + # Sample first 14 bytes of all traffic.
> + ovs_add_flow "test_psample" psample \
> + "in_port(1),eth(),eth_type(0x0800),ipv4()" \
> +"trunc(14),psample(group=1,cookie=c0ffee),2"
> +
> + # Sample all traffic. In this case, use a sample() action with both
> + # psample and an upcall emulating simultaneous local sampling and
> + # sFlow / IPFIX.
> + nlpid=$(grep -E "listening on upcall packet handler" \
> +$ovs_dir/s0.out | cut -d ":" -f 2 | tr -d ' ')
> +
> + ovs_add_flow "test_psample" psample \
> +"in_port(2),eth(),eth_type(0x0800),ipv4()" \
> +
> "sample(sample=100%,actions(psample(group=2,cookie=eeff0c),userspace(pid=${nlpid},userdata=eeff0c))),1"
> +
> + # Record psample data.
> + ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py
> psample-events
> +
> + # Send a single ping.
> + sleep 1
> + ovs_sbx "test_psample" ip netns exec client ping -I c1 172.31.110.20 -c
> 1 || return 1
> + sleep 1
> +
> + # We should have received one userspace action upcall and 2 psample
> packets.
> + grep -E "userspace action command" $ovs_dir/s0.out >/dev/null 2>&1 ||
> return 1
I wonder if it would be better to check a few times instead of the one
shot sleep. There are some constrained environments that may run this
test, and if you're worried about some kinds of races, maybe it makes
sense to check a few times?
Outside of that:
Reviewed-by: Aaron Conole
> +
> + # client -> server samples should only contain the first 14 bytes of
> the packet.
> + grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
> + $ovs_dir/stdout >/dev/null 2>&1 || return 1
> + grep -E "rate:4294967295,group:2,cookie:eeff0c" \
> + $ovs_dir/stdout >/dev/null 2>&1 || return 1
> +
> + return 0
> +}
> +
> # drop_reason test
> # - drop packets and verify the right drop reason is reported
> test_drop_reason() {
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index e8dc9af10d4d..1e15b0818074 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -28,8 +28,10 @@ try:
> from pyroute2.netlink import genlmsg
> from pyroute2.netlink import nla
> from pyroute2.netlink import nlmsg_atoms
> +from pyroute2.netlink.event import EventSocket
> from pyroute2.netlink.exceptions import NetlinkError
> from pyroute2.netlink.generic import GenericNetlinkSocket
> +from pyroute2.netlink.nlsocket import Marshal
> import pyroute2
> import pyroute2.iproute
>
> @@ -2460,10 +2462,70 @@ class OvsFlow(GenericNetlinkSocket):
> print("MISS upcall[%d/%s]: %s" % (seq, pktpres, keystr), flush=True)
>
> def execute(self, packetmsg):
> -print("userspace execute command")
> +print("userspace execute command", flush=True)
>
> def action(self, packetmsg):
> -print("userspace action command")
> +print("userspace action command", flush=True)
> +
> +
> +class psample_sample(genlmsg):
> +nla_map = (
> +("PSAMPLE_ATTR_IIFINDEX", "none"),
> +("PSAMPLE_ATTR_OIFINDEX", "none"),
> +("PSAMPLE_ATTR_ORIGSIZE"
Re: [ovs-dev] [PATCH net-next v7 06/10] net: openvswitch: store sampling probability in cb.
Adrian Moreno writes:
> When a packet sample is observed, the sampling rate that was used is
> important to estimate the real frequency of such event.
>
> Store the probability of the parent sample action in the skb's cb area
> and use it in psample action to pass it down to psample module.
>
> Acked-by: Eelco Chaudron
> Reviewed-by: Ilya Maximets
> Signed-off-by: Adrian Moreno
> ---
Reviewed-by: Aaron Conole
> include/uapi/linux/openvswitch.h | 3 ++-
> net/openvswitch/actions.c| 20 +---
> net/openvswitch/datapath.h | 3 +++
> net/openvswitch/vport.c | 1 +
> 4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index 3dd653748725..3a701bd1f31b 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -649,7 +649,8 @@ enum ovs_flow_attr {
> * Actions are passed as nested attributes.
> *
> * Executes the specified actions with the given probability on a per-packet
> - * basis.
> + * basis. Nested actions will be able to access the probability value of the
> + * parent @OVS_ACTION_ATTR_SAMPLE.
> */
> enum ovs_sample_attr {
> OVS_SAMPLE_ATTR_UNSPEC,
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index a035b7e677dd..34af6bce4085 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -1048,12 +1048,15 @@ static int sample(struct datapath *dp, struct sk_buff
> *skb,
> struct nlattr *sample_arg;
> int rem = nla_len(attr);
> const struct sample_arg *arg;
> + u32 init_probability;
> bool clone_flow_key;
> + int err;
>
> /* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
> sample_arg = nla_data(attr);
> arg = nla_data(sample_arg);
> actions = nla_next(sample_arg, &rem);
> + init_probability = OVS_CB(skb)->probability;
>
> if ((arg->probability != U32_MAX) &&
> (!arg->probability || get_random_u32() > arg->probability)) {
> @@ -1062,9 +1065,16 @@ static int sample(struct datapath *dp, struct sk_buff
> *skb,
> return 0;
> }
>
> + OVS_CB(skb)->probability = arg->probability;
> +
> clone_flow_key = !arg->exec;
> - return clone_execute(dp, skb, key, 0, actions, rem, last,
> - clone_flow_key);
> + err = clone_execute(dp, skb, key, 0, actions, rem, last,
> + clone_flow_key);
> +
> + if (!last)
> + OVS_CB(skb)->probability = init_probability;
> +
> + return err;
> }
>
> /* When 'last' is true, clone() should always consume the 'skb'.
> @@ -1311,6 +1321,7 @@ static void execute_psample(struct datapath *dp, struct
> sk_buff *skb,
> struct psample_group psample_group = {};
> struct psample_metadata md = {};
> const struct nlattr *a;
> + u32 rate;
> int rem;
>
> nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
> @@ -1329,8 +1340,11 @@ static void execute_psample(struct datapath *dp,
> struct sk_buff *skb,
> psample_group.net = ovs_dp_get_net(dp);
> md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> + md.rate_as_probability = 1;
> +
> + rate = OVS_CB(skb)->probability ? OVS_CB(skb)->probability : U32_MAX;
>
> - psample_sample_packet(&psample_group, skb, 0, &md);
> + psample_sample_packet(&psample_group, skb, rate, &md);
> }
> #else
> static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index 0cd29971a907..9ca6231ea647 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -115,12 +115,15 @@ struct datapath {
> * fragmented.
> * @acts_origlen: The netlink size of the flow actions applied to this skb.
> * @cutlen: The number of bytes from the packet end to be removed.
> + * @probability: The sampling probability that was applied to this skb; 0
> means
> + * no sampling has occurred; U32_MAX means 100% probability.
> */
> struct ovs_skb_cb {
> struct vport*input_vport;
> u16 mru;
> u16 acts_origlen;
> u32 cutlen;
> + u32 probability;
> };
> #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)
>
> diff --git a/net/openvswitch/vpo
Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action
Adrian Moreno writes:
> Add support for a new action: psample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
>
> Acked-by: Eelco Chaudron
> Signed-off-by: Adrian Moreno
> ---
Hi Adrian,
Just some nits below.
> Documentation/netlink/specs/ovs_flow.yaml | 17
> include/uapi/linux/openvswitch.h | 28 ++
> net/openvswitch/Kconfig | 1 +
> net/openvswitch/actions.c | 47 +++
> net/openvswitch/flow_netlink.c| 32 ++-
> 5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
> name: dec-ttl
> type: nest
> nested-attributes: dec-ttl-attrs
> + -
> +name: psample
> +type: nest
> +nested-attributes: psample-attrs
> +doc: |
> + Sends a packet sample to psample for external observation.
>-
> name: tunnel-key-attrs
> enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>-
> name: gbp
> type: u32
> + -
> +name: psample-attrs
> +enum-name: ovs-psample-attr
> +name-prefix: ovs-psample-attr-
> +attributes:
> + -
> +name: group
> +type: u32
> + -
> +name: cookie
> +type: binary
>
> operations:
>name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..3dd653748725 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_psample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> + * action.
> + *
> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> + * contains user-defined metadata. The maximum length is
> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group
> and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> + */
> +enum ovs_psample_attr {
> + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_PSAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> +
> + /* private: */
> + __OVS_PSAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> observers
> + * via psample.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> OVS_ACTION_ATTR_DROP, /* u32 error code. */
> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> * from userspace. */
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 29a7081858cd..2535f3f9f462 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -10,6 +10,7 @@ config OPENVSWITCH
> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>(!NF_NAT || NF_NAT) && \
>(!NETFILTER_CONNCOUNT ||
> NETFILTER_CONNCOUNT)))
> + depends on PSAMPLE || !PSAMPLE
> select LIBCRC32C
> select MPLS
> select NET_MPLS_GSO
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 964225580824..a035b7e677dd 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -24,6 +24,11 @@
> #include
> #include
> #include
> +
> +#if IS_ENABLED(CONFIG_PSAMPLE)
> +#include
> +#endif
> +
> #include
>
> #include "datapath.h"
> @@ -1299,6 +1304,39 @@ static int ex
Re: [ovs-dev] [PATCH net-next v7 04/10] net: psample: allow using rate as probability
Adrian Moreno writes:
> Although not explicitly documented in the psample module itself, the
> definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.
>
> Quoting tc-sample(8):
> "RATE of 100 will lead to an average of one sampled packet out of every
> 100 observed."
>
> With this semantics, the rates that we can express with an unsigned
> 32-bits number are very unevenly distributed and concentrated towards
> "sampling few packets".
> For example, we can express a probability of 2.32E-8% but we
> cannot express anything between 100% and 50%.
>
> For sampling applications that are capable of sampling a decent
> amount of packets, this sampling rate semantics is not very useful.
>
> Add a new flag to the uAPI that indicates that the sampling rate is
> expressed in scaled probability, this is:
> - 0 is 0% probability, no packets get sampled.
> - U32_MAX is 100% probability, all packets get sampled.
>
> Acked-by: Eelco Chaudron
> Reviewed-by: Ido Schimmel
> Signed-off-by: Adrian Moreno
> ---
Reviewed-by: Aaron Conole
> include/net/psample.h| 3 ++-
> include/uapi/linux/psample.h | 10 +-
> net/psample/psample.c| 3 +++
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 2ac71260a546..c52e9ebd88dd 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -24,7 +24,8 @@ struct psample_metadata {
> u8 out_tc_valid:1,
> out_tc_occ_valid:1,
> latency_valid:1,
> -unused:5;
> +rate_as_probability:1,
> +unused:4;
> const u8 *user_cookie;
> u32 user_cookie_len;
> };
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e80637e1d97b..b765f0e81f20 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -8,7 +8,11 @@ enum {
> PSAMPLE_ATTR_ORIGSIZE,
> PSAMPLE_ATTR_SAMPLE_GROUP,
> PSAMPLE_ATTR_GROUP_SEQ,
> - PSAMPLE_ATTR_SAMPLE_RATE,
> + PSAMPLE_ATTR_SAMPLE_RATE, /* u32, ratio between observed and
> + * sampled packets or scaled probability
> + * if PSAMPLE_ATTR_SAMPLE_PROBABILITY
> + * is set.
> + */
> PSAMPLE_ATTR_DATA,
> PSAMPLE_ATTR_GROUP_REFCOUNT,
> PSAMPLE_ATTR_TUNNEL,
> @@ -20,6 +24,10 @@ enum {
> PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
> PSAMPLE_ATTR_PROTO, /* u16 */
> PSAMPLE_ATTR_USER_COOKIE, /* binary, user provided data */
> + PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
> + * PSAMPLE_ATTR_SAMPLE_RATE as a
> + * probability scaled 0 - U32_MAX.
> + */
>
> __PSAMPLE_ATTR_MAX
> };
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index 1c76f3e48dcd..f48b5b9cd409 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -497,6 +497,9 @@ void psample_sample_packet(struct psample_group *group,
> struct sk_buff *skb,
> md->user_cookie))
> goto error;
>
> + if (md->rate_as_probability)
> + nla_put_flag(skb, PSAMPLE_ATTR_SAMPLE_PROBABILITY);
> +
> genlmsg_end(nl_skb, data);
> genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
> PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 03/10] net: psample: skip packet copy if no listeners
Adrian Moreno writes: > If nobody is listening on the multicast group, generating the sample, > which involves copying packet data, seems completely unnecessary. > > Return fast in this case. > > Acked-by: Eelco Chaudron > Reviewed-by: Ido Schimmel > Reviewed-by: Simon Horman > Signed-off-by: Adrian Moreno > --- Reviewed-by: Aaron Conole > net/psample/psample.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/net/psample/psample.c b/net/psample/psample.c > index b37488f426bc..1c76f3e48dcd 100644 > --- a/net/psample/psample.c > +++ b/net/psample/psample.c > @@ -376,6 +376,10 @@ void psample_sample_packet(struct psample_group *group, > struct sk_buff *skb, > void *data; > int ret; > > + if (!genl_has_listeners(&psample_nl_family, group->net, > + PSAMPLE_NL_MCGRP_SAMPLE)) > + return; > + > meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) + > (out_ifindex ? nla_total_size(sizeof(u16)) : 0) + > (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) + ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 02/10] net: sched: act_sample: add action cookie to sample
Adrian Moreno writes:
> If the action has a user_cookie, pass it along to the sample so it can
> be easily identified.
>
> Acked-by: Eelco Chaudron
> Reviewed-by: Ido Schimmel
> Signed-off-by: Adrian Moreno
> ---
Reviewed-by: Aaron Conole
> net/sched/act_sample.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index a69b53d54039..2ceb4d141b71 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
> {
> struct tcf_sample *s = to_sample(a);
> struct psample_group *psample_group;
> + u8 cookie_data[TC_COOKIE_MAX_SIZE];
> struct psample_metadata md = {};
> + struct tc_cookie *user_cookie;
> int retval;
>
> tcf_lastuse_update(&s->tcf_tm);
> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
> if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
> skb_push(skb, skb->mac_len);
>
> + rcu_read_lock();
> + user_cookie = rcu_dereference(a->user_cookie);
> + if (user_cookie) {
> + memcpy(cookie_data, user_cookie->data,
> +user_cookie->len);
> + md.user_cookie = cookie_data;
> + md.user_cookie_len = user_cookie->len;
> + }
> + rcu_read_unlock();
> +
> md.trunc_size = s->truncate ? s->trunc_size : skb->len;
> psample_sample_packet(psample_group, skb, s->rate, &md);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v7 01/10] net: psample: add user cookie
Adrian Moreno writes:
> Add a user cookie to the sample metadata so that sample emitters can
> provide more contextual information to samples.
>
> If present, send the user cookie in a new attribute:
> PSAMPLE_ATTR_USER_COOKIE.
>
> Acked-by: Eelco Chaudron
> Reviewed-by: Simon Horman
> Reviewed-by: Ido Schimmel
> Signed-off-by: Adrian Moreno
> ---
Reviewed-by: Aaron Conole
> include/net/psample.h| 2 ++
> include/uapi/linux/psample.h | 1 +
> net/psample/psample.c| 9 -
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 0509d2d6be67..2ac71260a546 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -25,6 +25,8 @@ struct psample_metadata {
> out_tc_occ_valid:1,
> latency_valid:1,
> unused:5;
> + const u8 *user_cookie;
> + u32 user_cookie_len;
> };
>
> struct psample_group *psample_group_get(struct net *net, u32 group_num);
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e585db5bf2d2..e80637e1d97b 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -19,6 +19,7 @@ enum {
> PSAMPLE_ATTR_LATENCY, /* u64, nanoseconds */
> PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
> PSAMPLE_ATTR_PROTO, /* u16 */
> + PSAMPLE_ATTR_USER_COOKIE, /* binary, user provided data */
>
> __PSAMPLE_ATTR_MAX
> };
> diff --git a/net/psample/psample.c b/net/psample/psample.c
> index a5d9b8446f77..b37488f426bc 100644
> --- a/net/psample/psample.c
> +++ b/net/psample/psample.c
> @@ -386,7 +386,9 @@ void psample_sample_packet(struct psample_group *group,
> struct sk_buff *skb,
> nla_total_size(sizeof(u32)) +/* group_num */
> nla_total_size(sizeof(u32)) +/* seq */
> nla_total_size_64bit(sizeof(u64)) + /* timestamp */
> -nla_total_size(sizeof(u16)); /* protocol */
> +nla_total_size(sizeof(u16)) +/* protocol */
> +(md->user_cookie_len ?
> + nla_total_size(md->user_cookie_len) : 0); /* user cookie */
>
> #ifdef CONFIG_INET
> tun_info = skb_tunnel_info(skb);
> @@ -486,6 +488,11 @@ void psample_sample_packet(struct psample_group *group,
> struct sk_buff *skb,
> }
> #endif
>
> + if (md->user_cookie && md->user_cookie_len &&
> + nla_put(nl_skb, PSAMPLE_ATTR_USER_COOKIE, md->user_cookie_len,
> + md->user_cookie))
> + goto error;
> +
> genlmsg_end(nl_skb, data);
> genlmsg_multicast_netns(&psample_nl_family, group->net, nl_skb, 0,
> PSAMPLE_NL_MCGRP_SAMPLE, GFP_ATOMIC);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v6 05/10] net: openvswitch: add psample action
Adrian Moreno writes:
> Add support for a new action: psample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.
>
> Signed-off-by: Adrian Moreno
> ---
I didn't thoroughly review this, but just wanted to comment that I like
the idea of a psample action here specific to the actual action that is
being performed on the packet - psample. Much like we do for userspace
and other actions.
> Documentation/netlink/specs/ovs_flow.yaml | 17
> include/uapi/linux/openvswitch.h | 28 ++
> net/openvswitch/Kconfig | 1 +
> net/openvswitch/actions.c | 47 +++
> net/openvswitch/flow_netlink.c| 32 ++-
> 5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/netlink/specs/ovs_flow.yaml
> b/Documentation/netlink/specs/ovs_flow.yaml
> index 4fdfc6b5cae9..46f5d1cd8a5f 100644
> --- a/Documentation/netlink/specs/ovs_flow.yaml
> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> @@ -727,6 +727,12 @@ attribute-sets:
> name: dec-ttl
> type: nest
> nested-attributes: dec-ttl-attrs
> + -
> +name: psample
> +type: nest
> +nested-attributes: psample-attrs
> +doc: |
> + Sends a packet sample to psample for external observation.
>-
> name: tunnel-key-attrs
> enum-name: ovs-tunnel-key-attr
> @@ -938,6 +944,17 @@ attribute-sets:
>-
> name: gbp
> type: u32
> + -
> +name: psample-attrs
> +enum-name: ovs-psample-attr
> +name-prefix: ovs-psample-attr-
> +attributes:
> + -
> +name: group
> +type: u32
> + -
> +name: cookie
> +type: binary
>
> operations:
>name-prefix: ovs-flow-cmd-
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index efc82c318fa2..07086759556b 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
> };
> #endif
>
> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
> +/**
> + * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
> + * action.
> + *
> + * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> + * sample.
> + * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
> + * contains user-defined metadata. The maximum length is
> + * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
> + *
> + * Sends the packet to the psample multicast group with the specified group
> and
> + * cookie. It is possible to combine this action with the
> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
> + */
> +enum ovs_psample_attr {
> + OVS_PSAMPLE_ATTR_GROUP = 1, /* u32 number. */
> + OVS_PSAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
> +
> + /* private: */
> + __OVS_PSAMPLE_ATTR_MAX
> +};
> +
> +#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> + * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external
> observers
> + * via psample.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> OVS_ACTION_ATTR_DROP, /* u32 error code. */
> + OVS_ACTION_ATTR_PSAMPLE, /* Nested OVS_PSAMPLE_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> * from userspace. */
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 29a7081858cd..2535f3f9f462 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -10,6 +10,7 @@ config OPENVSWITCH
> (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>(!NF_NAT || NF_NAT) && \
>(!NETFILTER_CONNCOUNT ||
> NETFILTER_CONNCOUNT)))
> + depends on PSAMPLE || !PSAMPLE
> select LIBCRC32C
> select MPLS
> select NET_MPLS_GSO
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 964225580824..a035b7e677dd 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -24,6 +24,11 @@
> #include
Re: [ovs-dev] [PATCH net-next v3 0/7] selftests: net: Switch pmtu.sh to use the internal ovs script.
Jakub Kicinski writes: > On Tue, 25 Jun 2024 13:22:38 -0400 Aaron Conole wrote: >> Currently, if a user wants to run pmtu.sh and cover all the provided test >> cases, they need to install the Open vSwitch userspace utilities. This >> dependency is difficult for users as well as CI environments, because the >> userspace build and setup may require lots of support and devel packages >> to be installed, system setup to be correct, and things like permissions >> and selinux policies to be properly configured. > > Hi Aaron! > > I merged this yesterday (with slight alphabetical reshuffling of > the config options). The pmtu.sh test is solid now, which is great! :) Thanks! That's great to see. > I also added the OvS tests themselves, and those are not passing, yet: > https://netdev.bots.linux.dev/contest.html?test=openvswitch-sh > Could you take a look and LMK if these are likely env issues or > something bad in the test itself? I saw that. I was looking for a place in the nipa repository where I could submit a small fix, because I noticed in the stdout: make -C tools/testing/selftests TARGETS="net/openvswitch" TEST_PROGS=openvvswitch.sh TEST_GEN_PROGS="" run_tests and I think the TEST_PROGS=openvvswitch.sh is misspelled (but it seems to not matter too much for the run_test target). >From what I understand, there are two things causing it to be flaky. First, the module detection is a bit flaky (and that's why it results is some 'skip' reports). Additionally, the connection oriented tests include negative cases and those hit timeouts. The default is to declare failure after 45s. That can be seen in: https://netdev-3.bots.linux.dev/vmksft-net/results/659601/91-openvswitch-sh/stdout ... # timeout set to 45 ... # TEST: nat_connect_v4[START] # Terminated # Terminated This is showing that the timeout is too short. I have patches ready for these issues, but I didn't know if you would like me to submit config and settings files to go under net/openvswitch, or if you would prefer to see the openvswitch.sh script, and ovs-dpctl.py utilities move out of their net/openvswitch/ directory. If the latter, I can submit patches quickly with config and settings (and a small change to the script itself) that addresses these. If you'd prefer the former (moving around the files), I'll need to spend some additional time modifying pmtu and doing a larger test. I don't have a strong opinion on either approach. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
