Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Preserve tc statistics when flow gets modified.
On 12 Jan 2023, at 18:29, Michael Santana wrote:
> On 12/15/22 09:52, Eelco Chaudron wrote:
>> 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 updated tc flows, with their counters.
>>
>> Signed-off-by: Eelco Chaudron
>> ---
>> 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.
>>
>> lib/netdev-offload-tc.c | 92
>> --
>> lib/tc.h |1
>> tests/system-offloads-traffic.at | 65 +--
>> tests/system-traffic.at | 64 ++
>> 4 files changed, 201 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index ce7f8ad97..03c25fc38 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,36 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>> ovs_mutex_unlock(_lock);
>> }
>> +static void
>> +netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
>> + struct dpif_flow_stats *adjust_stats)
>> +{
>> +/* Do not try to restore the stats->used, as in terse
>> + * mode dumps we will always report them as 0. */
>> +stats->n_bytes += adjust_stats->n_bytes;
>> +stats->n_packets += adjust_stats->n_packets;
> Why not also update the other members of dpif_flow_stats?
tcp_flags is not used by TC, so no need to use it. Will update the comment
above.
>> +}
>> +
>> /* 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, )) {
>> +struct dpif_flow_stats adjust_stats;
>> +
>> +parse_tc_flower_to_stats(, stats);
>> +if (!get_ufid_adjust_stats(ufid, _stats)) {
>> +netdev_tc_adjust_stats(stats, _stats);
>> +}
>> +}
>> +}
>> +
>> err = tc_del_filter(id);
>> if (!err) {
>> del_ufid_tc_mapping(ufid);
>> @@ -249,7 +283,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 +295,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;
>> +}
> lgtm
>>ovs_mutex_lock(_lock);
>>
Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Preserve tc statistics when flow gets modified.
On 12/15/22 09:52, Eelco Chaudron wrote:
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 updated tc flows, with their counters.
Signed-off-by: Eelco Chaudron
---
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.
lib/netdev-offload-tc.c | 92 --
lib/tc.h |1
tests/system-offloads-traffic.at | 65 +--
tests/system-traffic.at | 64 ++
4 files changed, 201 insertions(+), 21 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..03c25fc38 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,36 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
ovs_mutex_unlock(_lock);
}
+static void
+netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
+ struct dpif_flow_stats *adjust_stats)
+{
+/* Do not try to restore the stats->used, as in terse
+ * mode dumps we will always report them as 0. */
+stats->n_bytes += adjust_stats->n_bytes;
+stats->n_packets += adjust_stats->n_packets;
Why not also update the other members of dpif_flow_stats?
+}
+
/* 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, )) {
+struct dpif_flow_stats adjust_stats;
+
+parse_tc_flower_to_stats(, stats);
+if (!get_ufid_adjust_stats(ufid, _stats)) {
+netdev_tc_adjust_stats(stats, _stats);
+}
+}
+}
+
err = tc_del_filter(id);
if (!err) {
del_ufid_tc_mapping(ufid);
@@ -249,7 +283,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 +295,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;
+}
lgtm
ovs_mutex_lock(_lock);
hmap_insert(_to_tc, _data->ufid_to_tc_node, ufid_hash);
@@ -292,6 +329,25 @@ 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);
+
[ovs-dev] [PATCH v2] netdev-offload-tc: Preserve tc statistics when flow gets modified.
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 updated tc flows, with their counters.
Signed-off-by: Eelco Chaudron
---
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.
lib/netdev-offload-tc.c | 92 --
lib/tc.h |1
tests/system-offloads-traffic.at | 65 +--
tests/system-traffic.at | 64 ++
4 files changed, 201 insertions(+), 21 deletions(-)
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..03c25fc38 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,36 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
ovs_mutex_unlock(_lock);
}
+static void
+netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
+ struct dpif_flow_stats *adjust_stats)
+{
+/* Do not try to restore the stats->used, as in terse
+ * mode dumps we will always report them as 0. */
+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, )) {
+struct dpif_flow_stats adjust_stats;
+
+parse_tc_flower_to_stats(, stats);
+if (!get_ufid_adjust_stats(ufid, _stats)) {
+netdev_tc_adjust_stats(stats, _stats);
+}
+}
+}
+
err = tc_del_filter(id);
if (!err) {
del_ufid_tc_mapping(ufid);
@@ -249,7 +283,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 +295,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(_lock);
hmap_insert(_to_tc, _data->ufid_to_tc_node, ufid_hash);
@@ -292,6 +329,25 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id
*id)
return ENOENT;
}
+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(_lock);
+HMAP_FOR_EACH_WITH_HASH (data, ufid_to_tc_node, ufid_hash, _to_tc) {
+if (ovs_u128_equals(*ufid, data->ufid)) {
+*stats = data->adjust_stats;
+ovs_mutex_unlock(_lock);
+return 0;
+}
+}
+
