[v7,18/21] net/cpfl: add HW statistics
Checks
Commit Message
This patch add hardware packets/bytes statistics.
Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
---
drivers/net/cpfl/cpfl_ethdev.c | 86 ++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
Comments
On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> This patch add hardware packets/bytes statistics.
>
> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
<...>
> +static int
> +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> +{
> + struct idpf_vport *vport =
> + (struct idpf_vport *)dev->data->dev_private;
> + struct virtchnl2_vport_stats *pstats = NULL;
> + int ret;
> +
> + ret = idpf_vc_stats_query(vport, &pstats);
> + if (ret == 0) {
> + uint8_t crc_stats_len = (dev->data->dev_conf.rxmode.offloads &
> + RTE_ETH_RX_OFFLOAD_KEEP_CRC) ? 0 :
> + RTE_ETHER_CRC_LEN;
> +
> + idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
> + stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> + pstats->rx_broadcast - pstats->rx_discards;
> + stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
> + pstats->tx_unicast;
> + stats->imissed = pstats->rx_discards;
> + stats->oerrors = pstats->tx_errors + pstats->tx_discards;
> + stats->ibytes = pstats->rx_bytes;
> + stats->ibytes -= stats->ipackets * crc_stats_len;
> + stats->obytes = pstats->tx_bytes;
> +
> + dev->data->rx_mbuf_alloc_failed = cpfl_get_mbuf_alloc_failed_stats(dev);
'dev->data->rx_mbuf_alloc_failed' is also used by telemetry, updating
here only in stats_get() will make it wrong for telemetry.
Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, February 28, 2023 5:52 AM
> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>
> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> > This patch add hardware packets/bytes statistics.
> >
> > Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>
> <...>
>
> > +static int
> > +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> > +*stats) {
> > + struct idpf_vport *vport =
> > + (struct idpf_vport *)dev->data->dev_private;
> > + struct virtchnl2_vport_stats *pstats = NULL;
> > + int ret;
> > +
> > + ret = idpf_vc_stats_query(vport, &pstats);
> > + if (ret == 0) {
> > + uint8_t crc_stats_len = (dev->data-
> >dev_conf.rxmode.offloads &
> > + RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> 0 :
> > + RTE_ETHER_CRC_LEN;
> > +
> > + idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
> > + stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> > + pstats->rx_broadcast - pstats->rx_discards;
> > + stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> +
> > + pstats->tx_unicast;
> > + stats->imissed = pstats->rx_discards;
> > + stats->oerrors = pstats->tx_errors + pstats->tx_discards;
> > + stats->ibytes = pstats->rx_bytes;
> > + stats->ibytes -= stats->ipackets * crc_stats_len;
> > + stats->obytes = pstats->tx_bytes;
> > +
> > + dev->data->rx_mbuf_alloc_failed =
> > +cpfl_get_mbuf_alloc_failed_stats(dev);
>
> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry, updating here
> only in stats_get() will make it wrong for telemetry.
>
> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever alloc
> failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
[Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure provided to user, user need to access through rte_ethdev APIs.
Because we already put rx and tx burst func to common/idpf which has no dependcy with ethdev lib. If I update "dev->data->rx_mbuf_alloc_failed"
when allocate mbuf fails, it will break the design of our common/idpf interface to net/cpfl or net.idpf.
And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed' in lib code.
On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, February 28, 2023 5:52 AM
>> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>
>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>> This patch add hardware packets/bytes statistics.
>>>
>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>
>> <...>
>>
>>> +static int
>>> +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
>>> +*stats) {
>>> + struct idpf_vport *vport =
>>> + (struct idpf_vport *)dev->data->dev_private;
>>> + struct virtchnl2_vport_stats *pstats = NULL;
>>> + int ret;
>>> +
>>> + ret = idpf_vc_stats_query(vport, &pstats);
>>> + if (ret == 0) {
>>> + uint8_t crc_stats_len = (dev->data-
>>> dev_conf.rxmode.offloads &
>>> + RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>> 0 :
>>> + RTE_ETHER_CRC_LEN;
>>> +
>>> + idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
>>> + stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
>>> + pstats->rx_broadcast - pstats->rx_discards;
>>> + stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>> +
>>> + pstats->tx_unicast;
>>> + stats->imissed = pstats->rx_discards;
>>> + stats->oerrors = pstats->tx_errors + pstats->tx_discards;
>>> + stats->ibytes = pstats->rx_bytes;
>>> + stats->ibytes -= stats->ipackets * crc_stats_len;
>>> + stats->obytes = pstats->tx_bytes;
>>> +
>>> + dev->data->rx_mbuf_alloc_failed =
>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>
>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry, updating here
>> only in stats_get() will make it wrong for telemetry.
>>
>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever alloc
>> failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure provided to user, user need to access through rte_ethdev APIs.
> Because we already put rx and tx burst func to common/idpf which has no dependcy with ethdev lib. If I update "dev->data->rx_mbuf_alloc_failed"
> when allocate mbuf fails, it will break the design of our common/idpf interface to net/cpfl or net.idpf.
>
> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed' in lib code.
>
Please check 'eth_dev_handle_port_info()' function.
As I said this is used by telemetry, not directly exposed to the user.
I got the design concern, perhaps you can put a brief limitation to the
driver documentation.
OK, got it.
As our previous design did have flaws.
And if we don't want to affect correctness of telemetry, we have to redesign the idpf common module code,
which means a lot of work to do, so can we lower the priority of this issue?
Thanks,
BR,
mingxia
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, February 28, 2023 6:02 PM
> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>
> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Tuesday, February 28, 2023 5:52 AM
> >> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>
> >> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> >>> This patch add hardware packets/bytes statistics.
> >>>
> >>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> >>
> >> <...>
> >>
> >>> +static int
> >>> +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> >>> +*stats) {
> >>> + struct idpf_vport *vport =
> >>> + (struct idpf_vport *)dev->data->dev_private;
> >>> + struct virtchnl2_vport_stats *pstats = NULL;
> >>> + int ret;
> >>> +
> >>> + ret = idpf_vc_stats_query(vport, &pstats);
> >>> + if (ret == 0) {
> >>> + uint8_t crc_stats_len = (dev->data-
> >>> dev_conf.rxmode.offloads &
> >>> + RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> >> 0 :
> >>> + RTE_ETHER_CRC_LEN;
> >>> +
> >>> + idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
> >>> + stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> >>> + pstats->rx_broadcast - pstats->rx_discards;
> >>> + stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> >> +
> >>> + pstats->tx_unicast;
> >>> + stats->imissed = pstats->rx_discards;
> >>> + stats->oerrors = pstats->tx_errors + pstats->tx_discards;
> >>> + stats->ibytes = pstats->rx_bytes;
> >>> + stats->ibytes -= stats->ipackets * crc_stats_len;
> >>> + stats->obytes = pstats->tx_bytes;
> >>> +
> >>> + dev->data->rx_mbuf_alloc_failed =
> >>> +cpfl_get_mbuf_alloc_failed_stats(dev);
> >>
> >> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry, updating
> >> here only in stats_get() will make it wrong for telemetry.
> >>
> >> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
> >> alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> > [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure provided
> to user, user need to access through rte_ethdev APIs.
> > Because we already put rx and tx burst func to common/idpf which has no
> dependcy with ethdev lib. If I update "dev->data->rx_mbuf_alloc_failed"
> > when allocate mbuf fails, it will break the design of our common/idpf
> interface to net/cpfl or net.idpf.
> >
> > And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed' in lib
> code.
> >
>
> Please check 'eth_dev_handle_port_info()' function.
> As I said this is used by telemetry, not directly exposed to the user.
>
> I got the design concern, perhaps you can put a brief limitation to the driver
> documentation.
>
On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
Comment moved down, please don't top post, it makes very hard to follow
discussion.
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, February 28, 2023 6:02 PM
>> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>
>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Tuesday, February 28, 2023 5:52 AM
>>>> To: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>
>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>>>> This patch add hardware packets/bytes statistics.
>>>>>
>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> +static int
>>>>> +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
>>>>> +*stats) {
>>>>> + struct idpf_vport *vport =
>>>>> + (struct idpf_vport *)dev->data->dev_private;
>>>>> + struct virtchnl2_vport_stats *pstats = NULL;
>>>>> + int ret;
>>>>> +
>>>>> + ret = idpf_vc_stats_query(vport, &pstats);
>>>>> + if (ret == 0) {
>>>>> + uint8_t crc_stats_len = (dev->data-
>>>>> dev_conf.rxmode.offloads &
>>>>> + RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>>>> 0 :
>>>>> + RTE_ETHER_CRC_LEN;
>>>>> +
>>>>> + idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
>>>>> + stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
>>>>> + pstats->rx_broadcast - pstats->rx_discards;
>>>>> + stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>>>> +
>>>>> + pstats->tx_unicast;
>>>>> + stats->imissed = pstats->rx_discards;
>>>>> + stats->oerrors = pstats->tx_errors + pstats->tx_discards;
>>>>> + stats->ibytes = pstats->rx_bytes;
>>>>> + stats->ibytes -= stats->ipackets * crc_stats_len;
>>>>> + stats->obytes = pstats->tx_bytes;
>>>>> +
>>>>> + dev->data->rx_mbuf_alloc_failed =
>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>>>
>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry, updating
>>>> here only in stats_get() will make it wrong for telemetry.
>>>>
>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
>>>> alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure provided
>> to user, user need to access through rte_ethdev APIs.
>>> Because we already put rx and tx burst func to common/idpf which has no
>> dependcy with ethdev lib. If I update "dev->data->rx_mbuf_alloc_failed"
>>> when allocate mbuf fails, it will break the design of our common/idpf
>> interface to net/cpfl or net.idpf.
>>>
>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed' in lib
>> code.
>>>
>>
>> Please check 'eth_dev_handle_port_info()' function.
>> As I said this is used by telemetry, not directly exposed to the user.
>>
>> I got the design concern, perhaps you can put a brief limitation to the driver
>> documentation.
>>
> OK, got it.
>
> As our previous design did have flaws.
> And if we don't want to affect correctness of telemetry, we have to redesign the idpf common module code,
> which means a lot of work to do, so can we lower the priority of this issue?
>
I don't believe this is urgent, can you but a one line limitation to the
documentation for now, and fix it later?
And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where ever
'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you may need
to store 'dev->data' in rxq struct for this.
But,
I think it is also fair to question the assumption telemetry has that
'rx_mbuf_alloc_fail' is always available data, and consider moving it to
the 'eth_dev_handle_port_stats()' handler.
+Bruce for comment.
On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
>
> Comment moved down, please don't top post, it makes very hard to follow
> discussion.
>
> >> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
> >> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>
> >> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
> >>>
> >>>
> >>>> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
> >>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
> >>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>
> >>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> >>>>> This patch add hardware packets/bytes statistics.
> >>>>>
> >>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
> >>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
> >>>>> (struct idpf_vport *)dev->data->dev_private; + struct
> >>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
> >>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
> >>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads & +
> >>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> >>>> 0 :
> >>>>> + RTE_ETHER_CRC_LEN; + +
> >>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
> >>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
> >>>>> pstats->rx_broadcast - pstats->rx_discards; +
> >>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> >>>> +
> >>>>> + pstats->tx_unicast;
> >>>>> + stats->imissed = pstats->rx_discards; +
> >>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
> >>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
> >>>>> stats->ipackets * crc_stats_len; + stats->obytes =
> >>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
> >>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
> >>>>
> >>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
> >>>> updating here only in stats_get() will make it wrong for telemetry.
> >>>>
> >>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
> >>>> alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> >>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure
> >>> provided
> >> to user, user need to access through rte_ethdev APIs.
> >>> Because we already put rx and tx burst func to common/idpf which has
> >>> no
> >> dependcy with ethdev lib. If I update
> >> "dev->data->rx_mbuf_alloc_failed"
> >>> when allocate mbuf fails, it will break the design of our common/idpf
> >> interface to net/cpfl or net.idpf.
> >>>
> >>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
> >>> in lib
> >> code.
> >>>
> >>
> >> Please check 'eth_dev_handle_port_info()' function. As I said this is
> >> used by telemetry, not directly exposed to the user.
> >>
> >> I got the design concern, perhaps you can put a brief limitation to
> >> the driver documentation.
> >>
> > OK, got it.
> >
> > As our previous design did have flaws. And if we don't want to affect
> > correctness of telemetry, we have to redesign the idpf common module
> > code, which means a lot of work to do, so can we lower the priority of
> > this issue?
> >
> I don't believe this is urgent, can you but a one line limitation to the
> documentation for now, and fix it later?
>
> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where ever
> 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you may need
> to store 'dev->data' in rxq struct for this.
>
> But, I think it is also fair to question the assumption telemetry has
> that 'rx_mbuf_alloc_fail' is always available data, and consider moving
> it to the 'eth_dev_handle_port_stats()' handler. +Bruce for comment.
>
That's not really a telemetry assumption, it's one from the stats,
structure. Telemetry just outputs the contents of data reported by ethdev
stats, and rx_nombuf is just one of those fields.
/Bruce
On 2/28/2023 12:12 PM, Bruce Richardson wrote:
> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
>>
>> Comment moved down, please don't top post, it makes very hard to follow
>> discussion.
>>
>>>> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>
>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>>>>>
>>>>>
>>>>>> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>
>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>>>>>> This patch add hardware packets/bytes statistics.
>>>>>>>
>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads & +
>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>>>>>> 0 :
>>>>>>> + RTE_ETHER_CRC_LEN; + +
>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>>>>>> +
>>>>>>> + pstats->tx_unicast;
>>>>>>> + stats->imissed = pstats->rx_discards; +
>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>>>>>
>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
>>>>>> updating here only in stats_get() will make it wrong for telemetry.
>>>>>>
>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
>>>>>> alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure
>>>>> provided
>>>> to user, user need to access through rte_ethdev APIs.
>>>>> Because we already put rx and tx burst func to common/idpf which has
>>>>> no
>>>> dependcy with ethdev lib. If I update
>>>> "dev->data->rx_mbuf_alloc_failed"
>>>>> when allocate mbuf fails, it will break the design of our common/idpf
>>>> interface to net/cpfl or net.idpf.
>>>>>
>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
>>>>> in lib
>>>> code.
>>>>>
>>>>
>>>> Please check 'eth_dev_handle_port_info()' function. As I said this is
>>>> used by telemetry, not directly exposed to the user.
>>>>
>>>> I got the design concern, perhaps you can put a brief limitation to
>>>> the driver documentation.
>>>>
>>> OK, got it.
>>>
>>> As our previous design did have flaws. And if we don't want to affect
>>> correctness of telemetry, we have to redesign the idpf common module
>>> code, which means a lot of work to do, so can we lower the priority of
>>> this issue?
>>>
>> I don't believe this is urgent, can you but a one line limitation to the
>> documentation for now, and fix it later?
>>
>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where ever
>> 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you may need
>> to store 'dev->data' in rxq struct for this.
>>
>> But, I think it is also fair to question the assumption telemetry has
>> that 'rx_mbuf_alloc_fail' is always available data, and consider moving
>> it to the 'eth_dev_handle_port_stats()' handler. +Bruce for comment.
>>
>
> That's not really a telemetry assumption, it's one from the stats,
> structure. Telemetry just outputs the contents of data reported by ethdev
> stats, and rx_nombuf is just one of those fields.
>
Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()',
but talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
value, specially when 'rx_nombuf' is available?
Because at least for this driver returned 'rx_mbuf_alloc_fail' value
will be wrong, I believe that is same for 'idpf' driver.
On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
> On 2/28/2023 12:12 PM, Bruce Richardson wrote:
>> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
>>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
>>>
>>> Comment moved down, please don't top post, it makes very hard to follow
>>> discussion.
>>>
>>>>> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>
>>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>>>>>>
>>>>>>
>>>>>>> -----Original Message----- From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>>
>>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>>>>>>> This patch add hardware packets/bytes statistics.
>>>>>>>>
>>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>>>>>>
>>>>>>> <...>
>>>>>>>
>>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
>>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
>>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
>>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
>>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
>>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads & +
>>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>>>>>>> 0 :
>>>>>>>> + RTE_ETHER_CRC_LEN; + +
>>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
>>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
>>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
>>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>>>>>>> +
>>>>>>>> + pstats->tx_unicast;
>>>>>>>> + stats->imissed = pstats->rx_discards; +
>>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
>>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
>>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
>>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
>>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>>>>>>
>>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
>>>>>>> updating here only in stats_get() will make it wrong for telemetry.
>>>>>>>
>>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed' whenever
>>>>>>> alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
>>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public structure
>>>>>> provided
>>>>> to user, user need to access through rte_ethdev APIs.
>>>>>> Because we already put rx and tx burst func to common/idpf which has
>>>>>> no
>>>>> dependcy with ethdev lib. If I update
>>>>> "dev->data->rx_mbuf_alloc_failed"
>>>>>> when allocate mbuf fails, it will break the design of our common/idpf
>>>>> interface to net/cpfl or net.idpf.
>>>>>>
>>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
>>>>>> in lib
>>>>> code.
>>>>>>
>>>>>
>>>>> Please check 'eth_dev_handle_port_info()' function. As I said this is
>>>>> used by telemetry, not directly exposed to the user.
>>>>>
>>>>> I got the design concern, perhaps you can put a brief limitation to
>>>>> the driver documentation.
>>>>>
>>>> OK, got it.
>>>>
>>>> As our previous design did have flaws. And if we don't want to affect
>>>> correctness of telemetry, we have to redesign the idpf common module
>>>> code, which means a lot of work to do, so can we lower the priority of
>>>> this issue?
>>>>
>>> I don't believe this is urgent, can you but a one line limitation to the
>>> documentation for now, and fix it later?
>>>
>>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where ever
>>> 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you may need
>>> to store 'dev->data' in rxq struct for this.
>>>
>>> But, I think it is also fair to question the assumption telemetry has
>>> that 'rx_mbuf_alloc_fail' is always available data, and consider moving
>>> it to the 'eth_dev_handle_port_stats()' handler. +Bruce for comment.
>>>
>>
>> That's not really a telemetry assumption, it's one from the stats,
>> structure. Telemetry just outputs the contents of data reported by ethdev
>> stats, and rx_nombuf is just one of those fields.
>>
>
> Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()',
> but talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
>
> should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
> value, specially when 'rx_nombuf' is available?
>
> Because at least for this driver returned 'rx_mbuf_alloc_fail' value
> will be wrong, I believe that is same for 'idpf' driver.
>
>
Or, let me rephrase like this,
'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user directly
via ethdev APIs, but it is via telemetry.
I think it is not guaranteed that this value will be correct at any
given time as telemetry assumes, so should we remove it from telemetry?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, February 28, 2023 8:33 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>
> On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
> > On 2/28/2023 12:12 PM, Bruce Richardson wrote:
> >> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
> >>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
> >>>
> >>> Comment moved down, please don't top post, it makes very hard to
> >>> follow discussion.
> >>>
> >>>>> -----Original Message----- From: Ferruh Yigit
> >>>>> <ferruh.yigit@amd.com>
> >>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
> >>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>
> >>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message----- From: Ferruh Yigit
> >>>>>>> <ferruh.yigit@amd.com>
> >>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
> >>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>>>
> >>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> >>>>>>>> This patch add hardware packets/bytes statistics.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> >>>>>>>
> >>>>>>> <...>
> >>>>>>>
> >>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
> >>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
> >>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
> >>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
> >>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
> >>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads &
> >>>>>>>> +
> >>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> >>>>>>> 0 :
> >>>>>>>> + RTE_ETHER_CRC_LEN; + +
> >>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
> >>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
> >>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
> >>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> >>>>>>> +
> >>>>>>>> + pstats->tx_unicast;
> >>>>>>>> + stats->imissed = pstats->rx_discards; +
> >>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
> >>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
> >>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
> >>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
> >>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
> >>>>>>>
> >>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
> >>>>>>> updating here only in stats_get() will make it wrong for telemetry.
> >>>>>>>
> >>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed'
> >>>>>>> whenever alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> >>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public
> >>>>>> structure provided
> >>>>> to user, user need to access through rte_ethdev APIs.
> >>>>>> Because we already put rx and tx burst func to common/idpf which
> >>>>>> has no
> >>>>> dependcy with ethdev lib. If I update
> >>>>> "dev->data->rx_mbuf_alloc_failed"
> >>>>>> when allocate mbuf fails, it will break the design of our
> >>>>>> common/idpf
> >>>>> interface to net/cpfl or net.idpf.
> >>>>>>
> >>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
> >>>>>> in lib
> >>>>> code.
> >>>>>>
> >>>>>
> >>>>> Please check 'eth_dev_handle_port_info()' function. As I said
> >>>>> this is used by telemetry, not directly exposed to the user.
> >>>>>
> >>>>> I got the design concern, perhaps you can put a brief limitation
> >>>>> to the driver documentation.
> >>>>>
> >>>> OK, got it.
> >>>>
> >>>> As our previous design did have flaws. And if we don't want to
> >>>> affect correctness of telemetry, we have to redesign the idpf
> >>>> common module code, which means a lot of work to do, so can we
> >>>> lower the priority of this issue?
> >>>>
> >>> I don't believe this is urgent, can you but a one line limitation to
> >>> the documentation for now, and fix it later?
> >>>
> >>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where
> >>> ever 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you
> >>> may need to store 'dev->data' in rxq struct for this.
> >>>
> >>> But, I think it is also fair to question the assumption telemetry
> >>> has that 'rx_mbuf_alloc_fail' is always available data, and consider
> >>> moving it to the 'eth_dev_handle_port_stats()' handler. +Bruce for
> comment.
> >>>
> >>
> >> That's not really a telemetry assumption, it's one from the stats,
> >> structure. Telemetry just outputs the contents of data reported by
> >> ethdev stats, and rx_nombuf is just one of those fields.
> >>
> >
> > Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()', but
> > talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
> >
> > should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
> > value, specially when 'rx_nombuf' is available?
> >
> > Because at least for this driver returned 'rx_mbuf_alloc_fail' value
> > will be wrong, I believe that is same for 'idpf' driver.
> >
> >
>
> Or, let me rephrase like this,
> 'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user directly via
> ethdev APIs, but it is via telemetry.
>
> I think it is not guaranteed that this value will be correct at any given time as
> telemetry assumes, so should we remove it from telemetry?
May not be necessary, PMD should be able to give the right number, this is something we can fix in idpf and cpfl PMD, to align with other PMD.
On 2/28/2023 1:29 PM, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, February 28, 2023 8:33 PM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
>> Jingjing <jingjing.wu@intel.com>
>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>
>> On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
>>> On 2/28/2023 12:12 PM, Bruce Richardson wrote:
>>>> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
>>>>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
>>>>>
>>>>> Comment moved down, please don't top post, it makes very hard to
>>>>> follow discussion.
>>>>>
>>>>>>> -----Original Message----- From: Ferruh Yigit
>>>>>>> <ferruh.yigit@amd.com>
>>>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>>
>>>>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message----- From: Ferruh Yigit
>>>>>>>>> <ferruh.yigit@amd.com>
>>>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
>>>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>>>>
>>>>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>>>>>>>>> This patch add hardware packets/bytes statistics.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>>>>>>>>
>>>>>>>>> <...>
>>>>>>>>>
>>>>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
>>>>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
>>>>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
>>>>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
>>>>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
>>>>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads &
>>>>>>>>>> +
>>>>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>>>>>>>>> 0 :
>>>>>>>>>> + RTE_ETHER_CRC_LEN; + +
>>>>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
>>>>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
>>>>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
>>>>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>>>>>>>>> +
>>>>>>>>>> + pstats->tx_unicast;
>>>>>>>>>> + stats->imissed = pstats->rx_discards; +
>>>>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
>>>>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
>>>>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
>>>>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
>>>>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>>>>>>>>
>>>>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
>>>>>>>>> updating here only in stats_get() will make it wrong for telemetry.
>>>>>>>>>
>>>>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed'
>>>>>>>>> whenever alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
>>>>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public
>>>>>>>> structure provided
>>>>>>> to user, user need to access through rte_ethdev APIs.
>>>>>>>> Because we already put rx and tx burst func to common/idpf which
>>>>>>>> has no
>>>>>>> dependcy with ethdev lib. If I update
>>>>>>> "dev->data->rx_mbuf_alloc_failed"
>>>>>>>> when allocate mbuf fails, it will break the design of our
>>>>>>>> common/idpf
>>>>>>> interface to net/cpfl or net.idpf.
>>>>>>>>
>>>>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
>>>>>>>> in lib
>>>>>>> code.
>>>>>>>>
>>>>>>>
>>>>>>> Please check 'eth_dev_handle_port_info()' function. As I said
>>>>>>> this is used by telemetry, not directly exposed to the user.
>>>>>>>
>>>>>>> I got the design concern, perhaps you can put a brief limitation
>>>>>>> to the driver documentation.
>>>>>>>
>>>>>> OK, got it.
>>>>>>
>>>>>> As our previous design did have flaws. And if we don't want to
>>>>>> affect correctness of telemetry, we have to redesign the idpf
>>>>>> common module code, which means a lot of work to do, so can we
>>>>>> lower the priority of this issue?
>>>>>>
>>>>> I don't believe this is urgent, can you but a one line limitation to
>>>>> the documentation for now, and fix it later?
>>>>>
>>>>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where
>>>>> ever 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you
>>>>> may need to store 'dev->data' in rxq struct for this.
>>>>>
>>>>> But, I think it is also fair to question the assumption telemetry
>>>>> has that 'rx_mbuf_alloc_fail' is always available data, and consider
>>>>> moving it to the 'eth_dev_handle_port_stats()' handler. +Bruce for
>> comment.
>>>>>
>>>>
>>>> That's not really a telemetry assumption, it's one from the stats,
>>>> structure. Telemetry just outputs the contents of data reported by
>>>> ethdev stats, and rx_nombuf is just one of those fields.
>>>>
>>>
>>> Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()', but
>>> talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
>>>
>>> should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
>>> value, specially when 'rx_nombuf' is available?
>>>
>>> Because at least for this driver returned 'rx_mbuf_alloc_fail' value
>>> will be wrong, I believe that is same for 'idpf' driver.
>>>
>>>
>>
>> Or, let me rephrase like this,
>> 'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user directly via
>> ethdev APIs, but it is via telemetry.
>>
>> I think it is not guaranteed that this value will be correct at any given time as
>> telemetry assumes, so should we remove it from telemetry?
>
> May not be necessary, PMD should be able to give the right number, this is something we can fix in idpf and cpfl PMD, to align with other PMD.
Thanks Qi, Ok to have drivers aligned to common usage.
Still, for telemetry we can consider removing 'rx_mbuf_alloc_fail', user
can get that information from 'rx_nombuf'.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, February 28, 2023 9:35 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>
> On 2/28/2023 1:29 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Tuesday, February 28, 2023 8:33 PM
> >> To: Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
> >> Jingjing <jingjing.wu@intel.com>
> >> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>
> >> On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
> >>> On 2/28/2023 12:12 PM, Bruce Richardson wrote:
> >>>> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
> >>>>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
> >>>>>
> >>>>> Comment moved down, please don't top post, it makes very hard to
> >>>>> follow discussion.
> >>>>>
> >>>>>>> -----Original Message----- From: Ferruh Yigit
> >>>>>>> <ferruh.yigit@amd.com>
> >>>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
> >>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>>>
> >>>>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message----- From: Ferruh Yigit
> >>>>>>>>> <ferruh.yigit@amd.com>
> >>>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
> >>>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying
> >>>>>>>>> <yuying.zhang@intel.com>
> >>>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>>>>>
> >>>>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> >>>>>>>>>> This patch add hardware packets/bytes statistics.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> >>>>>>>>>
> >>>>>>>>> <...>
> >>>>>>>>>
> >>>>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev,
> >>>>>>>>>> +struct
> >>>>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
> >>>>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
> >>>>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
> >>>>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
> >>>>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads
> >>>>>>>>>> &
> >>>>>>>>>> +
> >>>>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> >>>>>>>>> 0 :
> >>>>>>>>>> + RTE_ETHER_CRC_LEN; + +
> >>>>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
> >>>>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> >>>>>>>>>> stats->+
> >>>>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
> >>>>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> >>>>>>>>> +
> >>>>>>>>>> + pstats->tx_unicast;
> >>>>>>>>>> + stats->imissed = pstats->rx_discards; +
> >>>>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
> >>>>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
> >>>>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
> >>>>>>>>>> pstats->tx_bytes; + + dev->data-
> >rx_mbuf_alloc_failed =
> >>>>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
> >>>>>>>>>
> >>>>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
> >>>>>>>>> updating here only in stats_get() will make it wrong for telemetry.
> >>>>>>>>>
> >>>>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed'
> >>>>>>>>> whenever alloc failed? (alongside 'rxq-
> >rx_stats.mbuf_alloc_failed').
> >>>>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public
> >>>>>>>> structure provided
> >>>>>>> to user, user need to access through rte_ethdev APIs.
> >>>>>>>> Because we already put rx and tx burst func to common/idpf
> >>>>>>>> which has no
> >>>>>>> dependcy with ethdev lib. If I update
> >>>>>>> "dev->data->rx_mbuf_alloc_failed"
> >>>>>>>> when allocate mbuf fails, it will break the design of our
> >>>>>>>> common/idpf
> >>>>>>> interface to net/cpfl or net.idpf.
> >>>>>>>>
> >>>>>>>> And I didn't find any reference of 'dev->data-
> >rx_mbuf_alloc_failed'
> >>>>>>>> in lib
> >>>>>>> code.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Please check 'eth_dev_handle_port_info()' function. As I said
> >>>>>>> this is used by telemetry, not directly exposed to the user.
> >>>>>>>
> >>>>>>> I got the design concern, perhaps you can put a brief limitation
> >>>>>>> to the driver documentation.
> >>>>>>>
> >>>>>> OK, got it.
> >>>>>>
> >>>>>> As our previous design did have flaws. And if we don't want to
> >>>>>> affect correctness of telemetry, we have to redesign the idpf
> >>>>>> common module code, which means a lot of work to do, so can we
> >>>>>> lower the priority of this issue?
> >>>>>>
> >>>>> I don't believe this is urgent, can you but a one line limitation
> >>>>> to the documentation for now, and fix it later?
> >>>>>
> >>>>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where
> >>>>> ever 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although
> >>>>> you may need to store 'dev->data' in rxq struct for this.
> >>>>>
> >>>>> But, I think it is also fair to question the assumption telemetry
> >>>>> has that 'rx_mbuf_alloc_fail' is always available data, and
> >>>>> consider moving it to the 'eth_dev_handle_port_stats()' handler.
> >>>>> +Bruce for
> >> comment.
> >>>>>
> >>>>
> >>>> That's not really a telemetry assumption, it's one from the stats,
> >>>> structure. Telemetry just outputs the contents of data reported by
> >>>> ethdev stats, and rx_nombuf is just one of those fields.
> >>>>
> >>>
> >>> Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()', but
> >>> talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
> >>>
> >>> should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
> >>> value, specially when 'rx_nombuf' is available?
> >>>
> >>> Because at least for this driver returned 'rx_mbuf_alloc_fail' value
> >>> will be wrong, I believe that is same for 'idpf' driver.
> >>>
> >>>
> >>
> >> Or, let me rephrase like this,
> >> 'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user
> >> directly via ethdev APIs, but it is via telemetry.
> >>
> >> I think it is not guaranteed that this value will be correct at any
> >> given time as telemetry assumes, so should we remove it from telemetry?
> >
> > May not be necessary, PMD should be able to give the right number, this is
> something we can fix in idpf and cpfl PMD, to align with other PMD.
>
> Thanks Qi, Ok to have drivers aligned to common usage.
>
> Still, for telemetry we can consider removing 'rx_mbuf_alloc_fail', user can
> get that information from 'rx_nombuf'.
No objection, if this is not a comprise for above issue :)
On Tue, Feb 28, 2023 at 01:34:43PM +0000, Ferruh Yigit wrote:
> On 2/28/2023 1:29 PM, Zhang, Qi Z wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Tuesday, February 28, 2023 8:33 PM
> >> To: Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
> >> Jingjing <jingjing.wu@intel.com>
> >> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>
> >> On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
> >>> On 2/28/2023 12:12 PM, Bruce Richardson wrote:
> >>>> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
> >>>>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
> >>>>>
> >>>>> Comment moved down, please don't top post, it makes very hard to
> >>>>> follow discussion.
> >>>>>
> >>>>>>> -----Original Message----- From: Ferruh Yigit
> >>>>>>> <ferruh.yigit@amd.com>
> >>>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
> >>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>>>
> >>>>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message----- From: Ferruh Yigit
> >>>>>>>>> <ferruh.yigit@amd.com>
> >>>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
> >>>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> >>>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
> >>>>>>>>>
> >>>>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
> >>>>>>>>>> This patch add hardware packets/bytes statistics.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> >>>>>>>>>
> >>>>>>>>> <...>
> >>>>>>>>>
> >>>>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
> >>>>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
> >>>>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
> >>>>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
> >>>>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
> >>>>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads &
> >>>>>>>>>> +
> >>>>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
> >>>>>>>>> 0 :
> >>>>>>>>>> + RTE_ETHER_CRC_LEN; + +
> >>>>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
> >>>>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
> >>>>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
> >>>>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
> >>>>>>>>> +
> >>>>>>>>>> + pstats->tx_unicast;
> >>>>>>>>>> + stats->imissed = pstats->rx_discards; +
> >>>>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
> >>>>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
> >>>>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
> >>>>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
> >>>>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
> >>>>>>>>>
> >>>>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
> >>>>>>>>> updating here only in stats_get() will make it wrong for telemetry.
> >>>>>>>>>
> >>>>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed'
> >>>>>>>>> whenever alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
> >>>>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public
> >>>>>>>> structure provided
> >>>>>>> to user, user need to access through rte_ethdev APIs.
> >>>>>>>> Because we already put rx and tx burst func to common/idpf which
> >>>>>>>> has no
> >>>>>>> dependcy with ethdev lib. If I update
> >>>>>>> "dev->data->rx_mbuf_alloc_failed"
> >>>>>>>> when allocate mbuf fails, it will break the design of our
> >>>>>>>> common/idpf
> >>>>>>> interface to net/cpfl or net.idpf.
> >>>>>>>>
> >>>>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
> >>>>>>>> in lib
> >>>>>>> code.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Please check 'eth_dev_handle_port_info()' function. As I said
> >>>>>>> this is used by telemetry, not directly exposed to the user.
> >>>>>>>
> >>>>>>> I got the design concern, perhaps you can put a brief limitation
> >>>>>>> to the driver documentation.
> >>>>>>>
> >>>>>> OK, got it.
> >>>>>>
> >>>>>> As our previous design did have flaws. And if we don't want to
> >>>>>> affect correctness of telemetry, we have to redesign the idpf
> >>>>>> common module code, which means a lot of work to do, so can we
> >>>>>> lower the priority of this issue?
> >>>>>>
> >>>>> I don't believe this is urgent, can you but a one line limitation to
> >>>>> the documentation for now, and fix it later?
> >>>>>
> >>>>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where
> >>>>> ever 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you
> >>>>> may need to store 'dev->data' in rxq struct for this.
> >>>>>
> >>>>> But, I think it is also fair to question the assumption telemetry
> >>>>> has that 'rx_mbuf_alloc_fail' is always available data, and consider
> >>>>> moving it to the 'eth_dev_handle_port_stats()' handler. +Bruce for
> >> comment.
> >>>>>
> >>>>
> >>>> That's not really a telemetry assumption, it's one from the stats,
> >>>> structure. Telemetry just outputs the contents of data reported by
> >>>> ethdev stats, and rx_nombuf is just one of those fields.
> >>>>
> >>>
> >>> Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()', but
> >>> talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
> >>>
> >>> should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
> >>> value, specially when 'rx_nombuf' is available?
> >>>
> >>> Because at least for this driver returned 'rx_mbuf_alloc_fail' value
> >>> will be wrong, I believe that is same for 'idpf' driver.
> >>>
> >>>
Thanks for the clarification, the question is clearer now. Having duplicate
info seems strange.
> >>
> >> Or, let me rephrase like this,
> >> 'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user directly via
> >> ethdev APIs, but it is via telemetry.
> >>
> >> I think it is not guaranteed that this value will be correct at any given time as
> >> telemetry assumes, so should we remove it from telemetry?
> >
> > May not be necessary, PMD should be able to give the right number, this is something we can fix in idpf and cpfl PMD, to align with other PMD.
>
> Thanks Qi, Ok to have drivers aligned to common usage.
>
> Still, for telemetry we can consider removing 'rx_mbuf_alloc_fail', user
> can get that information from 'rx_nombuf'.
I would agree with Ferruh. The information on nombufs should be available
just from the stats. It doesn't logically fit in the "info" category,
especially when it is in stats already.
/Bruce
On 2/28/2023 2:24 PM, Bruce Richardson wrote:
> On Tue, Feb 28, 2023 at 01:34:43PM +0000, Ferruh Yigit wrote:
>> On 2/28/2023 1:29 PM, Zhang, Qi Z wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Tuesday, February 28, 2023 8:33 PM
>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Liu, Mingxia <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>; Wu,
>>>> Jingjing <jingjing.wu@intel.com>
>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>
>>>> On 2/28/2023 12:24 PM, Ferruh Yigit wrote:
>>>>> On 2/28/2023 12:12 PM, Bruce Richardson wrote:
>>>>>> On Tue, Feb 28, 2023 at 12:04:53PM +0000, Ferruh Yigit wrote:
>>>>>>> On 2/28/2023 11:47 AM, Liu, Mingxia wrote:
>>>>>>>
>>>>>>> Comment moved down, please don't top post, it makes very hard to
>>>>>>> follow discussion.
>>>>>>>
>>>>>>>>> -----Original Message----- From: Ferruh Yigit
>>>>>>>>> <ferruh.yigit@amd.com>
>>>>>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
>>>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>>>>
>>>>>>>>> On 2/28/2023 6:46 AM, Liu, Mingxia wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> -----Original Message----- From: Ferruh Yigit
>>>>>>>>>>> <ferruh.yigit@amd.com>
>>>>>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
>>>>>>>>>>> <mingxia.liu@intel.com>; dev@dpdk.org; Xing, Beilei
>>>>>>>>>>> <beilei.xing@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
>>>>>>>>>>> Subject: Re: [PATCH v7 18/21] net/cpfl: add HW statistics
>>>>>>>>>>>
>>>>>>>>>>> On 2/16/2023 12:30 AM, Mingxia Liu wrote:
>>>>>>>>>>>> This patch add hardware packets/bytes statistics.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
>>>>>>>>>>>
>>>>>>>>>>> <...>
>>>>>>>>>>>
>>>>>>>>>>>> +static int +cpfl_dev_stats_get(struct rte_eth_dev *dev, struct
>>>>>>>>>>>> rte_eth_stats +*stats) { + struct idpf_vport *vport = +
>>>>>>>>>>>> (struct idpf_vport *)dev->data->dev_private; + struct
>>>>>>>>>>>> virtchnl2_vport_stats *pstats = NULL; + int ret; + + ret =
>>>>>>>>>>>> idpf_vc_stats_query(vport, &pstats); + if (ret == 0) { +
>>>>>>>>>>>> uint8_t crc_stats_len = (dev->data- dev_conf.rxmode.offloads &
>>>>>>>>>>>> +
>>>>>>>>>>>> RTE_ETH_RX_OFFLOAD_KEEP_CRC) ?
>>>>>>>>>>> 0 :
>>>>>>>>>>>> + RTE_ETHER_CRC_LEN; + +
>>>>>>>>>>>> idpf_vport_stats_update(&vport->eth_stats_offset, pstats); +
>>>>>>>>>>>> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast + +
>>>>>>>>>>>> pstats->rx_broadcast - pstats->rx_discards; +
>>>>>>>>>>>> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast
>>>>>>>>>>> +
>>>>>>>>>>>> + pstats->tx_unicast;
>>>>>>>>>>>> + stats->imissed = pstats->rx_discards; +
>>>>>>>>>>>> stats->oerrors = pstats->tx_errors + pstats->tx_discards; +
>>>>>>>>>>>> stats->ibytes = pstats->rx_bytes; + stats->ibytes -=
>>>>>>>>>>>> stats->ipackets * crc_stats_len; + stats->obytes =
>>>>>>>>>>>> pstats->tx_bytes; + + dev->data->rx_mbuf_alloc_failed =
>>>>>>>>>>>> +cpfl_get_mbuf_alloc_failed_stats(dev);
>>>>>>>>>>>
>>>>>>>>>>> 'dev->data->rx_mbuf_alloc_failed' is also used by telemetry,
>>>>>>>>>>> updating here only in stats_get() will make it wrong for telemetry.
>>>>>>>>>>>
>>>>>>>>>>> Is it possible to update 'dev->data->rx_mbuf_alloc_failed'
>>>>>>>>>>> whenever alloc failed? (alongside 'rxq->rx_stats.mbuf_alloc_failed').
>>>>>>>>>> [Liu, Mingxia] As I know, rte_eth_dev_data is not a public
>>>>>>>>>> structure provided
>>>>>>>>> to user, user need to access through rte_ethdev APIs.
>>>>>>>>>> Because we already put rx and tx burst func to common/idpf which
>>>>>>>>>> has no
>>>>>>>>> dependcy with ethdev lib. If I update
>>>>>>>>> "dev->data->rx_mbuf_alloc_failed"
>>>>>>>>>> when allocate mbuf fails, it will break the design of our
>>>>>>>>>> common/idpf
>>>>>>>>> interface to net/cpfl or net.idpf.
>>>>>>>>>>
>>>>>>>>>> And I didn't find any reference of 'dev->data->rx_mbuf_alloc_failed'
>>>>>>>>>> in lib
>>>>>>>>> code.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please check 'eth_dev_handle_port_info()' function. As I said
>>>>>>>>> this is used by telemetry, not directly exposed to the user.
>>>>>>>>>
>>>>>>>>> I got the design concern, perhaps you can put a brief limitation
>>>>>>>>> to the driver documentation.
>>>>>>>>>
>>>>>>>> OK, got it.
>>>>>>>>
>>>>>>>> As our previous design did have flaws. And if we don't want to
>>>>>>>> affect correctness of telemetry, we have to redesign the idpf
>>>>>>>> common module code, which means a lot of work to do, so can we
>>>>>>>> lower the priority of this issue?
>>>>>>>>
>>>>>>> I don't believe this is urgent, can you but a one line limitation to
>>>>>>> the documentation for now, and fix it later?
>>>>>>>
>>>>>>> And for the fix, updating 'dev->data->rx_mbuf_alloc_failed' where
>>>>>>> ever 'rxq->rx_stats.mbuf_alloc_failed' updated is easy, although you
>>>>>>> may need to store 'dev->data' in rxq struct for this.
>>>>>>>
>>>>>>> But, I think it is also fair to question the assumption telemetry
>>>>>>> has that 'rx_mbuf_alloc_fail' is always available data, and consider
>>>>>>> moving it to the 'eth_dev_handle_port_stats()' handler. +Bruce for
>>>> comment.
>>>>>>>
>>>>>>
>>>>>> That's not really a telemetry assumption, it's one from the stats,
>>>>>> structure. Telemetry just outputs the contents of data reported by
>>>>>> ethdev stats, and rx_nombuf is just one of those fields.
>>>>>>
>>>>>
>>>>> Not talking about 'rx_nombuf' in 'eth_dev_handle_port_stats()', but
>>>>> talking about 'rx_mbuf_alloc_fail' in 'eth_dev_handle_port_info()',
>>>>>
>>>>> should telemetry return interim 'eth_dev->data->rx_mbuf_alloc_failed'
>>>>> value, specially when 'rx_nombuf' is available?
>>>>>
>>>>> Because at least for this driver returned 'rx_mbuf_alloc_fail' value
>>>>> will be wrong, I believe that is same for 'idpf' driver.
>>>>>
>>>>>
>
> Thanks for the clarification, the question is clearer now. Having duplicate
> info seems strange.
>
>>>>
>>>> Or, let me rephrase like this,
>>>> 'eth_dev->data->rx_mbuf_alloc_failed' is not returned to user directly via
>>>> ethdev APIs, but it is via telemetry.
>>>>
>>>> I think it is not guaranteed that this value will be correct at any given time as
>>>> telemetry assumes, so should we remove it from telemetry?
>>>
>>> May not be necessary, PMD should be able to give the right number, this is something we can fix in idpf and cpfl PMD, to align with other PMD.
>>
>> Thanks Qi, Ok to have drivers aligned to common usage.
>>
>> Still, for telemetry we can consider removing 'rx_mbuf_alloc_fail', user
>> can get that information from 'rx_nombuf'.
>
> I would agree with Ferruh. The information on nombufs should be available
> just from the stats. It doesn't logically fit in the "info" category,
> especially when it is in stats already.
>
Thanks Bruce.
So, no need to update driver(s) related to 'rx_mbuf_alloc_fail',
existing patch is good.
I will send ethdev telemetry change later, it is a minor change.
@@ -178,6 +178,87 @@ cpfl_dev_supported_ptypes_get(struct rte_eth_dev *dev __rte_unused)
return ptypes;
}
+static uint64_t
+cpfl_get_mbuf_alloc_failed_stats(struct rte_eth_dev *dev)
+{
+ uint64_t mbuf_alloc_failed = 0;
+ struct idpf_rx_queue *rxq;
+ int i = 0;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ rxq = dev->data->rx_queues[i];
+ mbuf_alloc_failed += __atomic_load_n(&rxq->rx_stats.mbuf_alloc_failed,
+ __ATOMIC_RELAXED);
+ }
+
+ return mbuf_alloc_failed;
+}
+
+static int
+cpfl_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
+{
+ struct idpf_vport *vport =
+ (struct idpf_vport *)dev->data->dev_private;
+ struct virtchnl2_vport_stats *pstats = NULL;
+ int ret;
+
+ ret = idpf_vc_stats_query(vport, &pstats);
+ if (ret == 0) {
+ uint8_t crc_stats_len = (dev->data->dev_conf.rxmode.offloads &
+ RTE_ETH_RX_OFFLOAD_KEEP_CRC) ? 0 :
+ RTE_ETHER_CRC_LEN;
+
+ idpf_vport_stats_update(&vport->eth_stats_offset, pstats);
+ stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
+ pstats->rx_broadcast - pstats->rx_discards;
+ stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
+ pstats->tx_unicast;
+ stats->imissed = pstats->rx_discards;
+ stats->oerrors = pstats->tx_errors + pstats->tx_discards;
+ stats->ibytes = pstats->rx_bytes;
+ stats->ibytes -= stats->ipackets * crc_stats_len;
+ stats->obytes = pstats->tx_bytes;
+
+ dev->data->rx_mbuf_alloc_failed = cpfl_get_mbuf_alloc_failed_stats(dev);
+ stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
+ } else {
+ PMD_DRV_LOG(ERR, "Get statistics failed");
+ }
+ return ret;
+}
+
+static void
+cpfl_reset_mbuf_alloc_failed_stats(struct rte_eth_dev *dev)
+{
+ struct idpf_rx_queue *rxq;
+ int i;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ rxq = dev->data->rx_queues[i];
+ __atomic_store_n(&rxq->rx_stats.mbuf_alloc_failed, 0, __ATOMIC_RELAXED);
+ }
+}
+
+static int
+cpfl_dev_stats_reset(struct rte_eth_dev *dev)
+{
+ struct idpf_vport *vport =
+ (struct idpf_vport *)dev->data->dev_private;
+ struct virtchnl2_vport_stats *pstats = NULL;
+ int ret;
+
+ ret = idpf_vc_stats_query(vport, &pstats);
+ if (ret != 0)
+ return ret;
+
+ /* set stats offset base on current values */
+ vport->eth_stats_offset = *pstats;
+
+ cpfl_reset_mbuf_alloc_failed_stats(dev);
+
+ return 0;
+}
+
static int
cpfl_init_rss(struct idpf_vport *vport)
{
@@ -365,6 +446,9 @@ cpfl_dev_start(struct rte_eth_dev *dev)
goto err_vport;
}
+ if (cpfl_dev_stats_reset(dev))
+ PMD_DRV_LOG(ERR, "Failed to reset stats");
+
vport->stopped = 0;
return 0;
@@ -766,6 +850,8 @@ static const struct eth_dev_ops cpfl_eth_dev_ops = {
.tx_queue_release = cpfl_dev_tx_queue_release,
.mtu_set = cpfl_dev_mtu_set,
.dev_supported_ptypes_get = cpfl_dev_supported_ptypes_get,
+ .stats_get = cpfl_dev_stats_get,
+ .stats_reset = cpfl_dev_stats_reset,
};
static uint16_t