2019-10-06 18:46:03

by Julio Faracco

[permalink] [raw]
Subject: [PATCH RFC net-next 1/2] drivers: net: virtio_net: Add tx_timeout stats field

From: Julio Faracco <[email protected]>

For debug purpose of TX timeout events, a tx_timeout entry was added to
monitor this special case: when dev_watchdog identifies a tx_timeout and
throw an exception. We can both consider this event as an error, but
driver should report as a tx_timeout statistic.

Signed-off-by: Julio Faracco <[email protected]>
Signed-off-by: Daiane Mendes <[email protected]>
Cc: Jason Wang <[email protected]>
---
drivers/net/virtio_net.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f3de0ac8b0b..27f9b212c9f5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -75,6 +75,7 @@ struct virtnet_sq_stats {
u64 xdp_tx;
u64 xdp_tx_drops;
u64 kicks;
+ u64 tx_timeouts;
};

struct virtnet_rq_stats {
@@ -98,6 +99,7 @@ static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
{ "xdp_tx", VIRTNET_SQ_STAT(xdp_tx) },
{ "xdp_tx_drops", VIRTNET_SQ_STAT(xdp_tx_drops) },
{ "kicks", VIRTNET_SQ_STAT(kicks) },
+ { "tx_timeouts", VIRTNET_SQ_STAT(tx_timeouts) },
};

