When a flow gets modified, i.e. the actions are changes, the tc layer will
remove, and re-add the flow. This is causing all the counters to be reset.
This patch will remember the previous tc counters and adjust any requests
for statistics. This is done in a similar way as the rte_flow implementation.
It also updates the check_pkt_len tc test to purge the flows, so we do
not use existing updated tc flow counters, but start with fresh installed
set of datapath flows.
Signed-off-by: Eelco Chaudron <echau...@redhat.com>
---
Please note that for now two copies of the test case exists, but I will clean
this up once this gets applied by submitting a new revision of the
'tests: Add system-traffic.at tests to check-offloads' series.
-v2: Do not update the stats->used, as in terse dump they should be 0.
-v3: Added some comments based on the v2 review.
-v4: Re-based on latest upstream master.
Updated commit message.
Updated system-traffic test name.
Fixed byte counter usage in check_pkt_lent test cases.
Synced both new tests to use the same byte counter approach.
lib/netdev-offload-tc.c | 99 ++++++++++++++++++++++++++++++++------
lib/tc.h | 1
tests/system-offloads-traffic.at | 78 +++++++++++++++++++++++++++---
tests/system-traffic.at | 64 +++++++++++++++++++++++++
4 files changed, 219 insertions(+), 23 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 6e1bbaa28..134c24157 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -97,6 +97,12 @@ static int netdev_tc_parse_nl_actions(struct netdev *netdev,
bool *recirc_act, bool more_actions,
struct tc_action **need_jump_update);
+static void parse_tc_flower_to_stats(struct tc_flower *flower,
+ struct dpif_flow_stats *stats);
+
+static int get_ufid_adjust_stats(const ovs_u128 *ufid,
+ struct dpif_flow_stats *stats);
+
static bool
is_internal_port(const char *type)
{
@@ -193,6 +199,9 @@ static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER;
* @ufid: ufid assigned to the flow
* @id: tc filter id (tcf_id)
* @netdev: netdev associated with the tc rule
+ * @adjust_stats: When flow gets updated with new actions, we need to adjust
+ * the reported stats to include previous values as the hardware
+ * rule is removed and re-added. This stats copy is used for it.
*/
struct ufid_tc_data {
struct hmap_node ufid_to_tc_node;
@@ -200,6 +209,7 @@ struct ufid_tc_data {
ovs_u128 ufid;
struct tcf_id id;
struct netdev *netdev;
+ struct dpif_flow_stats adjust_stats;
};
static void
@@ -233,12 +243,38 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
ovs_mutex_unlock(&ufid_lock);
}
+static void
+netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
+ const struct dpif_flow_stats *adjust_stats)
+{
+ /* Do not try to restore the stats->used, as in terse mode dumps TC doesn't
+ * report TCA_ACT_OPTIONS, so the 'lastused' value is not available, hence
+ * we report used as 0.
+ * tcp_flags is not collected by tc, so no need to update it. */
+ stats->n_bytes += adjust_stats->n_bytes;
+ stats->n_packets += adjust_stats->n_packets;
+}
+
/* Wrapper function to delete filter and ufid tc mapping */
static int
-del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
+del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid,
+ struct dpif_flow_stats *stats)
{
+ struct tc_flower flower;
int err;
+ if (stats) {
+ memset(stats, 0, sizeof *stats);
+ if (!tc_get_flower(id, &flower)) {
+ struct dpif_flow_stats adjust_stats;
+
+ parse_tc_flower_to_stats(&flower, stats);
+ if (!get_ufid_adjust_stats(ufid, &adjust_stats)) {
+ netdev_tc_adjust_stats(stats, &adjust_stats);
+ }
+ }
+ }
+
err = tc_del_flower_filter(id);
if (!err) {
del_ufid_tc_mapping(ufid);
@@ -249,7 +285,7 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const
ovs_u128 *ufid)
/* Add ufid entry to ufid_to_tc hashmap. */
static void
add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid,
- struct tcf_id *id)
+ struct tcf_id *id, struct dpif_flow_stats *stats)
{
struct ufid_tc_data *new_data = xzalloc(sizeof *new_data);
size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
@@ -261,6 +297,9 @@ add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128
*ufid,
new_data->ufid = *ufid;
new_data->id = *id;
new_data->netdev = netdev_ref(netdev);
+ if (stats) {
+ new_data->adjust_stats = *stats;
+ }
ovs_mutex_lock(&ufid_lock);
hmap_insert(&ufid_to_tc, &new_data->ufid_to_tc_node, ufid_hash);
@@ -292,6 +331,30 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id
*id)
return ENOENT;
}
+/* Get adjust_stats from ufid_to_tc hashmap.
+ *
+ * Returns 0 if successful and fills stats with adjust_stats.
+ * Otherwise returns the error.
+*/
+static int
+get_ufid_adjust_stats(const ovs_u128 *ufid, struct dpif_flow_stats *stats)
+{
+ size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
+ struct ufid_tc_data *data;
+
+ ovs_mutex_lock(&ufid_lock);
+ HMAP_FOR_EACH_WITH_HASH (data, ufid_to_tc_node, ufid_hash, &ufid_to_tc) {
+ if (ovs_u128_equals(*ufid, data->ufid)) {
+ *stats = data->adjust_stats;
+ ovs_mutex_unlock(&ufid_lock);
+ return 0;
+ }
+ }
+ ovs_mutex_unlock(&ufid_lock);
+
+ return ENOENT;
+}
+
/* Find ufid entry in ufid_to_tc hashmap using tcf_id id.
* The result is saved in ufid.
*
@@ -1193,6 +1256,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
get_tc_qdisc_hook(netdev));
while (nl_dump_next(dump->nl_dump, &nl_flow, rbuffer)) {
+ struct dpif_flow_stats adjust_stats;
struct tc_flower flower;
if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower, dump->terse)) {
@@ -1210,6 +1274,10 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
continue;
}
+ if (!get_ufid_adjust_stats(ufid, &adjust_stats)) {
+ netdev_tc_adjust_stats(stats, &adjust_stats);
+ }
+
match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
match->flow.in_port.odp_port = dump->port;
match_set_recirc_id(match, id.chain);
@@ -2059,6 +2127,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
struct flow *mask = &match->wc.masks;
const struct flow_tnl *tnl = &match->flow.tunnel;
struct flow_tnl *tnl_mask = &mask->tunnel;
+ struct dpif_flow_stats adjust_stats;
bool recirc_act = false;
uint32_t block_id = 0;
struct tcf_id id;
@@ -2352,10 +2421,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
return EOPNOTSUPP;
}
+ memset(&adjust_stats, 0, sizeof adjust_stats);
if (get_ufid_tc_mapping(ufid, &id) == 0) {
VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
id.handle, id.prio);
- info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(&id, ufid);
+ info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(
+ &id, ufid, &adjust_stats);
}
prio = get_prio_for_tc_flower(&flower);
@@ -2373,8 +2444,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
*match,
if (!err) {
if (stats) {
memset(stats, 0, sizeof *stats);
+ netdev_tc_adjust_stats(stats, &adjust_stats);
}
- add_ufid_tc_mapping(netdev, ufid, &id);
+ add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats);
}
return err;
@@ -2415,6 +2487,13 @@ netdev_tc_flow_get(struct netdev *netdev,
parse_tc_flower_to_match(netdev, &flower, match, actions,
stats, attrs, buf, false);
+ if (stats) {
+ struct dpif_flow_stats adjust_stats;
+
+ if (!get_ufid_adjust_stats(ufid, &adjust_stats)) {
+ netdev_tc_adjust_stats(stats, &adjust_stats);
+ }
+ }
match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
match->flow.in_port.odp_port = in_port;
match_set_recirc_id(match, id.chain);
@@ -2427,7 +2506,6 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
const ovs_u128 *ufid,
struct dpif_flow_stats *stats)
{
- struct tc_flower flower;
struct tcf_id id;
int error;
@@ -2436,16 +2514,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
return error;
}
- if (stats) {
- memset(stats, 0, sizeof *stats);
- if (!tc_get_flower(&id, &flower)) {
- parse_tc_flower_to_stats(&flower, stats);
- }
- }
-
- error = del_filter_and_ufid_mapping(&id, ufid);
-
- return error;
+ return del_filter_and_ufid_mapping(&id, ufid, stats);
}
static int
diff --git a/lib/tc.h b/lib/tc.h
index ea4ce806b..cdd3b4f60 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -343,7 +343,6 @@ static inline bool
is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2)
{
return id1->prio == id2->prio
- && id1->handle == id2->handle
&& id1->handle == id2->handle
&& id1->hook == id2->hook
&& id1->block_id == id2->block_id
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 16a4c1a00..123bbde5b 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -397,7 +397,7 @@ AT_CHECK([cat p4.pcap | awk 'NF{print $NF}' | uniq -c | awk
'{$1=$1;print}'], [0
# This test verifies the total packet counters work when individual branches
# are taken.
-AT_CHECK([ovs-appctl revalidator/wait], [0])
+AT_CHECK([ovs-appctl revalidator/purge], [0])
AT_CHECK([ovs-ofctl del-flows br0])
AT_DATA([flows.txt], [dnl
table=0,in_port=2 actions=output:1
@@ -417,9 +417,9 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 1024
10.1.1.2 | FORMAT_PIN
10 packets transmitted, 10 received, 0% packet loss, time 0ms
], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0])
-AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" |
DUMP_CLEAN_SORTED | sed 's/bytes:11440/bytes:11720/'], [0], [dnl
-in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), packets:20, bytes:11720,
used:0.001s, actions:check_pkt_len(size=200,gt(3),le(3))
-in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:20, bytes:11720,
used:0.001s, actions:output
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" |
DUMP_CLEAN_SORTED | sed 's/bytes:11348/bytes:11614/'], [0], [dnl
+in_port(2),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614,
used:0.001s, actions:check_pkt_len(size=200,gt(3),le(3))
+in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614,
used:0.001s, actions:output
])
@@ -492,7 +492,7 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 1024
10.1.1.2 | FORMAT_PIN
OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(3,5),le(3,4))])
-AT_CHECK([ovs-appctl revalidator/wait], [0])
+AT_CHECK([ovs-appctl revalidator/purge], [0])
AT_CHECK([ovs-ofctl del-flows br0])
AT_DATA([flows.txt], [dnl
table=0,in_port=2 actions=output:1
@@ -517,9 +517,9 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 10 -i 0.1 -w 2 -s 1024
10.1.1.2 | FORMAT_PIN
10 packets transmitted, 10 received, 0% packet loss, time 0ms
], [], [ovs-appctl dpctl/dump-flows; ovs-ofctl dump-flows br0])
-AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
"eth_type(0x0800)" | DUMP_CLEAN_SORTED | sed -e 's/bytes:11348/bytes:11614/' -e
's/bytes:11440/bytes:11720/'], [0], [dnl
+AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
"eth_type(0x0800)" | DUMP_CLEAN_SORTED | sed -e 's/bytes:11348/bytes:11614/'],
[0], [dnl
in_port(2),eth(),eth_type(0x0800),ipv4(proto=1,tos=0/0xfc,frag=no),
packets:19, bytes:11614, used:0.001s,
actions:check_pkt_len(size=200,gt(set(ipv4(tos=0x4/0xfc)),4),le(set(ipv4(tos=0x8/0xfc)),5)),3
-in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:20, bytes:11720,
used:0.001s, actions:output
+in_port(3),eth(),eth_type(0x0800),ipv4(frag=no), packets:19, bytes:11614,
used:0.001s, actions:output
])
sleep 1
@@ -680,3 +680,67 @@
OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(5),le(check_pkt_len(size=100,gt(5),
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+
+
+AT_SETUP([offloads - simulated flow action update])
+OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch .
other_config:hw-offload=true])
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+AT_DATA([flows.txt], [dnl
+add in_port=ovs-p0,actions=ovs-p1,br0
+add in_port=ovs-p1,actions=ovs-p0,br0
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:756/bytes:882/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.0s, actions:3,1
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.0s, actions:2,1
+])
+
+AT_DATA([flows2.txt], [dnl
+modify in_port=ovs-p0,actions=ovs-p1
+modify in_port=ovs-p1,actions=ovs-p0
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows2.txt])
+AT_CHECK([ovs-appctl revalidator/wait], [0])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed -e
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:1596/bytes:1862/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:19,
bytes:1862, used:0.0s, actions:3
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:19,
bytes:1862, used:0.0s, actions:2
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl revalidator/wait], [0])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:2436/bytes:2842/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:29,
bytes:2842, used:0.0s, actions:3,1
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:29,
bytes:2842, used:0.0s, actions:2,1
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 6d8651a44..470c3f640 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7592,3 +7592,67 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000
*0000 *5002 *2000 *b85e *00
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+
+
+AT_SETUP([simulated flow action update])
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+AT_DATA([flows.txt], [dnl
+add in_port=ovs-p0,actions=ovs-p1,br0
+add in_port=ovs-p1,actions=ovs-p0,br0
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:756/bytes:882/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.0s, actions:3,1
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:9,
bytes:882, used:0.0s, actions:2,1
+])
+
+AT_DATA([flows2.txt], [dnl
+modify in_port=ovs-p0,actions=ovs-p1
+modify in_port=ovs-p1,actions=ovs-p0
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows2.txt])
+AT_CHECK([ovs-appctl revalidator/wait], [0])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed -e
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:1596/bytes:1862/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:19,
bytes:1862, used:0.0s, actions:3
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:19,
bytes:1862, used:0.0s, actions:2
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl revalidator/wait], [0])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING],
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | sort | dnl
+ strip_recirc | strip_used | dnl
+ sed
's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;s/bytes:2436/bytes:2842/'],
dnl
+ [0], [dnl
+recirc_id(<recirc>),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:29,
bytes:2842, used:0.0s, actions:3,1
+recirc_id(<recirc>),in_port(3),eth_type(0x0800),ipv4(frag=no), packets:29,
bytes:2842, used:0.0s, actions:2,1
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
