On 08/24/2016 11:44 AM, Thomas Monjalon wrote:

2016-08-24 13:46, Yuanhan Liu:

quoted

On Tue, Aug 23, 2016 at 12:45:54PM +0300, Panu Matilainen wrote:

quoted

quoted

quoted

quoted

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

quoted

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.

	- Panu -