[dpdk-dev] vhost: add pmd xstats
Commit Message
This feature adds vhost pmd extended statistics from per queue perspective
for the application such as OVS etc.
The statistics counters are based on RFC 2819 and 2863 as follows:
rx/tx_good_packets
rx/tx_total_bytes
rx/tx_dropped_pkts
rx/tx_broadcast_packets
rx/tx_multicast_packets
rx/tx_ucast_packets
rx/tx_undersize_errors
rx/tx_size_64_packets
rx/tx_size_65_to_127_packets;
rx/tx_size_128_to_255_packets;
rx/tx_size_256_to_511_packets;
rx/tx_size_512_to_1023_packets;
rx/tx_size_1024_to_1522_packets;
rx/tx_1523_to_max_packets;
rx/tx_errors
rx_fragmented_errors
rx_jabber_errors
rx_unknown_protos_packets;
No API is changed or added.
rte_eth_xstats_get_names() to retrieve what kinds of vhost xstats are
supported,
rte_eth_xstats_get() to retrieve vhost extended statistics,
rte_eth_xstats_reset() to reset vhost extended statistics.
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).
The usage of vhost pmd is the same as virtio pmd xstats.
for example, when test-pmd application is running in interactive mode
vhost pmd xstats will support the two following commands:
show port xstats all|port_id will show vhost xstats
clear port xstats all|port_id will reset vhost xstats
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
config/common_base | 1 +
drivers/net/vhost/rte_eth_vhost.c | 295 +++++++++++++++++++++++++++++++++++++-
2 files changed, 295 insertions(+), 1 deletion(-)
Comments
On 08/19/2016 03:16 PM, Zhiyong Yang wrote:
> This feature adds vhost pmd extended statistics from per queue perspective
> for the application such as OVS etc.
>
> The statistics counters are based on RFC 2819 and 2863 as follows:
>
> rx/tx_good_packets
> rx/tx_total_bytes
> rx/tx_dropped_pkts
> rx/tx_broadcast_packets
> rx/tx_multicast_packets
> rx/tx_ucast_packets
> rx/tx_undersize_errors
> rx/tx_size_64_packets
> rx/tx_size_65_to_127_packets;
> rx/tx_size_128_to_255_packets;
> rx/tx_size_256_to_511_packets;
> rx/tx_size_512_to_1023_packets;
> rx/tx_size_1024_to_1522_packets;
> rx/tx_1523_to_max_packets;
> rx/tx_errors
> rx_fragmented_errors
> rx_jabber_errors
> rx_unknown_protos_packets;
>
> No API is changed or added.
> rte_eth_xstats_get_names() to retrieve what kinds of vhost xstats are
> supported,
> rte_eth_xstats_get() to retrieve vhost extended statistics,
> rte_eth_xstats_reset() to reset vhost extended statistics.
>
> 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 -
Hi, Panu:
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Monday, August 22, 2016 3:53 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
>
> On 08/19/2016 03:16 PM, Zhiyong Yang wrote:
> > This feature adds vhost pmd extended statistics from per queue
> > perspective for the application such as OVS etc.
> >
> > The statistics counters are based on RFC 2819 and 2863 as follows:
> >
> > rx/tx_good_packets
> > rx/tx_total_bytes
> > rx/tx_dropped_pkts
> > rx/tx_broadcast_packets
> > rx/tx_multicast_packets
> > rx/tx_ucast_packets
> > rx/tx_undersize_errors
> > rx/tx_size_64_packets
> > rx/tx_size_65_to_127_packets;
> > rx/tx_size_128_to_255_packets;
> > rx/tx_size_256_to_511_packets;
> > rx/tx_size_512_to_1023_packets;
> > rx/tx_size_1024_to_1522_packets;
> > rx/tx_1523_to_max_packets;
> > rx/tx_errors
> > rx_fragmented_errors
> > rx_jabber_errors
> > rx_unknown_protos_packets;
> >
> > No API is changed or added.
> > rte_eth_xstats_get_names() to retrieve what kinds of vhost xstats are
> > supported,
> > rte_eth_xstats_get() to retrieve vhost extended statistics,
> > rte_eth_xstats_reset() to reset vhost extended statistics.
> >
> > 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.
-zhiyong-
On 08/23/2016 11:04 AM, Yang, Zhiyong wrote:
> Hi, Panu:
>
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>> Sent: Monday, August 22, 2016 3:53 PM
>> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
>>
>> On 08/19/2016 03:16 PM, Zhiyong Yang wrote:
>>> This feature adds vhost pmd extended statistics from per queue
>>> perspective for the application such as OVS etc.
>>>
>>> The statistics counters are based on RFC 2819 and 2863 as follows:
>>>
>>> rx/tx_good_packets
>>> rx/tx_total_bytes
>>> rx/tx_dropped_pkts
>>> rx/tx_broadcast_packets
>>> rx/tx_multicast_packets
>>> rx/tx_ucast_packets
>>> rx/tx_undersize_errors
>>> rx/tx_size_64_packets
>>> rx/tx_size_65_to_127_packets;
>>> rx/tx_size_128_to_255_packets;
>>> rx/tx_size_256_to_511_packets;
>>> rx/tx_size_512_to_1023_packets;
>>> rx/tx_size_1024_to_1522_packets;
>>> rx/tx_1523_to_max_packets;
>>> rx/tx_errors
>>> rx_fragmented_errors
>>> rx_jabber_errors
>>> rx_unknown_protos_packets;
>>>
>>> No API is changed or added.
>>> rte_eth_xstats_get_names() to retrieve what kinds of vhost xstats are
>>> supported,
>>> rte_eth_xstats_get() to retrieve vhost extended statistics,
>>> rte_eth_xstats_reset() to reset vhost extended statistics.
>>>
>>> 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.
- Panu -
>
> -zhiyong-
>
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
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.
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.
- Panu -
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Wednesday, August 24, 2016 8:37 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@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--
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@dpdk.org] On Behalf Of Yang, Zhiyong
Sent: Thursday, August 25, 2016 5:22 PM
To: Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Wednesday, August 24, 2016 8:37 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@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--
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@dpdk.org] On Behalf Of Yao, Lei A
Sent: Tuesday, August 30, 2016 10:46 AM
To: Yang, Zhiyong <zhiyong.yang@intel.com>; Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@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@dpdk.org] On Behalf Of Yang, Zhiyong
Sent: Thursday, August 25, 2016 5:22 PM
To: Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Wednesday, August 24, 2016 8:37 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@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--
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@intel.com>; Yang, Zhiyong <zhiyong.yang@intel.com>; Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@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@dpdk.org] On Behalf Of Yao, Lei A
Sent: Tuesday, August 30, 2016 10:46 AM
To: Yang, Zhiyong <zhiyong.yang@intel.com>; Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@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@dpdk.org] On Behalf Of Yang, Zhiyong
Sent: Thursday, August 25, 2016 5:22 PM
To: Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: Wednesday, August 24, 2016 8:37 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@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--
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@intel.com>; Yang, Zhiyong
> <zhiyong.yang@intel.com>; Panu Matilainen <pmatilai@redhat.com>;
> Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@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@intel.com>; Yang, Zhiyong
> <zhiyong.yang@intel.com>; Panu Matilainen <pmatilai@redhat.com>;
> Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Cc: dev@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@dpdk.org] On Behalf Of Yao, Lei A
> Sent: Tuesday, August 30, 2016 10:46 AM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; Panu Matilainen
> <pmatilai@redhat.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Cc: dev@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@dpdk.org] On Behalf Of Yang, Zhiyong
> Sent: Thursday, August 25, 2016 5:22 PM
> To: Panu Matilainen <pmatilai@redhat.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vhost: add pmd xstats
>
>
>
> > -----Original Message-----
> > From: Panu Matilainen [mailto:pmatilai@redhat.com]
> > Sent: Wednesday, August 24, 2016 8:37 PM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>; Yuanhan Liu
> > <yuanhan.liu@linux.intel.com>
> > Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@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--
Hi, all:
> -----Original Message-----
> From: Yang, Zhiyong
> Sent: Wednesday, August 31, 2016 3:19 PM
> To: dev@dpdk.org
> Cc: Panu Matilainen <pmatilai@redhat.com>; Liu, Yuanhan
> <yuanhan.liu@intel.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Yao, Lei A <lei.a.yao@intel.com>; Yang,
> Zhiyong <zhiyong.yang@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] vhost: add pmd xstats
>
> 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)."
>
I consult the requirement from dev@openvswitch.com, Jesse Gross
<jesse@kernel.org> reply the question as following:
All other stats in OVS (including flows and other counters that don't come
from hardware) count bytes without the CRC. Presumably it would be best to
adjust the physical NIC stats with DPDK to do the same.
@@ -561,6 +561,7 @@ CONFIG_RTE_LIBRTE_VHOST_DEBUG=n
# To compile, CONFIG_RTE_LIBRTE_VHOST should be enabled.
#
CONFIG_RTE_LIBRTE_PMD_VHOST=n
+CONFIG_RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS=n
#
#Compile Xen domain0 support
@@ -45,6 +45,9 @@
#include <rte_kvargs.h>
#include <rte_virtio_net.h>
#include <rte_spinlock.h>
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+#include <rte_common.h>
+#endif
#include "rte_eth_vhost.h"
@@ -72,6 +75,12 @@ static struct ether_addr base_eth_addr = {
}
};
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+struct vhost_xstats {
+ uint64_t stat[16];
+};
+#endif
+
struct vhost_queue {
int vid;
rte_atomic32_t allow_queuing;
@@ -85,7 +94,10 @@ struct vhost_queue {
uint64_t missed_pkts;
uint64_t rx_bytes;
uint64_t tx_bytes;
-};
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+ struct vhost_xstats xstats;
+#endif
+ };
struct pmd_internal {
char *dev_name;
@@ -127,6 +139,274 @@ struct rte_vhost_vring_state {
static struct rte_vhost_vring_state *vring_states[RTE_MAX_ETHPORTS];
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+enum rte_vhostqueue_rxtx {
+ RTE_VHOSTQUEUE_RX = 0,
+ RTE_VHOSTQUEUE_TX = 1
+};
+
+#define RTE_ETH_VHOST_XSTATS_NAME_SIZE 64
+
+struct rte_vhost_xstats_name_off {
+ char name[RTE_ETH_VHOST_XSTATS_NAME_SIZE];
+ uint64_t offset;
+};
+
+/* [rt]_qX_ is prepended to the name string here */
+static const struct rte_vhost_xstats_name_off rte_vhost_rxq_stat_strings[] = {
+ {"good_packets",
+ offsetof(struct vhost_queue, rx_pkts)},
+ {"total_bytes",
+ offsetof(struct vhost_queue, rx_bytes)},
+ {"dropped_pkts",
+ offsetof(struct vhost_queue, missed_pkts)},
+ {"broadcast_packets",
+ offsetof(struct vhost_queue, xstats.stat[8])},
+ {"multicast_packets",
+ offsetof(struct vhost_queue, xstats.stat[9])},
+ {"ucast_packets",
+ offsetof(struct vhost_queue, xstats.stat[10])},
+ {"undersize_errors",
+ offsetof(struct vhost_queue, xstats.stat[0])},
+ {"size_64_packets",
+ offsetof(struct vhost_queue, xstats.stat[1])},
+ {"size_65_to_127_packets",
+ offsetof(struct vhost_queue, xstats.stat[2])},
+ {"size_128_to_255_packets",
+ offsetof(struct vhost_queue, xstats.stat[3])},
+ {"size_256_to_511_packets",
+ offsetof(struct vhost_queue, xstats.stat[4])},
+ {"size_512_to_1023_packets",
+ offsetof(struct vhost_queue, xstats.stat[5])},
+ {"size_1024_to_1522_packets",
+ offsetof(struct vhost_queue, xstats.stat[6])},
+ {"size_1523_to_max_packets",
+ offsetof(struct vhost_queue, xstats.stat[7])},
+ {"errors",
+ offsetof(struct vhost_queue, xstats.stat[11])},
+ {"fragmented_errors",
+ offsetof(struct vhost_queue, xstats.stat[12])},
+ {"jabber_errors",
+ offsetof(struct vhost_queue, xstats.stat[13])},
+ {"unknown_protos_packets",
+ offsetof(struct vhost_queue, xstats.stat[14])},
+};
+
+/* [tx]_qX_ is prepended to the name string here */
+static const struct rte_vhost_xstats_name_off rte_vhost_txq_stat_strings[] = {
+ {"good_packets",
+ offsetof(struct vhost_queue, tx_pkts)},
+ {"total_bytes",
+ offsetof(struct vhost_queue, tx_bytes)},
+ {"dropped_pkts",
+ offsetof(struct vhost_queue, missed_pkts)},
+ {"broadcast_packets",
+ offsetof(struct vhost_queue, xstats.stat[8])},
+ {"multicast_packets",
+ offsetof(struct vhost_queue, xstats.stat[9])},
+ {"ucast_packets",
+ offsetof(struct vhost_queue, xstats.stat[10])},
+ {"size_64_packets",
+ offsetof(struct vhost_queue, xstats.stat[1])},
+ {"size_65_to_127_packets",
+ offsetof(struct vhost_queue, xstats.stat[2])},
+ {"size_128_to_255_packets",
+ offsetof(struct vhost_queue, xstats.stat[3])},
+ {"size_256_to_511_packets",
+ offsetof(struct vhost_queue, xstats.stat[4])},
+ {"size_512_to_1023_packets",
+ offsetof(struct vhost_queue, xstats.stat[5])},
+ {"size_1024_to_1522_packets",
+ offsetof(struct vhost_queue, xstats.stat[6])},
+ {"size_1523_to_max_packets",
+ offsetof(struct vhost_queue, xstats.stat[7])},
+ {"errors",
+ offsetof(struct vhost_queue, xstats.stat[11])},
+};
+
+#define VHOST_NB_RXQ_XSTATS (sizeof(rte_vhost_rxq_stat_strings) / \
+ sizeof(rte_vhost_rxq_stat_strings[0]))
+
+#define VHOST_NB_TXQ_XSTATS (sizeof(rte_vhost_txq_stat_strings) / \
+ sizeof(rte_vhost_txq_stat_strings[0]))
+
+static void
+vhost_dev_xstats_reset(struct rte_eth_dev *dev)
+{
+ struct vhost_queue *vqrx = NULL;
+ struct vhost_queue *vqtx = NULL;
+ unsigned int i = 0;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ if (!dev->data->rx_queues[i])
+ continue;
+ vqrx = (struct vhost_queue *)dev->data->rx_queues[i];
+ vqrx->rx_pkts = 0;
+ vqrx->rx_bytes = 0;
+ memset(&vqrx->xstats, 0, sizeof(vqrx->xstats));
+ }
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ if (!dev->data->tx_queues[i])
+ continue;
+ vqtx = (struct vhost_queue *)dev->data->tx_queues[i];
+ vqtx->tx_pkts = 0;
+ vqtx->tx_bytes = 0;
+ vqtx->missed_pkts = 0;
+ memset(&vqtx->xstats, 0, sizeof(vqtx->xstats));
+ }
+}
+
+static int
+vhost_dev_xstats_get_names(struct rte_eth_dev *dev,
+ struct rte_eth_xstat_name *xstats_names,
+ __rte_unused unsigned int limit)
+{
+ unsigned int i = 0;
+ unsigned int t = 0;
+ int count = 0;
+ int nstats = dev->data->nb_rx_queues * VHOST_NB_RXQ_XSTATS
+ + dev->data->nb_tx_queues * VHOST_NB_TXQ_XSTATS;
+
+ if (xstats_names) {
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ struct vhost_queue *rxvq = dev->data->rx_queues[i];
+
+ if (!rxvq)
+ continue;
+ for (t = 0; t < VHOST_NB_RXQ_XSTATS; t++) {
+ snprintf(xstats_names[count].name,
+ sizeof(xstats_names[count].name),
+ "rx_q%u_%s", i,
+ rte_vhost_rxq_stat_strings[t].name);
+ count++;
+ }
+ }
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ struct vhost_queue *txvq = dev->data->tx_queues[i];
+
+ if (!txvq)
+ continue;
+ for (t = 0; t < VHOST_NB_TXQ_XSTATS; t++) {
+ snprintf(xstats_names[count].name,
+ sizeof(xstats_names[count].name),
+ "tx_q%u_%s", i,
+ rte_vhost_txq_stat_strings[t].name);
+ count++;
+ }
+ }
+ return count;
+ }
+ return nstats;
+}
+
+static int
+vhost_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
+ unsigned int n)
+{
+ unsigned int i;
+ unsigned int t;
+ unsigned int count = 0;
+
+ unsigned int nxstats = dev->data->nb_rx_queues * VHOST_NB_RXQ_XSTATS
+ + dev->data->nb_tx_queues * VHOST_NB_TXQ_XSTATS;
+
+ if (n < nxstats)
+ return nxstats;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ struct vhost_queue *rxvq =
+ (struct vhost_queue *)dev->data->rx_queues[i];
+
+ if (!rxvq)
+ continue;
+
+ for (t = 0; t < VHOST_NB_RXQ_XSTATS; t++) {
+ xstats[count].value = *(uint64_t *)(((char *)rxvq)
+ + rte_vhost_rxq_stat_strings[t].offset);
+ count++;
+ }
+ }
+
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ struct vhost_queue *txvq =
+ (struct vhost_queue *)dev->data->tx_queues[i];
+
+ if (!txvq)
+ continue;
+
+ for (t = 0; t < VHOST_NB_TXQ_XSTATS; t++) {
+ xstats[count].value = *(uint64_t *)(((char *)txvq)
+ + rte_vhost_txq_stat_strings[t].offset);
+ count++;
+ }
+ }
+
+ return count;
+}
+
+static void
+vhost_update_packet_xstats(struct vhost_queue *vq,
+ struct rte_mbuf **bufs,
+ uint16_t nb_rxtx,
+ uint16_t nb_bufs,
+ enum rte_vhostqueue_rxtx vqueue_rxtx)
+{
+ uint32_t pkt_len = 0;
+ uint64_t i = 0;
+ uint64_t index;
+ struct ether_addr *ea = NULL;
+ struct vhost_xstats *xstats_update = &vq->xstats;
+
+ for (i = 0; i < nb_rxtx ; i++) {
+ pkt_len = bufs[i]->pkt_len;
+ if (pkt_len == 64) {
+ xstats_update->stat[1]++;
+
+ } else if (pkt_len > 64 && pkt_len < 1024) {
+ index = (sizeof(pkt_len) * 8)
+ - __builtin_clz(pkt_len) - 5;
+ xstats_update->stat[index]++;
+ } else {
+ if (pkt_len < 64)
+ xstats_update->stat[0]++;
+ else if (pkt_len <= 1522)
+ xstats_update->stat[6]++;
+ else if (pkt_len > 1522)
+ xstats_update->stat[7]++;
+ }
+
+ ea = rte_pktmbuf_mtod(bufs[i], struct ether_addr *);
+ if (is_multicast_ether_addr(ea)) {
+ if (is_broadcast_ether_addr(ea))
+ /* broadcast++; */
+ xstats_update->stat[8]++;
+ else
+ /* multicast++; */
+ xstats_update->stat[9]++;
+ }
+ }
+ /* non-multi/broadcast, multi/broadcast, including those
+ * that were discarded or not sent. from rfc2863
+ */
+ if (vqueue_rxtx == RTE_VHOSTQUEUE_RX) {
+ xstats_update->stat[10] = vq->rx_pkts + vq->missed_pkts
+ - (xstats_update->stat[8]
+ + xstats_update->stat[9]);
+ } else {
+ for (i = nb_rxtx; i < nb_bufs ; i++) {
+ if (is_multicast_ether_addr(ea)) {
+ if (is_broadcast_ether_addr(ea))
+ xstats_update->stat[8]++;
+ else
+ xstats_update->stat[9]++;
+ }
+ }
+ xstats_update->stat[10] = vq->tx_pkts + vq->missed_pkts
+ - (xstats_update->stat[8] + xstats_update->stat[9]);
+ }
+}
+#endif
+
static uint16_t
eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
{
@@ -152,6 +432,10 @@ eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
r->rx_bytes += bufs[i]->pkt_len;
}
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+ vhost_update_packet_xstats(r, bufs, nb_rx, nb_rx, RTE_VHOSTQUEUE_RX);
+#endif
+
out:
rte_atomic32_set(&r->while_queuing, 0);
@@ -182,6 +466,10 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
for (i = 0; likely(i < nb_tx); i++)
r->tx_bytes += bufs[i]->pkt_len;
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+ vhost_update_packet_xstats(r, bufs, nb_tx, nb_bufs, RTE_VHOSTQUEUE_TX);
+#endif
+
for (i = 0; likely(i < nb_tx); i++)
rte_pktmbuf_free(bufs[i]);
out:
@@ -682,6 +970,11 @@ static const struct eth_dev_ops ops = {
.link_update = eth_link_update,
.stats_get = eth_stats_get,
.stats_reset = eth_stats_reset,
+#ifdef RTE_LIBRTE_PMD_VHOST_UPDATE_XSTATS
+ .xstats_reset = vhost_dev_xstats_reset,
+ .xstats_get = vhost_dev_xstats_get,
+ .xstats_get_names = vhost_dev_xstats_get_names,
+#endif
};
static int