net/bnxt: fixes in flow counter mgr
diff mbox series

Message ID 20200803141219.5682-1-somnath.kotur@broadcom.com
State Accepted, archived
Delegated to: Ajit Khaparde
Headers show
Series
  • net/bnxt: fixes in flow counter mgr
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Somnath Kotur Aug. 3, 2020, 2:12 p.m. UTC
OVS-DPDK seems to set the reset bit for every flow query. Honor
the bit by resetting the SW counter values after assigning them.
Also set the 'hit' bit only if the counter value retrieved by HW
is non-zero.
While querying flow stats, use max possible entries in the fc table scan
for valid entries instead of active entries as the active entry can be in
any slot in the table.

This is a critical fix for OVS-DPDK flow aging.

Fixes: 306c2d28e247 ("net/bnxt: support count action in flow query")

Reviewed-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

Comments

Thomas Monjalon Aug. 5, 2020, 8:03 p.m. UTC | #1
03/08/2020 16:12, Somnath Kotur:
> OVS-DPDK seems to set the reset bit for every flow query. Honor
> the bit by resetting the SW counter values after assigning them.
> Also set the 'hit' bit only if the counter value retrieved by HW
> is non-zero.
> While querying flow stats, use max possible entries in the fc table scan
> for valid entries instead of active entries as the active entry can be in
> any slot in the table.
> 
> This is a critical fix for OVS-DPDK flow aging.
> 
> Fixes: 306c2d28e247 ("net/bnxt: support count action in flow query")
> 
> Reviewed-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>

Tags must be in chronological order (Reviewed-by after Signed-off-by).

> ---
>  drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)

That's not a small change.

The title does not convey the impact of the change.

Applied anyway because you insisted on the criticality.

Patch
diff mbox series

diff --git a/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c b/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
index febda94..df1921d 100644
--- a/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
+++ b/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
@@ -339,7 +339,7 @@  ulp_fc_mgr_alarm_cb(void *arg)
 	struct bnxt_ulp_fc_info *ulp_fc_info;
 	struct bnxt_ulp_device_params *dparms;
 	struct tf *tfp;
-	uint32_t dev_id, hw_cntr_id = 0;
+	uint32_t dev_id, hw_cntr_id = 0, num_entries = 0;
 
 	ulp_fc_info = bnxt_ulp_cntxt_ptr2_fc_info_get(ctxt);
 	if (!ulp_fc_info)
@@ -384,8 +384,9 @@  ulp_fc_mgr_alarm_cb(void *arg)
 			break;
 	}
 	*/
+	num_entries = dparms->flow_count_db_entries / 2;
 	for (i = 0; i < TF_DIR_MAX; i++) {
-		for (j = 0; j < ulp_fc_info->num_entries; j++) {
+		for (j = 0; j < num_entries; j++) {
 			if (!ulp_fc_info->sw_acc_tbl[i][j].valid)
 				continue;
 			hw_cntr_id = ulp_fc_info->sw_acc_tbl[i][j].hw_cntr_id;
@@ -551,7 +552,7 @@  int ulp_fc_mgr_query_count_get(struct bnxt_ulp_context *ctxt,
 	struct ulp_flow_db_res_params params;
 	enum tf_dir dir;
 	uint32_t hw_cntr_id = 0, sw_cntr_idx = 0;
-	struct sw_acc_counter sw_acc_tbl_entry;
+	struct sw_acc_counter *sw_acc_tbl_entry;
 	bool found_cntr_resource = false;
 
 	ulp_fc_info = bnxt_ulp_cntxt_ptr2_fc_info_get(ctxt);
@@ -584,13 +585,21 @@  int ulp_fc_mgr_query_count_get(struct bnxt_ulp_context *ctxt,
 		hw_cntr_id = params.resource_hndl;
 		sw_cntr_idx = hw_cntr_id -
 				ulp_fc_info->shadow_hw_tbl[dir].start_idx;
-		sw_acc_tbl_entry = ulp_fc_info->sw_acc_tbl[dir][sw_cntr_idx];
+		sw_acc_tbl_entry = &ulp_fc_info->sw_acc_tbl[dir][sw_cntr_idx];
 		if (params.resource_sub_type ==
 			BNXT_ULP_RESOURCE_SUB_TYPE_INDEX_TYPE_INT_COUNT) {
-			count->hits_set = 1;
-			count->bytes_set = 1;
-			count->hits = sw_acc_tbl_entry.pkt_count;
-			count->bytes = sw_acc_tbl_entry.byte_count;
+			pthread_mutex_lock(&ulp_fc_info->fc_lock);
+			if (sw_acc_tbl_entry->pkt_count) {
+				count->hits_set = 1;
+				count->bytes_set = 1;
+				count->hits = sw_acc_tbl_entry->pkt_count;
+				count->bytes = sw_acc_tbl_entry->byte_count;
+			}
+			if (count->reset) {
+				sw_acc_tbl_entry->pkt_count = 0;
+				sw_acc_tbl_entry->byte_count = 0;
+			}
+			pthread_mutex_unlock(&ulp_fc_info->fc_lock);
 		} else {
 			/* TBD: Handle External counters */
 			rc = -EINVAL;