[dpdk-dev] [PATCH] vhost: add pmd xstats

[Yang, Zhiyong]

Hi, ALL:

Physical NIC has a set of counters, such as 
u64 prc64;
u64 prc127;
u64 prc255; etc.
but now, DPDK has counted the prc64 in two ways. Physical NIC counts
prc64 with CRC by hardware. Virtio computes the counter like prc64
without CRC. This will cause the conflict, when a 64 packet from outer
network is sent to VM(virtio), NIC will show prc64 + 1, virtio will
actually receive the 64-4(CRC) = 60 bytes pkt, undersize(<64) counter
will be increased. Should Vhost do like NIC's behavior or virtio's behavior?

According to rfc2819 description as referece.
etherStatsPkts64Octets OBJECT-TYPE
SYNTAX Counter32
UNITS "Packets"
"The total number of packets (including bad packets) received that were
64 octets in length (excluding framing bits but including FCS octets)."

> -----Original Message-----
> From: Yao, Lei A
> Sent: Tuesday, August 30, 2016 11:22 AM
> To: Xu, Qian Q <qian.q.xu at intel.com>; Yang, Zhiyong
> <zhiyong.yang at intel.com>; Panu Matilainen <pmatilai at redhat.com>;
> Thomas Monjalon <thomas.monjalon at 6wind.com>; Yuanhan Liu
> <yuanhan.liu at linux.intel.com>
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] vhost: add pmd xstats
> 
> Hi, Qian
> 
> The test setup at my side is Vhost/VirtIO loopback with 64B packets.
> 
> 
> -----Original Message-----
> From: Xu, Qian Q
> Sent: Tuesday, August 30, 2016 11:03 AM
> To: Yao, Lei A <lei.a.yao at intel.com>; Yang, Zhiyong
> <zhiyong.yang at intel.com>; Panu Matilainen <pmatilai at redhat.com>;
> Thomas Monjalon <thomas.monjalon at 6wind.com>; Yuanhan Liu
> <yuanhan.liu at linux.intel.com>
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] vhost: add pmd xstats
> 
> Lei
> Could you list the test setup for below findings? I think we need at least to
> check below tests for mergeable=on/off path:
> 1. Vhost/virtio loopback
> 2. PVP test : virtio-pmd IO fwd and virtio-net IPV4 fwd
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yao, Lei A
> Sent: Tuesday, August 30, 2016 10:46 AM
> To: Yang, Zhiyong <zhiyong.yang at intel.com>; Panu Matilainen
> <pmatilai at redhat.com>; Thomas Monjalon
> <thomas.monjalon at 6wind.com>; Yuanhan Liu <yuanhan.liu at linux.intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> 
> Hi, Zhiyong
> 
> I have tested more  xstats performance drop data at my side.
> Vhost Xstats patch  with mergeable on :  ~3% Vhost Xstats patch with
> mergeable off : ~9%
> 
> Because Zhihong also submit patch to improve the performance on for the
> mergeable on: http://dpdk.org/dev/patchwork/patch/15245/      ~15249. If
> both patch integrated, the performance drop will be much higher
> Vhsot Xstats patch + Vhost mergeable on patch   with mergeable on : the
> performance drop is around  6%
> 
> 
> Best Regards
> Lei
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Yang, Zhiyong
> Sent: Thursday, August 25, 2016 5:22 PM
> To: Panu Matilainen <pmatilai at redhat.com>; Thomas Monjalon
> <thomas.monjalon at 6wind.com>; Yuanhan Liu <yuanhan.liu at linux.intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> 
> 
> 
> > -----Original Message-----
> > From: Panu Matilainen [mailto:pmatilai at redhat.com]
> > Sent: Wednesday, August 24, 2016 8:37 PM
> > To: Thomas Monjalon <thomas.monjalon at 6wind.com>; Yuanhan Liu
> > <yuanhan.liu at linux.intel.com>
> > Cc: dev at dpdk.org; Yang, Zhiyong <zhiyong.yang at intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> >
> > On 08/24/2016 11:44 AM, Thomas Monjalon wrote:
> > > 2016-08-24 13:46, Yuanhan Liu:
> > >> On Tue, Aug 23, 2016 at 12:45:54PM +0300, Panu Matilainen wrote:
> > >>>>>> Since collecting data of vhost_update_packet_xstats will have
> > >>>>>> some effect on RX/TX performance, so, Setting compiling switch
> > >>>>>> CONFIG_RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS=n by default
> > in the
> > >>>>> file
> > >>>>>> config/common_base, if needing xstats data, you can enable it(y).
> > >>>>>
> > >>>>> NAK, such things need to be switchable at run-time.
> > >>>>>
> > >>>>> 	- Panu -
> > >>>>
> > >>>> Considering the following reasons using the compiler switch, not
> > >>>> command-line at run-time.
> > >>>>
> > >>>> 1.Similar xstats update functions are always collecting stats
> > >>>> data in the background when rx/tx are running, such as the
> > >>>> physical NIC or virtio, which have no switch. Compiler switch for
> > >>>> vhost pmd xstats is added as a option when performance is viewed
> > >>>> as critical
> > factor.
> > >>>>
> > >>>> 2. No data structure and API in any layer support the xstats
> > >>>> update switch at run-time. Common data structure (struct
> > >>>> rte_eth_dev_data) has no device-specific data member, if
> > >>>> implementing enable/disable of vhost_update _packet_xstats at
> > >>>> run-time, must define a
> > >>>> flag(device-specific) in it, because the definition of struct
> > >>>> vhost_queue in the driver code (eth_vhost_rx/eth_vhost_tx
> > processing)is not visible from device perspective.
> > >>>>
> > >>>> 3. I tested RX/TX with v1 patch (y) as reference based on
> > >>>> Intel(R)
> > >>>> Xeon(R) CPU E5-2699 v3 @ 2.30GHz, for 64byts packets in burst
> > >>>> mode,
> > >>>> 32 packets in one RX/TX processing. Overhead of
> > >>>> vhost_update_packet_xstats is less than 3% for the rx/tx
> > >>>> processing. It looks that vhost_update_packet_xstats has a
> > >>>> limited
> > effect on performance drop.
> > >>>
> > >>> Well, either the performance overhead is acceptable and it should
> > >>> always be on (like with physical NICs I think). Or it is not. In
> > >>> which case it needs to be turnable on and off, at run-time.
> > >>> Rebuilding is not an option in the world of distros.
> > >>
> > >> I think the less than 3% overhead is acceptable here, that I agree
> > >> with Panu we should always keep it on. If someone compains it later
> > >> that even 3% is too big for them, let's consider to make it be
> > >> switchable at run-time. Either we could introduce a generic eth API
> > >> for that, Or just introduce a vhost one if that doesn't make too
> > >> much sense to other eth drivers.
> > >
> > > +1
> > > It may have sense to introduce a generic run-time option for stats.
> > >
> >
> > Yup, sounds good.
> >
> It sounds better , if DPDK can add generic API and structure to the switch of
> xstats update. So, any device can use it at run time if necessary.
> 
> Can we define one bit data member (xstats_update) in the data structure
> struct rte_eth_dev_data?
> such as:
> 	uint8_t promiscuous : 1, /**< RX promiscuous mode ON(1) / OFF(0).
> */
> 	scattered_rx : 1,  /**< RX of scattered packets is ON(1) / OFF(0) */
> 	all_multicast : 1, /**< RX all multicast mode ON(1) / OFF(0). */
> 	dev_started : 1,   /**< Device state: STARTED(1) / STOPPED(0). */
> 	lro         : 1,   /**< RX LRO is ON(1) / OFF(0) */
> 	xstats_update: 1;   /**< xstats update is ON(1) / OFF(0) */
> 
> Define 3 functions:
> 
> void rte_eth_xstats_update_enable(uint8_t port_id); void
> rte_eth_xstats_update_disable(uint8_t port_id); int
> rte_eth_xstats_update_get(uint8_t port_id);
> 
> Or define two:
> 
> /* uint8_t xstats_update ; 1 on, 0 off*/ void
> rte_eth_xstats_update_enable(uint8_t port_id, uint8_t xstats_update); int
> rte_eth_xstats_update_get(uint8_t port_id);
> 
> In the struct eth_dev_ops, adding two functions to pass xstats_update to
> driver, because the rxq/txq can't access xstats_update directly.
> So, add a xstats flag per queue data structure.
> for example
> struct vhost_queue {
> 	......
> 	Uint64_t  xstats_flag;
> 	......
> };
> typedef uint16_t (*eth_rx_burst_t)(void *rxq,
> 				   struct rte_mbuf **rx_pkts,
> 				   uint16_t nb_pkts);
> typedef uint16_t (*eth_tx_burst_t)(void *txq,
> 				   struct rte_mbuf **tx_pkts,
> 				   uint16_t nb_pkts);
> 
> struct eth_dev_ops {
> ......
> 	eth_xstats_update_enable_t  xstats_update_enable; /**< xstats
> ON. */
> 	eth_xstats_update_disable_t xstats_update_disable;/**< xstats
> OFF. */ ......
> };
> 
> 	--zhiyong--