[1/4] net/bnxt: fix extended port counter statistics
Checks
Commit Message
From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
We were trying to fill in more rx extended stats than the size allocated
for stats causing segfault. Fixed this by adding an explicit check.
Rearranged the code to return statistic values in xstats_get as per the
names returned in xstats_get_names.
Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
Signed-off-by: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
Comments
On Thu, Jul 25, 2019 at 7:05 AM Somnath Kotur
<somnath.kotur@broadcom.com> wrote:
>
> From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
>
> We were trying to fill in more rx extended stats than the size allocated
> for stats causing segfault. Fixed this by adding an explicit check.
> Rearranged the code to return statistic values in xstats_get as per the
> names returned in xstats_get_names.
>
> Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
>
> Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
> Signed-off-by: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> ---
> drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
> index 4e74f8a..69ac2dd 100644
> --- a/drivers/net/bnxt/bnxt_stats.c
> +++ b/drivers/net/bnxt/bnxt_stats.c
[snip]
> @@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev *eth_dev,
> xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
> count++;
>
> - for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> - uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
> + for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> + uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
>
> xstats[count].value = rte_le_to_cpu_64
> - (*(uint64_t *)((char *)tx_stats_ext +
> - bnxt_tx_ext_stats_strings[i].offset));
> + (*(uint64_t *)((char *)rx_stats_ext +
> + bnxt_rx_ext_stats_strings[i].offset));
>
> count++;
> }
>
> - for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> - uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
> + for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> + uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
>
> xstats[count].value = rte_le_to_cpu_64
> - (*(uint64_t *)((char *)rx_stats_ext +
> - bnxt_rx_ext_stats_strings[i].offset));
> + (*(uint64_t *)((char *)tx_stats_ext +
> + bnxt_tx_ext_stats_strings[i].offset));
>
> count++;
> }
> --
> 1.8.3.1
>
This whole hunk just adds some noise, right? or is there anything fixed in it?
+Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
On Thu, Jul 25, 2019 at 10:35 AM Somnath Kotur <somnath.kotur@broadcom.com>
wrote:
> From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
>
> We were trying to fill in more rx extended stats than the size allocated
> for stats causing segfault. Fixed this by adding an explicit check.
> Rearranged the code to return statistic values in xstats_get as per the
> names returned in xstats_get_names.
>
> Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
>
> Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
> Signed-off-by: Santoshkumar Karanappa Rastapur <
> santosh.rastapur@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> ---
> drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
> index 4e74f8a..69ac2dd 100644
> --- a/drivers/net/bnxt/bnxt_stats.c
> +++ b/drivers/net/bnxt/bnxt_stats.c
> @@ -427,8 +427,12 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev
> *eth_dev,
> bnxt_hwrm_port_qstats(bp);
> bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, &tx_drop_pkts);
> bnxt_hwrm_ext_port_qstats(bp);
> - rx_port_stats_ext_cnt = bp->fw_rx_port_stats_ext_size / stat_size;
> - tx_port_stats_ext_cnt = bp->fw_tx_port_stats_ext_size / stat_size;
> + rx_port_stats_ext_cnt = RTE_MIN(RTE_DIM(bnxt_rx_ext_stats_strings),
> + (bp->fw_rx_port_stats_ext_size /
> + stat_size));
> + tx_port_stats_ext_cnt = RTE_MIN(RTE_DIM(bnxt_tx_ext_stats_strings),
> + (bp->fw_tx_port_stats_ext_size /
> + stat_size));
>
> count = RTE_DIM(bnxt_rx_stats_strings) +
> RTE_DIM(bnxt_tx_stats_strings) + 1/* For tx_drop_pkts */ +
> @@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev
> *eth_dev,
> xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
> count++;
>
> - for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> - uint64_t *tx_stats_ext = (uint64_t
> *)bp->hw_tx_port_stats_ext;
> + for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> + uint64_t *rx_stats_ext = (uint64_t
> *)bp->hw_rx_port_stats_ext;
>
> xstats[count].value = rte_le_to_cpu_64
> - (*(uint64_t *)((char
> *)tx_stats_ext +
> -
> bnxt_tx_ext_stats_strings[i].offset));
> + (*(uint64_t *)((char
> *)rx_stats_ext +
> +
> bnxt_rx_ext_stats_strings[i].offset));
>
> count++;
> }
>
> - for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> - uint64_t *rx_stats_ext = (uint64_t
> *)bp->hw_rx_port_stats_ext;
> + for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> + uint64_t *tx_stats_ext = (uint64_t
> *)bp->hw_tx_port_stats_ext;
>
> xstats[count].value = rte_le_to_cpu_64
> - (*(uint64_t *)((char
> *)rx_stats_ext +
> -
> bnxt_rx_ext_stats_strings[i].offset));
> + (*(uint64_t *)((char
> *)tx_stats_ext +
> +
> bnxt_tx_ext_stats_strings[i].offset));
>
> count++;
> }
> --
> 1.8.3.1
>
>
+Santosh
On Thu, Jul 25, 2019 at 12:52 PM David Marchand <david.marchand@redhat.com>
wrote:
> On Thu, Jul 25, 2019 at 7:05 AM Somnath Kotur
> <somnath.kotur@broadcom.com> wrote:
> >
> > From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
> >
> > We were trying to fill in more rx extended stats than the size allocated
> > for stats causing segfault. Fixed this by adding an explicit check.
> > Rearranged the code to return statistic values in xstats_get as per the
> > names returned in xstats_get_names.
> >
> > Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
> >
> > Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
> > Signed-off-by: Santoshkumar Karanappa Rastapur <
> santosh.rastapur@broadcom.com>
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > ---
> > drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_stats.c
> b/drivers/net/bnxt/bnxt_stats.c
> > index 4e74f8a..69ac2dd 100644
> > --- a/drivers/net/bnxt/bnxt_stats.c
> > +++ b/drivers/net/bnxt/bnxt_stats.c
>
> [snip]
>
> > @@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev
> *eth_dev,
> > xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
> > count++;
> >
> > - for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> > - uint64_t *tx_stats_ext = (uint64_t
> *)bp->hw_tx_port_stats_ext;
> > + for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> > + uint64_t *rx_stats_ext = (uint64_t
> *)bp->hw_rx_port_stats_ext;
> >
> > xstats[count].value = rte_le_to_cpu_64
> > - (*(uint64_t *)((char
> *)tx_stats_ext +
> > -
> bnxt_tx_ext_stats_strings[i].offset));
> > + (*(uint64_t *)((char
> *)rx_stats_ext +
> > +
> bnxt_rx_ext_stats_strings[i].offset));
> >
> > count++;
> > }
> >
> > - for (i = 0; i < rx_port_stats_ext_cnt; i++) {
> > - uint64_t *rx_stats_ext = (uint64_t
> *)bp->hw_rx_port_stats_ext;
> > + for (i = 0; i < tx_port_stats_ext_cnt; i++) {
> > + uint64_t *tx_stats_ext = (uint64_t
> *)bp->hw_tx_port_stats_ext;
> >
> > xstats[count].value = rte_le_to_cpu_64
> > - (*(uint64_t *)((char
> *)rx_stats_ext +
> > -
> bnxt_rx_ext_stats_strings[i].offset));
> > + (*(uint64_t *)((char
> *)tx_stats_ext +
> > +
> bnxt_tx_ext_stats_strings[i].offset));
> >
> > count++;
> > }
> > --
> > 1.8.3.1
> >
>
> This whole hunk just adds some noise, right? or is there anything fixed in
> it?
>
>
> --
> David Marchand
>
On Thu, Jul 25, 2019 at 1:09 PM Somnath Kotur <somnath.kotur@broadcom.com>
wrote:
> +Santosh
>
> On Thu, Jul 25, 2019 at 12:52 PM David Marchand <david.marchand@redhat.com>
> wrote:
>
>> On Thu, Jul 25, 2019 at 7:05 AM Somnath Kotur
>> <somnath.kotur@broadcom.com> wrote:
>> >
>> > From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
>> >
>> > We were trying to fill in more rx extended stats than the size allocated
>> > for stats causing segfault. Fixed this by adding an explicit check.
>> > Rearranged the code to return statistic values in xstats_get as per the
>> > names returned in xstats_get_names.
>> >
>> > Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
>> >
>> > Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
>> > Signed-off-by: Santoshkumar Karanappa Rastapur <
>> santosh.rastapur@broadcom.com>
>> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>> > ---
>> > drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
>> > 1 file changed, 14 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/net/bnxt/bnxt_stats.c
>> b/drivers/net/bnxt/bnxt_stats.c
>> > index 4e74f8a..69ac2dd 100644
>> > --- a/drivers/net/bnxt/bnxt_stats.c
>> > +++ b/drivers/net/bnxt/bnxt_stats.c
>>
>> [snip]
>>
>> > @@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev
>> *eth_dev,
>> > xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
>> > count++;
>> >
>> > - for (i = 0; i < tx_port_stats_ext_cnt; i++) {
>> > - uint64_t *tx_stats_ext = (uint64_t
>> *)bp->hw_tx_port_stats_ext;
>> > + for (i = 0; i < rx_port_stats_ext_cnt; i++) {
>> > + uint64_t *rx_stats_ext = (uint64_t
>> *)bp->hw_rx_port_stats_ext;
>> >
>> > xstats[count].value = rte_le_to_cpu_64
>> > - (*(uint64_t *)((char
>> *)tx_stats_ext +
>> > -
>> bnxt_tx_ext_stats_strings[i].offset));
>> > + (*(uint64_t *)((char
>> *)rx_stats_ext +
>> > +
>> bnxt_rx_ext_stats_strings[i].offset));
>> >
>> > count++;
>> > }
>> >
>> > - for (i = 0; i < rx_port_stats_ext_cnt; i++) {
>> > - uint64_t *rx_stats_ext = (uint64_t
>> *)bp->hw_rx_port_stats_ext;
>> > + for (i = 0; i < tx_port_stats_ext_cnt; i++) {
>> > + uint64_t *tx_stats_ext = (uint64_t
>> *)bp->hw_tx_port_stats_ext;
>> >
>> > xstats[count].value = rte_le_to_cpu_64
>> > - (*(uint64_t *)((char
>> *)rx_stats_ext +
>> > -
>> bnxt_rx_ext_stats_strings[i].offset));
>> > + (*(uint64_t *)((char
>> *)tx_stats_ext +
>> > +
>> bnxt_tx_ext_stats_strings[i].offset));
>> >
>> > count++;
>> > }
>> > --
>> > 1.8.3.1
>> >
>>
>> This whole hunk just adds some noise, right? or is there anything fixed
>> in it?
>>
>>
>> --
>> David Marchand
>>
>
In bnxt_dev_xstats_get_names_op, we were filling statistics names in
xstats_names in this order.
bnxt_rx_stats_strings
bnxt_tx_stats_strings
bnxt_rx_ext_stats_strings
bnxt_tx_ext_stats_strings
Where as in bnxt_dev_xstats_get_op, we were returning stats values in
xstats in this order.
bnxt_rx_stats_strings
bnxt_tx_stats_strings
bnxt_tx_ext_stats_strings
bnxt_rx_ext_stats_strings
We were ending up displaying extended Tx stats values against extended Rx
stats names and vice versa.
This above code fixes this order.
Regards
-Santosh
On Thu, Jul 25, 2019 at 9:54 AM Santoshkumar Karanappa Rastapur
<santosh.rastapur@broadcom.com> wrote:
>
>
>
> On Thu, Jul 25, 2019 at 1:09 PM Somnath Kotur <somnath.kotur@broadcom.com> wrote:
>>
>> +Santosh
>>
>> On Thu, Jul 25, 2019 at 12:52 PM David Marchand <david.marchand@redhat.com> wrote:
>>>
>>> On Thu, Jul 25, 2019 at 7:05 AM Somnath Kotur
>>> <somnath.kotur@broadcom.com> wrote:
>>> >
>>> > From: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
>>> >
>>> > We were trying to fill in more rx extended stats than the size allocated
>>> > for stats causing segfault. Fixed this by adding an explicit check.
>>> > Rearranged the code to return statistic values in xstats_get as per the
>>> > names returned in xstats_get_names.
>>> >
>>> > Fixes: f55e12f33416 ("net/bnxt: support extended port counters")
>>> >
>>> > Signed-off-by: Rahul Gupta <rahul.gupta@broadcom.com>
>>> > Signed-off-by: Santoshkumar Karanappa Rastapur <santosh.rastapur@broadcom.com>
>>> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> > ---
>>> > drivers/net/bnxt/bnxt_stats.c | 24 ++++++++++++++----------
>>> > 1 file changed, 14 insertions(+), 10 deletions(-)
>>> >
>>> > diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
>>> > index 4e74f8a..69ac2dd 100644
>>> > --- a/drivers/net/bnxt/bnxt_stats.c
>>> > +++ b/drivers/net/bnxt/bnxt_stats.c
>>>
>>> [snip]
>>>
>>> > @@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev *eth_dev,
>>> > xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
>>> > count++;
>>> >
>>> > - for (i = 0; i < tx_port_stats_ext_cnt; i++) {
>>> > - uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
>>> > + for (i = 0; i < rx_port_stats_ext_cnt; i++) {
>>> > + uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
>>> >
>>> > xstats[count].value = rte_le_to_cpu_64
>>> > - (*(uint64_t *)((char *)tx_stats_ext +
>>> > - bnxt_tx_ext_stats_strings[i].offset));
>>> > + (*(uint64_t *)((char *)rx_stats_ext +
>>> > + bnxt_rx_ext_stats_strings[i].offset));
>>> >
>>> > count++;
>>> > }
>>> >
>>> > - for (i = 0; i < rx_port_stats_ext_cnt; i++) {
>>> > - uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
>>> > + for (i = 0; i < tx_port_stats_ext_cnt; i++) {
>>> > + uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
>>> >
>>> > xstats[count].value = rte_le_to_cpu_64
>>> > - (*(uint64_t *)((char *)rx_stats_ext +
>>> > - bnxt_rx_ext_stats_strings[i].offset));
>>> > + (*(uint64_t *)((char *)tx_stats_ext +
>>> > + bnxt_tx_ext_stats_strings[i].offset));
>>> >
>>> > count++;
>>> > }
>>> > --
>>> > 1.8.3.1
>>> >
>>>
>>> This whole hunk just adds some noise, right? or is there anything fixed in it?
>>>
>>>
>>> --
>>> David Marchand
>
>
>
> In bnxt_dev_xstats_get_names_op, we were filling statistics names in xstats_names in this order.
>
> bnxt_rx_stats_strings
>
> bnxt_tx_stats_strings
>
> bnxt_rx_ext_stats_strings
>
> bnxt_tx_ext_stats_strings
>
>
>
> Where as in bnxt_dev_xstats_get_op, we were returning stats values in xstats in this order.
>
> bnxt_rx_stats_strings
>
> bnxt_tx_stats_strings
>
> bnxt_tx_ext_stats_strings
>
> bnxt_rx_ext_stats_strings
>
>
>
> We were ending up displaying extended Tx stats values against extended Rx stats names and vice versa.
>
> This above code fixes this order.
Erf, I must have read your commitlog too quickly, or you could have
split it in two patches, anyway understood.
@@ -427,8 +427,12 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev *eth_dev,
bnxt_hwrm_port_qstats(bp);
bnxt_hwrm_func_qstats_tx_drop(bp, 0xffff, &tx_drop_pkts);
bnxt_hwrm_ext_port_qstats(bp);
- rx_port_stats_ext_cnt = bp->fw_rx_port_stats_ext_size / stat_size;
- tx_port_stats_ext_cnt = bp->fw_tx_port_stats_ext_size / stat_size;
+ rx_port_stats_ext_cnt = RTE_MIN(RTE_DIM(bnxt_rx_ext_stats_strings),
+ (bp->fw_rx_port_stats_ext_size /
+ stat_size));
+ tx_port_stats_ext_cnt = RTE_MIN(RTE_DIM(bnxt_tx_ext_stats_strings),
+ (bp->fw_tx_port_stats_ext_size /
+ stat_size));
count = RTE_DIM(bnxt_rx_stats_strings) +
RTE_DIM(bnxt_tx_stats_strings) + 1/* For tx_drop_pkts */ +
@@ -463,22 +467,22 @@ int bnxt_dev_xstats_get_op(struct rte_eth_dev *eth_dev,
xstats[count].value = rte_le_to_cpu_64(tx_drop_pkts);
count++;
- for (i = 0; i < tx_port_stats_ext_cnt; i++) {
- uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
+ for (i = 0; i < rx_port_stats_ext_cnt; i++) {
+ uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
xstats[count].value = rte_le_to_cpu_64
- (*(uint64_t *)((char *)tx_stats_ext +
- bnxt_tx_ext_stats_strings[i].offset));
+ (*(uint64_t *)((char *)rx_stats_ext +
+ bnxt_rx_ext_stats_strings[i].offset));
count++;
}
- for (i = 0; i < rx_port_stats_ext_cnt; i++) {
- uint64_t *rx_stats_ext = (uint64_t *)bp->hw_rx_port_stats_ext;
+ for (i = 0; i < tx_port_stats_ext_cnt; i++) {
+ uint64_t *tx_stats_ext = (uint64_t *)bp->hw_tx_port_stats_ext;
xstats[count].value = rte_le_to_cpu_64
- (*(uint64_t *)((char *)rx_stats_ext +
- bnxt_rx_ext_stats_strings[i].offset));
+ (*(uint64_t *)((char *)tx_stats_ext +
+ bnxt_tx_ext_stats_strings[i].offset));
count++;
}