static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
@@ -1721,7 +1723,7 @@ static void virtnet_stats(struct net_device *dev,
int i;

for (i = 0; i < vi->max_queue_pairs; i++) {
- u64 tpackets, tbytes, rpackets, rbytes, rdrops;
+ u64 tpackets, tbytes, terrors, rpackets, rbytes, rdrops;
struct receive_queue *rq = &vi->rq[i];
struct send_queue *sq = &vi->sq[i];

@@ -1729,6 +1731,7 @@ static void virtnet_stats(struct net_device *dev,
start = u64_stats_fetch_begin_irq(&sq->stats.syncp);
tpackets = sq->stats.packets;
tbytes = sq->stats.bytes;
+ terrors = sq->stats.tx_timeouts;
} while (u64_stats_fetch_retry_irq(&sq->stats.syncp, start));

do {
@@ -1743,6 +1746,7 @@ static void virtnet_stats(struct net_device *dev,
tot->rx_bytes += rbytes;
tot->tx_bytes += tbytes;
tot->rx_dropped += rdrops;
+ tot->tx_errors += terrors;
}

tot->tx_dropped = dev->stats.tx_dropped;
--
2.21.0


2019-10-07 14:18:29

by Julian Wiedmann

[permalink] [raw]
Subject: Re: [PATCH RFC net-next 1/2] drivers: net: virtio_net: Add tx_timeout stats field

On 06.10.19 20:45, [email protected] wrote:
> From: Julio Faracco <[email protected]>
>
> For debug purpose of TX timeout events, a tx_timeout entry was added to
> monitor this special case: when dev_watchdog identifies a tx_timeout and
> throw an exception. We can both consider this event as an error, but
> driver should report as a tx_timeout statistic.
>

Hi Julio,
dev_watchdog() updates txq->trans_timeout, why isn't that sufficient?


> Signed-off-by: Julio Faracco <[email protected]>
> Signed-off-by: Daiane Mendes <[email protected]>
> Cc: Jason Wang <[email protected]>
> ---
> drivers/net/virtio_net.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f3de0ac8b0b..27f9b212c9f5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -75,6 +75,7 @@ struct virtnet_sq_stats {
> u64 xdp_tx;
> u64 xdp_tx_drops;
> u64 kicks;
> + u64 tx_timeouts;
> };
>
> struct virtnet_rq_stats {
> @@ -98,6 +99,7 @@ static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
> { "xdp_tx", VIRTNET_SQ_STAT(xdp_tx) },
> { "xdp_tx_drops", VIRTNET_SQ_STAT(xdp_tx_drops) },
> { "kicks", VIRTNET_SQ_STAT(kicks) },
> + { "tx_timeouts", VIRTNET_SQ_STAT(tx_timeouts) },
> };
>
> static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> @@ -1721,7 +1723,7 @@ static void virtnet_stats(struct net_device *dev,
> int i;
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> - u64 tpackets, tbytes, rpackets, rbytes, rdrops;
> + u64 tpackets, tbytes, terrors, rpackets, rbytes, rdrops;
> struct receive_queue *rq = &vi->rq[i];
> struct send_queue *sq = &vi->sq[i];
>
> @@ -1729,6 +1731,7 @@ static void virtnet_stats(struct net_device *dev,
> start = u64_stats_fetch_begin_irq(&sq->stats.syncp);
> tpackets = sq->stats.packets;
> tbytes = sq->stats.bytes;
> + terrors = sq->stats.tx_timeouts;
> } while (u64_stats_fetch_retry_irq(&sq->stats.syncp, start));
>
> do {
> @@ -1743,6 +1746,7 @@ static void virtnet_stats(struct net_device *dev,
> tot->rx_bytes += rbytes;
> tot->tx_bytes += tbytes;
> tot->rx_dropped += rdrops;
> + tot->tx_errors += terrors;
> }
>
> tot->tx_dropped = dev->stats.tx_dropped;
>

2019-10-07 14:57:56

by Julio Faracco

[permalink] [raw]
Subject: Re: [PATCH RFC net-next 1/2] drivers: net: virtio_net: Add tx_timeout stats field

Em seg, 7 de out de 2019 às 11:15, Julian Wiedmann <[email protected]> escreveu:
>
> On 06.10.19 20:45, [email protected] wrote:
> > From: Julio Faracco <[email protected]>
> >
> > For debug purpose of TX timeout events, a tx_timeout entry was added to
> > monitor this special case: when dev_watchdog identifies a tx_timeout and
> > throw an exception. We can both consider this event as an error, but
> > driver should report as a tx_timeout statistic.
> >
>
> Hi Julio,
> dev_watchdog() updates txq->trans_timeout, why isn't that sufficient?

Hi Julian,
Good catch! This case (this patch) it would be useful only for ethtool stats.
This is not so relevant as the method implementation itself.
But, on the other hand, I think it should be relevant if we split into
tx_timeout per queue.
Anyway, suggestions are welcome.

>
>
> > Signed-off-by: Julio Faracco <[email protected]>
> > Signed-off-by: Daiane Mendes <[email protected]>
> > Cc: Jason Wang <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4f3de0ac8b0b..27f9b212c9f5 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -75,6 +75,7 @@ struct virtnet_sq_stats {
> > u64 xdp_tx;
> > u64 xdp_tx_drops;
> > u64 kicks;
> > + u64 tx_timeouts;
> > };
> >
> > struct virtnet_rq_stats {
> > @@ -98,6 +99,7 @@ static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
> > { "xdp_tx", VIRTNET_SQ_STAT(xdp_tx) },
> > { "xdp_tx_drops", VIRTNET_SQ_STAT(xdp_tx_drops) },
> > { "kicks", VIRTNET_SQ_STAT(kicks) },
> > + { "tx_timeouts", VIRTNET_SQ_STAT(tx_timeouts) },
> > };
> >
> > static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> > @@ -1721,7 +1723,7 @@ static void virtnet_stats(struct net_device *dev,
> > int i;
> >
> > for (i = 0; i < vi->max_queue_pairs; i++) {
> > - u64 tpackets, tbytes, rpackets, rbytes, rdrops;
> > + u64 tpackets, tbytes, terrors, rpackets, rbytes, rdrops;
> > struct receive_queue *rq = &vi->rq[i];
> > struct send_queue *sq = &vi->sq[i];
> >
> > @@ -1729,6 +1731,7 @@ static void virtnet_stats(struct net_device *dev,
> > start = u64_stats_fetch_begin_irq(&sq->stats.syncp);
> > tpackets = sq->stats.packets;
> > tbytes = sq->stats.bytes;
> > + terrors = sq->stats.tx_timeouts;
> > } while (u64_stats_fetch_retry_irq(&sq->stats.syncp, start));
> >
> > do {
> > @@ -1743,6 +1746,7 @@ static void virtnet_stats(struct net_device *dev,
> > tot->rx_bytes += rbytes;
> > tot->tx_bytes += tbytes;
> > tot->rx_dropped += rdrops;
> > + tot->tx_errors += terrors;
> > }
> >
> > tot->tx_dropped = dev->stats.tx_dropped;
> >
>