diff mbox

[ovs-dev] netdev-dpdk: vhost get stats fix

Message ID 1503629500-28795-1-git-send-email-wangzhike@jd.com
State Superseded
Headers show

Commit Message

Zhike Wang Aug. 25, 2017, 2:51 a.m. UTC 
1. "+=" should be "="
2. tx_errors is a generic param, and should be 0 since vhost does not
   create such error.
   Or some app, like libvirt will complain for failure to find this key.

Signed-off-by: wangzhike <wangzhike@jd.com>
---
 lib/netdev-dpdk.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Darrell Ball Aug. 25, 2017, 6:03 a.m. UTC | #1 
I am wondering if we should split the 
    +    stats->tx_errors = 0;
out from this patch and discuss it separately ?

In theory, if a stat is really not supported, we should not display a value for it.
Displaying 0 could be misleading if there really is a problem and we are not detecting it.

Darrell

On 8/24/17, 7:51 PM, "ovs-dev-bounces@openvswitch.org on behalf of wangzhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:

    1. "+=" should be "="
    2. tx_errors is a generic param, and should be 0 since vhost does not
       create such error.
       Or some app, like libvirt will complain for failure to find this key.
    
    Signed-off-by: wangzhike <wangzhike@jd.com>
    ---
     lib/netdev-dpdk.c | 7 ++++---
     1 file changed, 4 insertions(+), 3 deletions(-)
    
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index e90fd0e..1c50aa3 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -2016,14 +2016,15 @@ netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
     
         rte_spinlock_lock(&dev->stats_lock);
         /* Supported Stats */
    -    stats->rx_packets += dev->stats.rx_packets;
    -    stats->tx_packets += dev->stats.tx_packets;
    +    stats->rx_packets = dev->stats.rx_packets;
    +    stats->tx_packets = dev->stats.tx_packets;
         stats->rx_dropped = dev->stats.rx_dropped;
    -    stats->tx_dropped += dev->stats.tx_dropped;
    +    stats->tx_dropped = dev->stats.tx_dropped;
         stats->multicast = dev->stats.multicast;
         stats->rx_bytes = dev->stats.rx_bytes;
         stats->tx_bytes = dev->stats.tx_bytes;
         stats->rx_errors = dev->stats.rx_errors;
    +    stats->tx_errors = 0;
         stats->rx_length_errors = dev->stats.rx_length_errors;
     
         stats->rx_1_to_64_packets = dev->stats.rx_1_to_64_packets;
    -- 
    1.8.3.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=pn_VA8IUu6tNFyFWU0igR0Qo4OPeZ8lCpCMjsGYlKA0&s=1aewY464s93D6GGArf7n8hyc--1TGkrxNBn89LfUNro&e=
Zhike Wang Aug. 25, 2017, 6:12 a.m. UTC | #2 
OK, I can separate this.

Regarding this issue, my idea:
   I donot think it is libvirtd bug. We can have different stats set, say common set, extended set and so on, but we should NOT have one paramter level difference, since it is too hard to stats user to parse it.
   Now I think it is better to change __netdev_dpdk_vhost_send()/netdev_dpdk_vhost_update_tx_counters() to reflect the tx_errors (set as zero for now since no error case). 


I copy more background info as below:


I saw below system log in /var/log/message. I am using libvirtd 3.2.0.
********************
Aug 21 11:21:00 A01-R06-I29-183 libvirtd[24192]: 2017-08-21 03:21:00.057+0000: 24198: error : virCommandWait:2572 : internal error: Child process (ovs-vsctl --timeout=5 get Interface port-7zel2so9sg statistics:rx_errors statistics:rx_dropped statistics:tx_errors statistics:tx_dropped) unexpected exit status 1: ovs-vsctl: no key "tx_errors" in Interface record "port-7zel2so9sg" column statistics
Aug 21 11:21:00 A01-R06-I29-183 ovs-vsctl: ovs|00001|db_ctl_base|ERR|no key "tx_errors" in Interface record "port-ij1mlalpxt" column statistics
*******************

