Re: [dpdk-dev] [PATCH v2] net/bnxt: add flow stats in extended stats

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/17/2020 3:49 PM, Ajit Khaparde wrote:
> On Thu, Apr 16, 2020 at 10:49 PM Ajit Khaparde <ajit.khaparde@broadcom.com>
> wrote:
> 
>> From: Somnath Kotur <somnath.kotur@broadcom.com>
>>
>> This patch allows to display flow stats in extended stats.
>> To do this, DMA-able memory is registered with the FW during device
>> initialization. Then the driver uses an alarm thread to query the
>> per flow stats using the HWRM_CFA_COUNTER_QSTATS HWRM command at
>> regular intervals and stores it locally which will be displayed
>> when the application queries the xstats.
>> The DMA-able memory is unregistered during driver cleanup.
>> This functionality can be enabled using the flow-xstat devarg and
>> will be disabled by default. The intention behind this is to allow
>> stats to be displayed for all the flows in one shot instead of
>> querying one at a time.
>>
>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Reviewed-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
>>
> Patch applied to dpdk-next-net-brcm.
> 

<...>

>> +static int bnxt_register_fc_ctx_mem(struct bnxt *bp)
>> +{
>> +       int rc = 0;
>> +
>> +       rc = bnxt_hwrm_ctx_rgtr(bp, bp->rx_fc_in_tbl.dma,
>> +                               &bp->rx_fc_in_tbl.ctx_id);
>> +       if (rc)
>> +               return rc;
>> +
>> +       PMD_DRV_LOG(DEBUG,
>> +                   "rx_fc_in_tbl.va = %p rx_fc_in_tbl.dma = %p"
>> +                   " rx_fc_in_tbl.ctx_id = %d\n",
>> +                   bp->rx_fc_in_tbl.va, (void *)bp->rx_fc_in_tbl.dma,
>> +                   bp->rx_fc_in_tbl.ctx_id);
>> +
>> +       rc = bnxt_hwrm_ctx_rgtr(bp, bp->rx_fc_out_tbl.dma,
>> +                               &bp->rx_fc_out_tbl.ctx_id);
>> +       if (rc)
>> +               return rc;
>> +
>> +       PMD_DRV_LOG(DEBUG,
>> +                   "rx_fc_out_tbl.va = %p rx_fc_out_tbl.dma = %p"
>> +                   " rx_fc_out_tbl.ctx_id = %d\n",
>> +                   bp->rx_fc_out_tbl.va, (void *)bp->rx_fc_out_tbl.dma,

This fails on 32-bit, as it does in previous set, please check 32-bit build
before submitting.

build error [1], same for below logs. Fixing while merging as following:
 -                   bp->rx_fc_in_tbl.va, (void *)bp->rx_fc_in_tbl.dma,
 +                   bp->rx_fc_in_tbl.va,
 +                   (void *)((uintptr_t)bp->rx_fc_in_tbl.dma),


[1]
...dpdk/drivers/net/bnxt/bnxt_ethdev.c:356:28: error: cast to pointer from
integer of different size [-Werror=int-to-pointer-cast]
  356 |       bp->rx_fc_in_tbl.va, (void *)bp->rx_fc_in_tbl.dma,
      |                            ^
...dpdk/drivers/net/bnxt/bnxt.h:747:16: note: in definition of macro
‘PMD_DRV_LOG_RAW’
  747 |   __func__, ## args)
      |                ^~~~
...dpdk/drivers/net/bnxt/bnxt_ethdev.c:353:2: note: in expansion of macro
‘PMD_DRV_LOG’
  353 |  PMD_DRV_LOG(DEBUG,
      |  ^~~~~~~~~~~

<...>

>> +void bnxt_flow_cnt_alarm_cb(void *arg)
>> +{
>> +       int rc = 0;
>> +       struct bnxt *bp = arg;
>> +
>> +       if (!bp->rx_fc_out_tbl.va) {
>> +               PMD_DRV_LOG(ERR, "bp->rx_fc_out_tbl.va is NULL?\n");
>> +               bnxt_cancel_fc_thread(bp);
>> +               return;
>> +       }
>> +
>> +       if (!bp->flow_count) {
>> +               bnxt_cancel_fc_thread(bp);
>> +               return;
>> +       }
>> +
>> +       if (!bp->eth_dev->data->dev_started) {
>> +               bnxt_cancel_fc_thread(bp);
>> +               return;
>> +       }
>> +
>> +       rc = bnxt_flow_stats_req(bp);
>> +       if (rc) {
>> +               PMD_DRV_LOG(ERR, "Flow stat alarm not rescheduled.\n");
>> +               return;
>> +       }
>> +
>> +       rte_eal_alarm_set(US_PER_S * BNXT_FC_TIMER,
>> +                         bnxt_flow_cnt_alarm_cb,
>> +                         (void *)bp);
>> +}

Cross build is failing because of 'US_PER_S' [3], it seems header is missing,
adding it while merging [4].

[3]
...dpdk/drivers/net/bnxt/bnxt_flow.c: In function ‘bnxt_setup_flow_counter’:
...dpdk/drivers/net/bnxt/bnxt_flow.c:1636:21: error: ‘US_PER_S’ undeclared
(first use in this function); did you mean ‘US_PER_MS’?
   rte_eal_alarm_set(US_PER_S * BNXT_FC_TIMER,
                     ^~~~~~~~
                     US_PER_MS
...dpdk/drivers/net/bnxt/bnxt_flow.c:1636:21: note: each undeclared identifier
is reported only once for each function it appears in
...dpdk/drivers/net/bnxt/bnxt_flow.c: In function ‘bnxt_flow_cnt_alarm_cb’:
...dpdk/drivers/net/bnxt/bnxt_flow.c:1670:20: error: ‘US_PER_S’ undeclared
(first use in this function); did you mean ‘US_PER_MS’?
  rte_eal_alarm_set(US_PER_S * BNXT_FC_TIMER,
                    ^~~~~~~~
                    US_PER_MS

[4]
 --- a/drivers/net/bnxt/bnxt_flow.c
 +++ b/drivers/net/bnxt/bnxt_flow.c
 @@ -10,6 +10,8 @@
  #include <rte_flow.h>
  #include <rte_flow_driver.h>
  #include <rte_tailq.h>
 +#include <rte_alarm.h>
 +#include <rte_cycles.h>