[v4,10/20] net/bnxt: remove devargs for stats accumulation
Checks
Commit Message
From: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
The accumulation of flow counters is not determined by the
application device arguments anymore. Instead it is now dictated by
the platform capabilities whether to do software based accumulation or not.
Signed-off-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Reviewed-by: Mike Baucom <michael.baucom@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
drivers/net/bnxt/bnxt.h | 3 --
drivers/net/bnxt/bnxt_ethdev.c | 53 ----------------------------
drivers/net/bnxt/tf_ulp/bnxt_ulp.c | 8 -----
drivers/net/bnxt/tf_ulp/bnxt_ulp.h | 1 -
drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c | 12 +++----
5 files changed, 4 insertions(+), 73 deletions(-)
Comments
On 11/3/2021 12:52 AM, Ajit Khaparde wrote:
> From: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
>
> The accumulation of flow counters is not determined by the
> application device arguments anymore. Instead it is now dictated by
> the platform capabilities whether to do software based accumulation or not.
>
> Signed-off-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Reviewed-by: Mike Baucom <michael.baucom@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
<...>
> @@ -87,7 +87,6 @@ static const struct rte_pci_id bnxt_pci_id_map[] = {
> { .vendor_id = 0, /* sentinel */ },
> };
>
> -#define BNXT_DEVARG_ACCUM_STATS "accum-stats"
> #define BNXT_DEVARG_FLOW_XSTAT "flow-xstat"
> #define BNXT_DEVARG_MAX_NUM_KFLOWS "max-num-kflows"
> #define BNXT_DEVARG_REPRESENTOR "representor"
Hi Ajit,
Not for this patch, but I recognized that runtime devargs for the driver
is not documented at all.
Can you please have a separate patch to document them in bnxt.rst?
On Wed, Nov 3, 2021 at 6:24 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 11/3/2021 12:52 AM, Ajit Khaparde wrote:
> > From: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> >
> > The accumulation of flow counters is not determined by the
> > application device arguments anymore. Instead it is now dictated by
> > the platform capabilities whether to do software based accumulation or not.
> >
> > Signed-off-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Reviewed-by: Mike Baucom <michael.baucom@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> <...>
>
> > @@ -87,7 +87,6 @@ static const struct rte_pci_id bnxt_pci_id_map[] = {
> > { .vendor_id = 0, /* sentinel */ },
> > };
> >
> > -#define BNXT_DEVARG_ACCUM_STATS "accum-stats"
> > #define BNXT_DEVARG_FLOW_XSTAT "flow-xstat"
> > #define BNXT_DEVARG_MAX_NUM_KFLOWS "max-num-kflows"
> > #define BNXT_DEVARG_REPRESENTOR "representor"
>
> Hi Ajit,
>
> Not for this patch, but I recognized that runtime devargs for the driver
> is not documented at all.
>
> Can you please have a separate patch to document them in bnxt.rst?
Hi Freeuh,
Yes. I am working on that.
@@ -718,11 +718,8 @@ struct bnxt {
uint32_t flags2;
#define BNXT_FLAGS2_PTP_TIMESYNC_ENABLED BIT(0)
#define BNXT_FLAGS2_PTP_ALARM_SCHEDULED BIT(1)
-#define BNXT_FLAGS2_ACCUM_STATS_EN BIT(2)
#define BNXT_P5_PTP_TIMESYNC_ENABLED(bp) \
((bp)->flags2 & BNXT_FLAGS2_PTP_TIMESYNC_ENABLED)
-#define BNXT_ACCUM_STATS_EN(bp) \
- ((bp)->flags2 & BNXT_FLAGS2_ACCUM_STATS_EN)
uint16_t chip_num;
#define CHIP_NUM_58818 0xd818
@@ -87,7 +87,6 @@ static const struct rte_pci_id bnxt_pci_id_map[] = {
{ .vendor_id = 0, /* sentinel */ },
};
-#define BNXT_DEVARG_ACCUM_STATS "accum-stats"
#define BNXT_DEVARG_FLOW_XSTAT "flow-xstat"
#define BNXT_DEVARG_MAX_NUM_KFLOWS "max-num-kflows"
#define BNXT_DEVARG_REPRESENTOR "representor"
@@ -101,7 +100,6 @@ static const struct rte_pci_id bnxt_pci_id_map[] = {
static const char *const bnxt_dev_args[] = {
BNXT_DEVARG_REPRESENTOR,
- BNXT_DEVARG_ACCUM_STATS,
BNXT_DEVARG_FLOW_XSTAT,
BNXT_DEVARG_MAX_NUM_KFLOWS,
BNXT_DEVARG_REP_BASED_PF,
@@ -114,12 +112,6 @@ static const char *const bnxt_dev_args[] = {
NULL
};
-/*
- * accum-stats == false to disable flow counter accumulation
- * accum-stats == true to enable flow counter accumulation
- */
-#define BNXT_DEVARG_ACCUM_STATS_INVALID(accum_stats) ((accum_stats) > 1)
-
/*
* app-id = an non-negative 8-bit number
*/
@@ -5290,45 +5282,6 @@ static int bnxt_init_resources(struct bnxt *bp, bool reconfig_dev)
return 0;
}
-static int
-bnxt_parse_devarg_accum_stats(__rte_unused const char *key,
- const char *value, void *opaque_arg)
-{
- struct bnxt *bp = opaque_arg;
- unsigned long accum_stats;
- char *end = NULL;
-
- if (!value || !opaque_arg) {
- PMD_DRV_LOG(ERR,
- "Invalid parameter passed to accum-stats devargs.\n");
- return -EINVAL;
- }
-
- accum_stats = strtoul(value, &end, 10);
- if (end == NULL || *end != '\0' ||
- (accum_stats == ULONG_MAX && errno == ERANGE)) {
- PMD_DRV_LOG(ERR,
- "Invalid parameter passed to accum-stats devargs.\n");
- return -EINVAL;
- }
-
- if (BNXT_DEVARG_ACCUM_STATS_INVALID(accum_stats)) {
- PMD_DRV_LOG(ERR,
- "Invalid value passed to accum-stats devargs.\n");
- return -EINVAL;
- }
-
- if (accum_stats) {
- bp->flags2 |= BNXT_FLAGS2_ACCUM_STATS_EN;
- PMD_DRV_LOG(INFO, "Host-based accum-stats feature enabled.\n");
- } else {
- bp->flags2 &= ~BNXT_FLAGS2_ACCUM_STATS_EN;
- PMD_DRV_LOG(INFO, "Host-based accum-stats feature disabled.\n");
- }
-
- return 0;
-}
-
static int
bnxt_parse_devarg_flow_xstat(__rte_unused const char *key,
const char *value, void *opaque_arg)
@@ -5681,12 +5634,6 @@ bnxt_parse_dev_args(struct bnxt *bp, struct rte_devargs *devargs)
if (ret)
goto err;
- /*
- * Handler for "accum-stats" devarg.
- * Invoked as for ex: "-a 0000:00:0d.0,accum-stats=1"
- */
- rte_kvargs_process(kvlist, BNXT_DEVARG_ACCUM_STATS,
- bnxt_parse_devarg_accum_stats, bp);
/*
* Handler for "max_num_kflows" devarg.
* Invoked as for ex: "-a 000:00:0d.0,max_num_kflows=32"
@@ -1490,14 +1490,6 @@ bnxt_ulp_port_init(struct bnxt *bp)
goto jump_to_error;
}
- /* set the accumulation of the stats */
- if (BNXT_ACCUM_STATS_EN(bp))
- bp->ulp_ctx->cfg_data->accum_stats = true;
-
- BNXT_TF_DBG(DEBUG, "BNXT Port:%d ULP port init, accum_stats:%d\n",
- bp->eth_dev->data->port_id,
- bp->ulp_ctx->cfg_data->accum_stats);
-
/* set the unicast mode */
if (bnxt_ulp_cntxt_ptr2_ulp_flags_get(bp->ulp_ctx, &ulp_flags)) {
BNXT_TF_DBG(ERR, "Error in getting ULP context flags\n");
@@ -92,7 +92,6 @@ struct bnxt_ulp_data {
#define BNXT_ULP_TUN_ENTRY_INVALID -1
#define BNXT_ULP_MAX_TUN_CACHE_ENTRIES 16
struct bnxt_tun_cache_entry tun_tbl[BNXT_ULP_MAX_TUN_CACHE_ENTRIES];
- bool accum_stats;
uint8_t app_id;
uint8_t num_shared_clients;
struct bnxt_flow_app_tun_ent app_tun[BNXT_ULP_MAX_TUN_CACHE_ENTRIES];
@@ -396,21 +396,17 @@ static int ulp_get_single_flow_stat(struct bnxt_ulp_context *ctxt,
return rc;
}
- /* TBD - Get PKT/BYTE COUNT SHIFT/MASK from Template */
+ /* PKT/BYTE COUNT SHIFT/MASK are device specific */
sw_cntr_indx = hw_cntr_id - fc_info->shadow_hw_tbl[dir].start_idx;
sw_acc_tbl_entry = &fc_info->sw_acc_tbl[dir][sw_cntr_indx];
+
/* Some dpdk applications may accumulate the flow counters while some
* may not. In cases where the application is accumulating the counters
* the PMD need not do the accumulation itself and viceversa to report
* the correct flow counters.
*/
- if (ctxt->cfg_data->accum_stats) {
- sw_acc_tbl_entry->pkt_count += FLOW_CNTR_PKTS(stats, dparms);
- sw_acc_tbl_entry->byte_count += FLOW_CNTR_BYTES(stats, dparms);
- } else {
- sw_acc_tbl_entry->pkt_count = FLOW_CNTR_PKTS(stats, dparms);
- sw_acc_tbl_entry->byte_count = FLOW_CNTR_BYTES(stats, dparms);
- }
+ sw_acc_tbl_entry->pkt_count += FLOW_CNTR_PKTS(stats, dparms);
+ sw_acc_tbl_entry->byte_count += FLOW_CNTR_BYTES(stats, dparms);
/* Update the parent counters if it is child flow */
if (sw_acc_tbl_entry->pc_flow_idx & FLOW_CNTR_PC_FLOW_VALID) {