Br.
Wang Zhike
-----Original Message-----
From: Darrell Ball [mailto:dball@vmware.com] 

Sent: Friday, August 25, 2017 2:04 PM
To: 王志克; dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: vhost get stats fix

I am wondering if we should split the 
    +    stats->tx_errors = 0;
out from this patch and discuss it separately ?

In theory, if a stat is really not supported, we should not display a value for it.
Displaying 0 could be misleading if there really is a problem and we are not detecting it.

Darrell

On 8/24/17, 7:51 PM, "ovs-dev-bounces@openvswitch.org on behalf of wangzhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:

    1. "+=" should be "="
    2. tx_errors is a generic param, and should be 0 since vhost does not
       create such error.
       Or some app, like libvirt will complain for failure to find this key.
    
    Signed-off-by: wangzhike <wangzhike@jd.com>

    ---
     lib/netdev-dpdk.c | 7 ++++---
     1 file changed, 4 insertions(+), 3 deletions(-)
    
    diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
    index e90fd0e..1c50aa3 100644
    --- a/lib/netdev-dpdk.c
    +++ b/lib/netdev-dpdk.c
    @@ -2016,14 +2016,15 @@ netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
     
         rte_spinlock_lock(&dev->stats_lock);
         /* Supported Stats */
    -    stats->rx_packets += dev->stats.rx_packets;
    -    stats->tx_packets += dev->stats.tx_packets;
    +    stats->rx_packets = dev->stats.rx_packets;
    +    stats->tx_packets = dev->stats.tx_packets;
         stats->rx_dropped = dev->stats.rx_dropped;
    -    stats->tx_dropped += dev->stats.tx_dropped;
    +    stats->tx_dropped = dev->stats.tx_dropped;
         stats->multicast = dev->stats.multicast;
         stats->rx_bytes = dev->stats.rx_bytes;
         stats->tx_bytes = dev->stats.tx_bytes;
         stats->rx_errors = dev->stats.rx_errors;
    +    stats->tx_errors = 0;
         stats->rx_length_errors = dev->stats.rx_length_errors;
     
         stats->rx_1_to_64_packets = dev->stats.rx_1_to_64_packets;
    -- 
    1.8.3.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=pn_VA8IUu6tNFyFWU0igR0Qo4OPeZ8lCpCMjsGYlKA0&s=1aewY464s93D6GGArf7n8hyc--1TGkrxNBn89LfUNro&e=
Darrell Ball Aug. 25, 2017, 6:30 a.m. UTC | #3 
On 8/24/17, 11:12 PM, "王志克" <wangzhike@jd.com> wrote:

    OK, I can separate this.
    
    Regarding this issue, my idea:
       I donot think it is libvirtd bug. We can have different stats set, say common set, extended set and so on, but we should NOT have one paramter level difference, since it is too hard to stats user to parse it.
       Now I think it is better to change __netdev_dpdk_vhost_send()/netdev_dpdk_vhost_update_tx_counters() to reflect the tx_errors (set as zero for now since no error case). 
    
    
