Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics
On 10.09.2019 20:31, Sriram Vatala wrote:
> -Original Message-
> From: Ilya Maximets
> Sent: 10 September 2019 19:29
> To: Sriram Vatala ; ovs-dev@openvswitch.org
> Cc: ktray...@redhat.com; ian.sto...@intel.com
> Subject: Re: [PATCH v8] netdev-dpdk:Detailed packet drop statistics
>
> On 08.09.2019 19:01, Sriram Vatala wrote:
>> OVS may be unable to transmit packets for multiple reasons and today
>> there is a single counter to track packets dropped due to any of those
>> reasons. The most common reason is that a VM is unable to read packets
>> fast enough causing the vhostuser port transmit queue on the OVS side
>> to become full. This manifests as a problem with VNFs not receiving
>> all packets. Having a separate drop counter to track packets dropped
>> because the transmit queue is full will clearly indicate that the
>> problem is on the VM side and not in OVS. Similarly maintaining
>> separate counters for all possible drops helps in indicating sensible
>> cause for packet drops.
>>
>> This patch adds custom software stats counters to track packets
>> dropped at port level and these counters are displayed along with
>> other stats in "ovs-vsctl get interface statistics"
>> command. The detailed stats will be available for both dpdk and
>> vhostuser ports.
>>
>> Signed-off-by: Sriram Vatala
>> ---
>> Changes since v7 :
>> Defined structure netdev_dpdk_sw_stats and moved all custom stats into
>> it.
>> Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk
>> strucure.
>> stats are reported with prefix with netdev_dpdk
>> ---
>> include/openvswitch/netdev.h | 14 +++
>> lib/netdev-dpdk.c | 109 +++---
>> utilities/bugtool/automake.mk | 3 +-
>> utilities/bugtool/ovs-bugtool-get-port-stats | 25
>> .../plugins/network-status/openvswitch.xml| 1 +
>> vswitchd/vswitch.xml | 24
>> 6 files changed, 156 insertions(+), 20 deletions(-) create mode
>> 100755 utilities/bugtool/ovs-bugtool-get-port-stats
>>
>> diff --git a/include/openvswitch/netdev.h
>> b/include/openvswitch/netdev.h index 0c10f7b48..c17e6a97d 100644
>> --- a/include/openvswitch/netdev.h
>> +++ b/include/openvswitch/netdev.h
>> @@ -89,6 +89,20 @@ struct netdev_stats {
>> uint64_t rx_jabber_errors;
>> };
>>
>> +/* Custom software stats for dpdk ports */ struct
>> +netdev_dpdk_sw_stats {
>> +/* No. of retries when unable to transmit. */
>> +uint64_t tx_retries;
>> +/* Pkt drops when unable to transmit; Probably Tx queue is full */
>> +uint64_t tx_failure_drops;
>> +/* Pkt len greater than max dev MTU */
>> +uint64_t tx_mtu_exceeded_drops;
>> +/* Pkt drops in egress policier processing */
>> +uint64_t tx_qos_drops;
>> +/* Pkt drops in ingress policier processing */
>> +uint64_t rx_qos_drops;
>> +};
>
> This should not be in a common header since the only user is netdev-dpdk.c.
> Regarding this code itself:
> 1. s/policier/policer/
> 2. s/Pkt/Packet/, s/len/length/
> 3. s/max dev MTU/device MTU/ (MTU already has 'maximum' word).
> 4. All comments should end with a period.
>
> Sorry I missed to check spell. I will fix this in my next patch v9
> I will move this structure definition to netdev-dpdk.c and make it static.
>
>> +
>> /* Structure representation of custom statistics counter */ struct
>> netdev_custom_counter {
>> uint64_t value;
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> bc20d6843..39aab4672 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -413,11 +413,10 @@ struct netdev_dpdk {
>>
>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>> struct netdev_stats stats;
>> -/* Custom stat for retries when unable to transmit. */
>> -uint64_t tx_retries;
>> +struct netdev_dpdk_sw_stats *sw_stats;
>> /* Protects stats */
>> rte_spinlock_t stats_lock;
>> -/* 4 pad bytes here. */
>> +/* 36 pad bytes here. */
>> );
>>
>> PADDED_MEMBERS(CACHE_LINE_SIZE,
>> @@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
>> port_no,
>> dev->rte_xstats_ids = NULL;
>> dev->rte_xstats_ids_size = 0;
>>
>> -dev->tx_retries = 0;
>> +dev->sw_stats = (struct netdev_dpdk_sw_stats *)
>> +xcalloc(1,sizeof(struct
>> + netdev_dpdk_sw_stats));
>
> This should be:
>dev->sw_stats = xzalloc(sizeof *dev->sw_stats); Or
>dev->sw_stats = dpdk_rte_mzalloc(sizeof *dev->sw_stats);
>
> The later variant will require proper error handling as it could return NULL.
> I will change this in next patch v9.
>
> See the Documentation/internals/contributing/coding-style.rst for details.
>
>>
>> return 0;
>> }
>> @@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
>> ovs_list_remove(>list_node);
>> free(ovsrcu_get_protected(struct ingress_policer *,
>>
Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics
-Original Message-
From: Ilya Maximets
Sent: 10 September 2019 19:29
To: Sriram Vatala ; ovs-dev@openvswitch.org
Cc: ktray...@redhat.com; ian.sto...@intel.com
Subject: Re: [PATCH v8] netdev-dpdk:Detailed packet drop statistics
On 08.09.2019 19:01, Sriram Vatala wrote:
> OVS may be unable to transmit packets for multiple reasons and today
> there is a single counter to track packets dropped due to any of those
> reasons. The most common reason is that a VM is unable to read packets
> fast enough causing the vhostuser port transmit queue on the OVS side
> to become full. This manifests as a problem with VNFs not receiving
> all packets. Having a separate drop counter to track packets dropped
> because the transmit queue is full will clearly indicate that the
> problem is on the VM side and not in OVS. Similarly maintaining
> separate counters for all possible drops helps in indicating sensible
> cause for packet drops.
>
> This patch adds custom software stats counters to track packets
> dropped at port level and these counters are displayed along with
> other stats in "ovs-vsctl get interface statistics"
> command. The detailed stats will be available for both dpdk and
> vhostuser ports.
>
> Signed-off-by: Sriram Vatala
> ---
> Changes since v7 :
> Defined structure netdev_dpdk_sw_stats and moved all custom stats into
> it.
> Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk
> strucure.
> stats are reported with prefix with netdev_dpdk
> ---
> include/openvswitch/netdev.h | 14 +++
> lib/netdev-dpdk.c | 109 +++---
> utilities/bugtool/automake.mk | 3 +-
> utilities/bugtool/ovs-bugtool-get-port-stats | 25
> .../plugins/network-status/openvswitch.xml| 1 +
> vswitchd/vswitch.xml | 24
> 6 files changed, 156 insertions(+), 20 deletions(-) create mode
> 100755 utilities/bugtool/ovs-bugtool-get-port-stats
>
> diff --git a/include/openvswitch/netdev.h
> b/include/openvswitch/netdev.h index 0c10f7b48..c17e6a97d 100644
> --- a/include/openvswitch/netdev.h
> +++ b/include/openvswitch/netdev.h
> @@ -89,6 +89,20 @@ struct netdev_stats {
> uint64_t rx_jabber_errors;
> };
>
> +/* Custom software stats for dpdk ports */ struct
> +netdev_dpdk_sw_stats {
> +/* No. of retries when unable to transmit. */
> +uint64_t tx_retries;
> +/* Pkt drops when unable to transmit; Probably Tx queue is full */
> +uint64_t tx_failure_drops;
> +/* Pkt len greater than max dev MTU */
> +uint64_t tx_mtu_exceeded_drops;
> +/* Pkt drops in egress policier processing */
> +uint64_t tx_qos_drops;
> +/* Pkt drops in ingress policier processing */
> +uint64_t rx_qos_drops;
> +};
This should not be in a common header since the only user is netdev-dpdk.c.
Regarding this code itself:
1. s/policier/policer/
2. s/Pkt/Packet/, s/len/length/
3. s/max dev MTU/device MTU/ (MTU already has 'maximum' word).
4. All comments should end with a period.
Sorry I missed to check spell. I will fix this in my next patch v9
I will move this structure definition to netdev-dpdk.c and make it static.
> +
> /* Structure representation of custom statistics counter */ struct
> netdev_custom_counter {
> uint64_t value;
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> bc20d6843..39aab4672 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -413,11 +413,10 @@ struct netdev_dpdk {
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> struct netdev_stats stats;
> -/* Custom stat for retries when unable to transmit. */
> -uint64_t tx_retries;
> +struct netdev_dpdk_sw_stats *sw_stats;
> /* Protects stats */
> rte_spinlock_t stats_lock;
> -/* 4 pad bytes here. */
> +/* 36 pad bytes here. */
> );
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> dev->rte_xstats_ids = NULL;
> dev->rte_xstats_ids_size = 0;
>
> -dev->tx_retries = 0;
> +dev->sw_stats = (struct netdev_dpdk_sw_stats *)
> +xcalloc(1,sizeof(struct
> + netdev_dpdk_sw_stats));
This should be:
dev->sw_stats = xzalloc(sizeof *dev->sw_stats); Or
dev->sw_stats = dpdk_rte_mzalloc(sizeof *dev->sw_stats);
The later variant will require proper error handling as it could return NULL.
I will change this in next patch v9.
See the Documentation/internals/contributing/coding-style.rst for details.
>
> return 0;
> }
> @@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
> ovs_list_remove(>list_node);
> free(ovsrcu_get_protected(struct ingress_policer *,
>>ingress_policer));
> +free(dev->sw_stats);
> ovs_mutex_destroy(>mutex);
> }
>
> @@ -2171,6 +2172,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> struct ingress_policer *policer =
Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics
On 08.09.2019 19:01, Sriram Vatala wrote:
> OVS may be unable to transmit packets for multiple reasons and
> today there is a single counter to track packets dropped due to
> any of those reasons. The most common reason is that a VM is
> unable to read packets fast enough causing the vhostuser port
> transmit queue on the OVS side to become full. This manifests
> as a problem with VNFs not receiving all packets. Having a
> separate drop counter to track packets dropped because the
> transmit queue is full will clearly indicate that the problem
> is on the VM side and not in OVS. Similarly maintaining separate
> counters for all possible drops helps in indicating sensible
> cause for packet drops.
>
> This patch adds custom software stats counters to track packets
> dropped at port level and these counters are displayed along with
> other stats in "ovs-vsctl get interface statistics"
> command. The detailed stats will be available for both dpdk and
> vhostuser ports.
>
> Signed-off-by: Sriram Vatala
> ---
> Changes since v7 :
> Defined structure netdev_dpdk_sw_stats and moved all custom stats
> into it.
> Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk
> strucure.
> stats are reported with prefix with netdev_dpdk
> ---
> include/openvswitch/netdev.h | 14 +++
> lib/netdev-dpdk.c | 109 +++---
> utilities/bugtool/automake.mk | 3 +-
> utilities/bugtool/ovs-bugtool-get-port-stats | 25
> .../plugins/network-status/openvswitch.xml| 1 +
> vswitchd/vswitch.xml | 24
> 6 files changed, 156 insertions(+), 20 deletions(-)
> create mode 100755 utilities/bugtool/ovs-bugtool-get-port-stats
>
> diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
> index 0c10f7b48..c17e6a97d 100644
> --- a/include/openvswitch/netdev.h
> +++ b/include/openvswitch/netdev.h
> @@ -89,6 +89,20 @@ struct netdev_stats {
> uint64_t rx_jabber_errors;
> };
>
> +/* Custom software stats for dpdk ports */
> +struct netdev_dpdk_sw_stats {
> +/* No. of retries when unable to transmit. */
> +uint64_t tx_retries;
> +/* Pkt drops when unable to transmit; Probably Tx queue is full */
> +uint64_t tx_failure_drops;
> +/* Pkt len greater than max dev MTU */
> +uint64_t tx_mtu_exceeded_drops;
> +/* Pkt drops in egress policier processing */
> +uint64_t tx_qos_drops;
> +/* Pkt drops in ingress policier processing */
> +uint64_t rx_qos_drops;
> +};
This should not be in a common header since the only user is netdev-dpdk.c.
Regarding this code itself:
1. s/policier/policer/
2. s/Pkt/Packet/, s/len/length/
3. s/max dev MTU/device MTU/ (MTU already has 'maximum' word).
4. All comments should end with a period.
> +
> /* Structure representation of custom statistics counter */
> struct netdev_custom_counter {
> uint64_t value;
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index bc20d6843..39aab4672 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -413,11 +413,10 @@ struct netdev_dpdk {
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> struct netdev_stats stats;
> -/* Custom stat for retries when unable to transmit. */
> -uint64_t tx_retries;
> +struct netdev_dpdk_sw_stats *sw_stats;
> /* Protects stats */
> rte_spinlock_t stats_lock;
> -/* 4 pad bytes here. */
> +/* 36 pad bytes here. */
> );
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> @@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> dev->rte_xstats_ids = NULL;
> dev->rte_xstats_ids_size = 0;
>
> -dev->tx_retries = 0;
> +dev->sw_stats = (struct netdev_dpdk_sw_stats *)
> +xcalloc(1,sizeof(struct netdev_dpdk_sw_stats));
This should be:
dev->sw_stats = xzalloc(sizeof *dev->sw_stats);
Or
dev->sw_stats = dpdk_rte_mzalloc(sizeof *dev->sw_stats);
The later variant will require proper error handling as it could return NULL.
See the Documentation/internals/contributing/coding-style.rst for details.
>
> return 0;
> }
> @@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
> ovs_list_remove(>list_node);
> free(ovsrcu_get_protected(struct ingress_policer *,
>>ingress_policer));
> +free(dev->sw_stats);
> ovs_mutex_destroy(>mutex);
> }
>
> @@ -2171,6 +2172,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
> uint16_t nb_rx = 0;
> uint16_t dropped = 0;
> +uint16_t qos_drops = 0;
> int qid = rxq->queue_id * VIRTIO_QNUM + VIRTIO_TXQ;
> int vid = netdev_dpdk_get_vid(dev);
>
> @@ -2202,11 +2204,13 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
> (struct rte_mbuf **) batch->packets,
>
Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics
Hi All,
I have addressed the comments given in patch v7. Please review the patch v8
and let me know your comments if any.
Thanks & Regards,
Sriram.
-Original Message-
From: Sriram Vatala
Sent: 08 September 2019 21:32
To: ovs-dev@openvswitch.org; i.maxim...@samsung.com
Cc: ktray...@redhat.com; ian.sto...@intel.com; Sriram Vatala
Subject: [PATCH v8] netdev-dpdk:Detailed packet drop statistics
OVS may be unable to transmit packets for multiple reasons and today there
is a single counter to track packets dropped due to any of those reasons.
The most common reason is that a VM is unable to read packets fast enough
causing the vhostuser port transmit queue on the OVS side to become full.
This manifests as a problem with VNFs not receiving all packets. Having a
separate drop counter to track packets dropped because the transmit queue is
full will clearly indicate that the problem is on the VM side and not in
OVS. Similarly maintaining separate counters for all possible drops helps in
indicating sensible cause for packet drops.
This patch adds custom software stats counters to track packets dropped at
port level and these counters are displayed along with other stats in
"ovs-vsctl get interface statistics"
command. The detailed stats will be available for both dpdk and vhostuser
ports.
Signed-off-by: Sriram Vatala
---
Changes since v7 :
Defined structure netdev_dpdk_sw_stats and moved all custom stats into it.
Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk strucure.
stats are reported with prefix with netdev_dpdk
---
include/openvswitch/netdev.h | 14 +++
lib/netdev-dpdk.c | 109 +++---
utilities/bugtool/automake.mk | 3 +-
utilities/bugtool/ovs-bugtool-get-port-stats | 25
.../plugins/network-status/openvswitch.xml| 1 +
vswitchd/vswitch.xml | 24
6 files changed, 156 insertions(+), 20 deletions(-) create mode 100755
utilities/bugtool/ovs-bugtool-get-port-stats
diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
index 0c10f7b48..c17e6a97d 100644
--- a/include/openvswitch/netdev.h
+++ b/include/openvswitch/netdev.h
@@ -89,6 +89,20 @@ struct netdev_stats {
uint64_t rx_jabber_errors;
};
+/* Custom software stats for dpdk ports */ struct netdev_dpdk_sw_stats
+{
+/* No. of retries when unable to transmit. */
+uint64_t tx_retries;
+/* Pkt drops when unable to transmit; Probably Tx queue is full */
+uint64_t tx_failure_drops;
+/* Pkt len greater than max dev MTU */
+uint64_t tx_mtu_exceeded_drops;
+/* Pkt drops in egress policier processing */
+uint64_t tx_qos_drops;
+/* Pkt drops in ingress policier processing */
+uint64_t rx_qos_drops;
+};
+
/* Structure representation of custom statistics counter */ struct
netdev_custom_counter {
uint64_t value;
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
bc20d6843..39aab4672 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -413,11 +413,10 @@ struct netdev_dpdk {
PADDED_MEMBERS(CACHE_LINE_SIZE,
struct netdev_stats stats;
-/* Custom stat for retries when unable to transmit. */
-uint64_t tx_retries;
+struct netdev_dpdk_sw_stats *sw_stats;
/* Protects stats */
rte_spinlock_t stats_lock;
-/* 4 pad bytes here. */
+/* 36 pad bytes here. */
);
PADDED_MEMBERS(CACHE_LINE_SIZE,
@@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
port_no,
dev->rte_xstats_ids = NULL;
dev->rte_xstats_ids_size = 0;
-dev->tx_retries = 0;
+dev->sw_stats = (struct netdev_dpdk_sw_stats *)
+xcalloc(1,sizeof(struct netdev_dpdk_sw_stats));
return 0;
}
@@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
ovs_list_remove(>list_node);
free(ovsrcu_get_protected(struct ingress_policer *,
>ingress_policer));
+free(dev->sw_stats);
ovs_mutex_destroy(>mutex);
}
@@ -2171,6 +2172,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
uint16_t nb_rx = 0;
uint16_t dropped = 0;
+uint16_t qos_drops = 0;
int qid = rxq->queue_id * VIRTIO_QNUM + VIRTIO_TXQ;
int vid = netdev_dpdk_get_vid(dev);
@@ -2202,11 +2204,13 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
(struct rte_mbuf **) batch->packets,
nb_rx, true);
dropped -= nb_rx;
+qos_drops = dropped;
}
rte_spinlock_lock(>stats_lock);
netdev_dpdk_vhost_update_rx_counters(>stats, batch->packets,
nb_rx, dropped);
+dev->sw_stats->rx_qos_drops += qos_drops;
rte_spinlock_unlock(>stats_lock);
batch->count = nb_rx;
@@ -2232,6 +2236,7 @@
Re: [ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics
Bleep bloop. Greetings Sriram Vatala via dev, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 122 characters long (recommended limit is 79) #422 FILE: utilities/bugtool/plugins/network-status/openvswitch.xml:43: /usr/share/openvswitch/scripts/ovs-bugtool-get-port-stats Lines checked: 461, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
