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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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.