[Darrell]
I understand your valid points.
I would like to dig a little into this with some discussion, JTBS.



    I copy more background info as below:
    
    
    I saw below system log in /var/log/message. I am using libvirtd 3.2.0.
    ********************
    Aug 21 11:21:00 A01-R06-I29-183 libvirtd[24192]: 2017-08-21 03:21:00.057+0000: 24198: error : virCommandWait:2572 : internal error: Child process (ovs-vsctl --timeout=5 get Interface port-7zel2so9sg statistics:rx_errors statistics:rx_dropped statistics:tx_errors statistics:tx_dropped) unexpected exit status 1: ovs-vsctl: no key "tx_errors" in Interface record "port-7zel2so9sg" column statistics
    Aug 21 11:21:00 A01-R06-I29-183 ovs-vsctl: ovs|00001|db_ctl_base|ERR|no key "tx_errors" in Interface record "port-ij1mlalpxt" column statistics
    *******************
    
    Br.
    Wang Zhike
    -----Original Message-----
    From: Darrell Ball [mailto:dball@vmware.com] 

    Sent: Friday, August 25, 2017 2:04 PM
    To: 王志克; dev@openvswitch.org
    Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: vhost get stats fix
    
    I am wondering if we should split the 
        +    stats->tx_errors = 0;
    out from this patch and discuss it separately ?
    
    In theory, if a stat is really not supported, we should not display a value for it.
    Displaying 0 could be misleading if there really is a problem and we are not detecting it.
    
    Darrell
    
    On 8/24/17, 7:51 PM, "ovs-dev-bounces@openvswitch.org on behalf of wangzhike" <ovs-dev-bounces@openvswitch.org on behalf of wangzhike@jd.com> wrote:
    
        1. "+=" should be "="
        2. tx_errors is a generic param, and should be 0 since vhost does not
           create such error.
           Or some app, like libvirt will complain for failure to find this key.
        
        Signed-off-by: wangzhike <wangzhike@jd.com>

        ---
         lib/netdev-dpdk.c | 7 ++++---
         1 file changed, 4 insertions(+), 3 deletions(-)
        
        diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
        index e90fd0e..1c50aa3 100644
        --- a/lib/netdev-dpdk.c
        +++ b/lib/netdev-dpdk.c
        @@ -2016,14 +2016,15 @@ netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
         
             rte_spinlock_lock(&dev->stats_lock);
             /* Supported Stats */
        -    stats->rx_packets += dev->stats.rx_packets;
        -    stats->tx_packets += dev->stats.tx_packets;
        +    stats->rx_packets = dev->stats.rx_packets;
        +    stats->tx_packets = dev->stats.tx_packets;
             stats->rx_dropped = dev->stats.rx_dropped;
        -    stats->tx_dropped += dev->stats.tx_dropped;
        +    stats->tx_dropped = dev->stats.tx_dropped;
             stats->multicast = dev->stats.multicast;
             stats->rx_bytes = dev->stats.rx_bytes;
             stats->tx_bytes = dev->stats.tx_bytes;
             stats->rx_errors = dev->stats.rx_errors;
        +    stats->tx_errors = 0;
             stats->rx_length_errors = dev->stats.rx_length_errors;
         
             stats->rx_1_to_64_packets = dev->stats.rx_1_to_64_packets;
        -- 
        1.8.3.1
        
        _______________________________________________
        dev mailing list
        dev@openvswitch.org
        https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=pn_VA8IUu6tNFyFWU0igR0Qo4OPeZ8lCpCMjsGYlKA0&s=1aewY464s93D6GGArf7n8hyc--1TGkrxNBn89LfUNro&e=
diff mbox

Patch

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index e90fd0e..1c50aa3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2016,14 +2016,15 @@  netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
 
     rte_spinlock_lock(&dev->stats_lock);
     /* Supported Stats */
-    stats->rx_packets += dev->stats.rx_packets;
-    stats->tx_packets += dev->stats.tx_packets;
+    stats->rx_packets = dev->stats.rx_packets;
+    stats->tx_packets = dev->stats.tx_packets;
     stats->rx_dropped = dev->stats.rx_dropped;
-    stats->tx_dropped += dev->stats.tx_dropped;
+    stats->tx_dropped = dev->stats.tx_dropped;
     stats->multicast = dev->stats.multicast;
     stats->rx_bytes = dev->stats.rx_bytes;
     stats->tx_bytes = dev->stats.tx_bytes;
     stats->rx_errors = dev->stats.rx_errors;
+    stats->tx_errors = 0;
     stats->rx_length_errors = dev->stats.rx_length_errors;
 
     stats->rx_1_to_64_packets = dev->stats.rx_1_to_64_packets;