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.
--yliu
