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.yi...@amd.com>
> >> Sent: Tuesday, February 28, 2023 8:33 PM
> >> To: Richardson, Bruce <bruce.richard...@intel.com>
> >> Cc: Liu, Mingxia <mingxia....@intel.com>; dev@dpdk.org; Xing, Beilei
> >> <beilei.x...@intel.com>; Zhang, Yuying <yuying.zh...@intel.com>; Wu,
> >> Jingjing <jingjing...@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.yi...@amd.com>
> >>>>>>> Sent: Tuesday, February 28, 2023 6:02 PM To: Liu, Mingxia
> >>>>>>> <mingxia....@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>> <beilei.x...@intel.com>; Zhang, Yuying <yuying.zh...@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.yi...@amd.com>
> >>>>>>>>> Sent: Tuesday, February 28, 2023 5:52 AM To: Liu, Mingxia
> >>>>>>>>> <mingxia....@intel.com>; dev@dpdk.org; Xing, Beilei
> >>>>>>>>> <beilei.x...@intel.com>; Zhang, Yuying <yuying.zh...@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....@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
