[dpdk-dev,v2,20/25] bnxt: add support to get and clear VF specific stats
Checks
Commit Message
This patch adds code to get and clear VF stats.
It also adds the necessary HWRM structures to send the command
to the firmware.
Signed-off-by: Stephen Hurd <stephen.hurd@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
--
v1->v2: regroup related patches and incorporate other review comments
---
drivers/net/bnxt/bnxt_hwrm.c | 38 ++++++++++++++++++
drivers/net/bnxt/bnxt_hwrm.h | 3 ++
drivers/net/bnxt/rte_pmd_bnxt.c | 87 +++++++++++++++++++++++++++++++++++++++++
drivers/net/bnxt/rte_pmd_bnxt.h | 64 ++++++++++++++++++++++++++++++
4 files changed, 192 insertions(+)
Comments
On 5/26/2017 7:39 PM, Ajit Khaparde wrote:
> This patch adds code to get and clear VF stats.
> It also adds the necessary HWRM structures to send the command
> to the firmware.
>
> Signed-off-by: Stephen Hurd <stephen.hurd@broadcom.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> --
> v1->v2: regroup related patches and incorporate other review comments
> ---
> drivers/net/bnxt/bnxt_hwrm.c | 38 ++++++++++++++++++
> drivers/net/bnxt/bnxt_hwrm.h | 3 ++
> drivers/net/bnxt/rte_pmd_bnxt.c | 87 +++++++++++++++++++++++++++++++++++++++++
> drivers/net/bnxt/rte_pmd_bnxt.h | 64 ++++++++++++++++++++++++++++++
> 4 files changed, 192 insertions(+)
It looks like new APIs not added into .map file.
This doesn't cause a build error because they are not used anywhere,
that is why using testpmd to implement them helps to find any issues.
<...>
> +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count)
> +{
> + struct rte_eth_dev *dev;
> + struct rte_eth_dev_info dev_info;
> + struct bnxt *bp;
> +
> + dev = &rte_eth_devices[port];
> + rte_eth_dev_info_get(port, &dev_info);
> + bp = (struct bnxt *)dev->data->dev_private;
> +
> + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> +}
This function is not to get VF stats from PF. As far as I can see this
just gets queue stats, does this really needs to be PMD specific API,
isn't this something generic?
<...>
On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:
> > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count)
> > +{
> > + struct rte_eth_dev *dev;
> > + struct rte_eth_dev_info dev_info;
> > + struct bnxt *bp;
> > +
> > + dev = &rte_eth_devices[port];
> > + rte_eth_dev_info_get(port, &dev_info);
> > + bp = (struct bnxt *)dev->data->dev_private;
> > +
> > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> > +}
>
> This function is not to get VF stats from PF. As far as I can see this
> just gets queue stats, does this really needs to be PMD specific API,
> isn't this something generic?
Yes. That is right. It returns a count of number of packets which were not
transmitted
because it did not pass the MAC/VLAN spoof check. It does not necessarily
mean "failure to transmit" and so I don't think it is right to map it to
oerrors.
So in the current form I don't see a way to make a generic function out of
it.
On 5/31/2017 3:12 AM, Ajit Khaparde wrote:
>
> On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
>
> > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count)
> > +{
> > + struct rte_eth_dev *dev;
> > + struct rte_eth_dev_info dev_info;
> > + struct bnxt *bp;
> > +
> > + dev = &rte_eth_devices[port];
> > + rte_eth_dev_info_get(port, &dev_info);
> > + bp = (struct bnxt *)dev->data->dev_private;
> > +
> > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> > +}
>
> This function is not to get VF stats from PF. As far as I can see this
> just gets queue stats, does this really needs to be PMD specific API,
> isn't this something generic?
>
>
> Yes. That is right. It returns a count of number of packets which were
> not transmitted
> because it did not pass the MAC/VLAN spoof check.
> It does not
> necessarily mean "failure to transmit" and so I don't think it is right
> to map it to oerrors.
> So in the current form I don't see a way to make a generic function out
> of it.
I see, this is implemented because there is no place in basic stats to
put tx_drop_pkts.
Can xstats be used to get this value? Can new .xstats_get_by_id help here?
On Wed, May 31, 2017 at 4:57 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:
> On 5/31/2017 3:12 AM, Ajit Khaparde wrote:
> >
> > On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> > > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count)
> > > +{
> > > + struct rte_eth_dev *dev;
> > > + struct rte_eth_dev_info dev_info;
> > > + struct bnxt *bp;
> > > +
> > > + dev = &rte_eth_devices[port];
> > > + rte_eth_dev_info_get(port, &dev_info);
> > > + bp = (struct bnxt *)dev->data->dev_private;
> > > +
> > > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> > > +}
> >
> > This function is not to get VF stats from PF. As far as I can see
> this
> > just gets queue stats, does this really needs to be PMD specific API,
> > isn't this something generic?
> >
> >
> > Yes. That is right. It returns a count of number of packets which were
> > not transmitted
> > because it did not pass the MAC/VLAN spoof check.
> > It does not
> > necessarily mean "failure to transmit" and so I don't think it is right
> > to map it to oerrors.
> > So in the current form I don't see a way to make a generic function out
> > of it.
>
> I see, this is implemented because there is no place in basic stats to
> put tx_drop_pkts.
>
> Can xstats be used to get this value? Can new .xstats_get_by_id help here?
>
May be we could. Do we have time for that?
On 5/31/2017 3:27 PM, Ajit Khaparde wrote:
> On Wed, May 31, 2017 at 4:57 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>>wrote:
>
> On 5/31/2017 3:12 AM, Ajit Khaparde wrote:
> >
> > On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
> > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
> wrote:
> >
> > > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t
> *count)
> > > +{
> > > + struct rte_eth_dev *dev;
> > > + struct rte_eth_dev_info dev_info;
> > > + struct bnxt *bp;
> > > +
> > > + dev = &rte_eth_devices[port];
> > > + rte_eth_dev_info_get(port, &dev_info);
> > > + bp = (struct bnxt *)dev->data->dev_private;
> > > +
> > > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> > > +}
> >
> > This function is not to get VF stats from PF. As far as I can
> see this
> > just gets queue stats, does this really needs to be PMD
> specific API,
> > isn't this something generic?
> >
> >
> > Yes. That is right. It returns a count of number of packets which
> were
> > not transmitted
> > because it did not pass the MAC/VLAN spoof check.
> > It does not
> > necessarily mean "failure to transmit" and so I don't think it is
> right
> > to map it to oerrors.
> > So in the current form I don't see a way to make a generic
> function out
> > of it.
>
> I see, this is implemented because there is no place in basic stats to
> put tx_drop_pkts.
>
> Can xstats be used to get this value? Can new .xstats_get_by_id help
> here?
>
> May be we could. Do we have time for that?
Agreed that this may require effort, but I believe we should not use PMD
specific APIs as much as possible, and stick to ethdev abstraction layer.
PMD specific API kills the portability, and only should be used to
benefit from features that are available only for that PMD.
This was my concern to start implementing more thing in PMD layer, like
getting tx_drop stats..
Thanks,
ferruh
31/05/2017 16:46, Ferruh Yigit:
> On 5/31/2017 3:27 PM, Ajit Khaparde wrote:
> > On Wed, May 31, 2017 at 4:57 AM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>>wrote:
> >
> > On 5/31/2017 3:12 AM, Ajit Khaparde wrote:
> > >
> > > On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
> > > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
> > wrote:
> > >
> > > > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t
> > *count)
> > > > +{
> > > > + struct rte_eth_dev *dev;
> > > > + struct rte_eth_dev_info dev_info;
> > > > + struct bnxt *bp;
> > > > +
> > > > + dev = &rte_eth_devices[port];
> > > > + rte_eth_dev_info_get(port, &dev_info);
> > > > + bp = (struct bnxt *)dev->data->dev_private;
> > > > +
> > > > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
> > > > +}
> > >
> > > This function is not to get VF stats from PF. As far as I can
> > see this
> > > just gets queue stats, does this really needs to be PMD
> > specific API,
> > > isn't this something generic?
> > >
> > >
> > > Yes. That is right. It returns a count of number of packets which
> > were
> > > not transmitted
> > > because it did not pass the MAC/VLAN spoof check.
> > > It does not
> > > necessarily mean "failure to transmit" and so I don't think it is
> > right
> > > to map it to oerrors.
> > > So in the current form I don't see a way to make a generic
> > function out
> > > of it.
> >
> > I see, this is implemented because there is no place in basic stats to
> > put tx_drop_pkts.
> >
> > Can xstats be used to get this value? Can new .xstats_get_by_id help
> > here?
> >
> > May be we could. Do we have time for that?
>
> Agreed that this may require effort, but I believe we should not use PMD
> specific APIs as much as possible, and stick to ethdev abstraction layer.
>
> PMD specific API kills the portability, and only should be used to
> benefit from features that are available only for that PMD.
>
> This was my concern to start implementing more thing in PMD layer, like
> getting tx_drop stats..
Yes I agree the specific statistics should be added in xstats.
On Wed, May 31, 2017 at 11:15 AM, Thomas Monjalon <thomas@monjalon.net>
wrote:
> 31/05/2017 16:46, Ferruh Yigit:
> > On 5/31/2017 3:27 PM, Ajit Khaparde wrote:
> > > On Wed, May 31, 2017 at 4:57 AM, Ferruh Yigit <ferruh.yigit@intel.com
> > > <mailto:ferruh.yigit@intel.com>>wrote:
> > >
> > > On 5/31/2017 3:12 AM, Ajit Khaparde wrote:
> > > >
> > > > On Mon, May 29, 2017 at 12:43 PM, Ferruh Yigit <
> ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>
> > > > <mailto:ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>>
> > > wrote:
> > > >
> > > > > +int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t
> > > *count)
> > > > > +{
> > > > > + struct rte_eth_dev *dev;
> > > > > + struct rte_eth_dev_info dev_info;
> > > > > + struct bnxt *bp;
> > > > > +
> > > > > + dev = &rte_eth_devices[port];
> > > > > + rte_eth_dev_info_get(port, &dev_info);
> > > > > + bp = (struct bnxt *)dev->data->dev_private;
> > > > > +
> > > > > + return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff,
> count);
> > > > > +}
> > > >
> > > > This function is not to get VF stats from PF. As far as I can
> > > see this
> > > > just gets queue stats, does this really needs to be PMD
> > > specific API,
> > > > isn't this something generic?
> > > >
> > > >
> > > > Yes. That is right. It returns a count of number of packets
> which
> > > were
> > > > not transmitted
> > > > because it did not pass the MAC/VLAN spoof check.
> > > > It does not
> > > > necessarily mean "failure to transmit" and so I don't think it is
> > > right
> > > > to map it to oerrors.
> > > > So in the current form I don't see a way to make a generic
> > > function out
> > > > of it.
> > >
> > > I see, this is implemented because there is no place in basic
> stats to
> > > put tx_drop_pkts.
> > >
> > > Can xstats be used to get this value? Can new .xstats_get_by_id
> help
> > > here?
> > >
> > > May be we could. Do we have time for that?
> >
> > Agreed that this may require effort, but I believe we should not use PMD
> > specific APIs as much as possible, and stick to ethdev abstraction layer.
> >
> > PMD specific API kills the portability, and only should be used to
> > benefit from features that are available only for that PMD.
> >
> > This was my concern to start implementing more thing in PMD layer, like
> > getting tx_drop stats..
>
> Yes I agree the specific statistics should be added in xstats.
>
And I am already working on that. Thanks
@@ -1237,6 +1237,27 @@ int bnxt_hwrm_func_vf_mac(struct bnxt *bp, uint16_t vf, const uint8_t *mac_addr)
return rc;
}
+int bnxt_hwrm_func_qstats_tx_drop(struct bnxt *bp, uint16_t fid,
+ uint64_t *dropped)
+{
+ int rc = 0;
+ struct hwrm_func_qstats_input req = {.req_type = 0};
+ struct hwrm_func_qstats_output *resp = bp->hwrm_cmd_resp_addr;
+
+ HWRM_PREP(req, FUNC_QSTATS, -1, resp);
+
+ req.fid = rte_cpu_to_le_16(fid);
+
+ rc = bnxt_hwrm_send_message(bp, &req, sizeof(req));
+
+ HWRM_CHECK_RESULT;
+
+ if (dropped)
+ *dropped = rte_le_to_cpu_64(resp->tx_drop_pkts);
+
+ return rc;
+}
+
int bnxt_hwrm_func_qstats(struct bnxt *bp, uint16_t fid,
struct rte_eth_stats *stats)
{
@@ -1274,6 +1295,23 @@ int bnxt_hwrm_func_qstats(struct bnxt *bp, uint16_t fid,
return rc;
}
+int bnxt_hwrm_func_clr_stats(struct bnxt *bp, uint16_t fid)
+{
+ int rc = 0;
+ struct hwrm_func_clr_stats_input req = {.req_type = 0};
+ struct hwrm_func_clr_stats_output *resp = bp->hwrm_cmd_resp_addr;
+
+ HWRM_PREP(req, FUNC_CLR_STATS, -1, resp);
+
+ req.fid = rte_cpu_to_le_16(fid);
+
+ rc = bnxt_hwrm_send_message(bp, &req, sizeof(req));
+
+ HWRM_CHECK_RESULT;
+
+ return rc;
+}
+
/*
* HWRM utility functions
*/
@@ -66,6 +66,9 @@ int bnxt_hwrm_func_reset(struct bnxt *bp);
int bnxt_hwrm_func_driver_unregister(struct bnxt *bp, uint32_t flags);
int bnxt_hwrm_func_qstats(struct bnxt *bp, uint16_t fid,
struct rte_eth_stats *stats);
+int bnxt_hwrm_func_qstats_tx_drop(struct bnxt *bp, uint16_t fid,
+ uint64_t *dropped);
+int bnxt_hwrm_func_clr_stats(struct bnxt *bp, uint16_t fid);
int bnxt_hwrm_func_cfg_def_cp(struct bnxt *bp);
int bnxt_hwrm_vf_func_cfg_def_cp(struct bnxt *bp);
@@ -430,3 +430,90 @@ int rte_pmd_bnxt_set_vf_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
return rc;
}
+
+int rte_pmd_bnxt_get_vf_stats(uint8_t port,
+ uint16_t vf_id,
+ struct rte_eth_stats *stats)
+{
+ struct rte_eth_dev *dev;
+ struct rte_eth_dev_info dev_info;
+ struct bnxt *bp;
+
+ dev = &rte_eth_devices[port];
+ rte_eth_dev_info_get(port, &dev_info);
+ bp = (struct bnxt *)dev->data->dev_private;
+
+ if (vf_id >= dev_info.max_vfs)
+ return -EINVAL;
+
+ if (!BNXT_PF(bp)) {
+ RTE_LOG(ERR, PMD,
+ "Attempt to get VF %d stats on non-PF port %d!\n",
+ vf_id, port);
+ return -ENOTSUP;
+ }
+
+ return bnxt_hwrm_func_qstats(bp, bp->pf.first_vf_id + vf_id, stats);
+}
+
+int rte_pmd_bnxt_reset_vf_stats(uint8_t port,
+ uint16_t vf_id)
+{
+ struct rte_eth_dev *dev;
+ struct rte_eth_dev_info dev_info;
+ struct bnxt *bp;
+
+ dev = &rte_eth_devices[port];
+ rte_eth_dev_info_get(port, &dev_info);
+ bp = (struct bnxt *)dev->data->dev_private;
+
+ if (vf_id >= dev_info.max_vfs)
+ return -EINVAL;
+
+ if (!BNXT_PF(bp)) {
+ RTE_LOG(ERR, PMD,
+ "Attempt to reset VF %d stats on non-PF port %d!\n",
+ vf_id, port);
+ return -ENOTSUP;
+ }
+
+ return bnxt_hwrm_func_clr_stats(bp, bp->pf.first_vf_id + vf_id);
+}
+
+int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count)
+{
+ struct rte_eth_dev *dev;
+ struct rte_eth_dev_info dev_info;
+ struct bnxt *bp;
+
+ dev = &rte_eth_devices[port];
+ rte_eth_dev_info_get(port, &dev_info);
+ bp = (struct bnxt *)dev->data->dev_private;
+
+ return bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, count);
+}
+
+int rte_pmd_bnxt_get_vf_tx_drop_count(uint8_t port, uint16_t vf_id,
+ uint64_t *count)
+{
+ struct rte_eth_dev *dev;
+ struct rte_eth_dev_info dev_info;
+ struct bnxt *bp;
+
+ dev = &rte_eth_devices[port];
+ rte_eth_dev_info_get(port, &dev_info);
+ bp = (struct bnxt *)dev->data->dev_private;
+
+ if (vf_id >= dev_info.max_vfs)
+ return -EINVAL;
+
+ if (!BNXT_PF(bp)) {
+ RTE_LOG(ERR, PMD,
+ "Attempt to query VF %d TX drops on non-PF port %d!\n",
+ vf_id, port);
+ return -ENOTSUP;
+ }
+
+ return bnxt_hwrm_func_qstats_tx_drop(bp, bp->pf.first_vf_id + vf_id,
+ count);
+}
@@ -165,6 +165,41 @@ int rte_pmd_bnxt_set_vf_rate_limit(uint8_t port, uint16_t vf,
uint16_t tx_rate, uint64_t q_msk);
/**
+ * Get VF's statistics
+ *
+ * @param port
+ * The port identifier of the Ethernet device.
+ * @param vf_id
+ * VF on which to get.
+ * @param stats
+ * A pointer to a structure of type *rte_eth_stats* to be filled with
+ * the values of device counters supported statistics:
+ * @return
+ * - (0) if successful.
+ * - (-ENODEV) if *port* invalid.
+ * - (-EINVAL) if bad parameter.
+ */
+
+int rte_pmd_bnxt_get_vf_stats(uint8_t port,
+ uint16_t vf_id,
+ struct rte_eth_stats *stats);
+
+/**
+ * Clear VF's statistics
+ *
+ * @param port
+ * The port identifier of the Ethernet device.
+ * @param vf_id
+ * VF on which to get.
+ * @return
+ * - (0) if successful.
+ * - (-ENODEV) if *port* invalid.
+ * - (-EINVAL) if bad parameter.
+ */
+int rte_pmd_bnxt_reset_vf_stats(uint8_t port,
+ uint16_t vf_id);
+
+/**
* Enable/Disable VF VLAN anti spoof
*
* @param port
@@ -181,4 +216,33 @@ int rte_pmd_bnxt_set_vf_rate_limit(uint8_t port, uint16_t vf,
* - (-EINVAL) if bad parameter.
*/
int rte_pmd_bnxt_set_vf_vlan_anti_spoof(uint8_t port, uint16_t vf, uint8_t on);
+
+/**
+ * Queries the TX drop counter for the function
+ *
+ * @param port
+ * The port identifier of the Ethernet device.
+ * @param count
+ * Pointer to a uint64_t that will be populated with the counter value.
+ * @return
+ * - Positive Non-zero value - Error code from HWRM
+ */
+int rte_pmd_bnxt_get_tx_drop_count(uint8_t port, uint64_t *count);
+
+/**
+ * Queries the TX drop counter for the function
+ *
+ * @param port
+ * The port identifier of the Ethernet device.
+ * @param vf_id
+ * VF on which to get.
+ * @param count
+ * Pointer to a uint64_t that will be populated with the counter value.
+ * @return
+ * - Positive Non-zero value - Error code from HWRM
+ * - (-EINVAL) invalid vf_id specified.
+ * - (-ENOTSUP) Ethernet device is not a PF
+ */
+int rte_pmd_bnxt_get_vf_tx_drop_count(uint8_t port, uint16_t vf_id,
+ uint64_t *count);
#endif /* _PMD_BNXT_H_ */