On Tue, 18 Feb 2020 18:17:16 +0100
Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

On Tue, 18 Feb 2020 01:14:29 +0100
Lorenzo Bianconi [off-list ref] wrote:
  

quoted

Introduce "rx" prefix in the name scheme for xdp counters
on rx path.
Differentiate between XDP_TX and ndo_xdp_xmit counters

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b7045b6a15c2..6223700dc3df 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c

@@ -344,6 +344,7 @@ enum {
 	ETHTOOL_XDP_REDIRECT,
 	ETHTOOL_XDP_PASS,
 	ETHTOOL_XDP_DROP,
+	ETHTOOL_XDP_XMIT,
 	ETHTOOL_XDP_TX,
 	ETHTOOL_MAX_STATS,
 };

@@ -399,10 +400,11 @@ static const struct mvneta_statistic mvneta_statistics[] = {
 	{ ETHTOOL_STAT_EEE_WAKEUP, T_SW, "eee_wakeup_errors", },
 	{ ETHTOOL_STAT_SKB_ALLOC_ERR, T_SW, "skb_alloc_errors", },
 	{ ETHTOOL_STAT_REFILL_ERR, T_SW, "refill_errors", },
-	{ ETHTOOL_XDP_REDIRECT, T_SW, "xdp_redirect", },
-	{ ETHTOOL_XDP_PASS, T_SW, "xdp_pass", },
-	{ ETHTOOL_XDP_DROP, T_SW, "xdp_drop", },
-	{ ETHTOOL_XDP_TX, T_SW, "xdp_tx", },
+	{ ETHTOOL_XDP_REDIRECT, T_SW, "rx_xdp_redirect", },
+	{ ETHTOOL_XDP_PASS, T_SW, "rx_xdp_pass", },
+	{ ETHTOOL_XDP_DROP, T_SW, "rx_xdp_drop", },
+	{ ETHTOOL_XDP_TX, T_SW, "rx_xdp_tx_xmit", },  

Hmmm... "rx_xdp_tx_xmit", I expected this to be named "rx_xdp_tx" to
count the XDP_TX actions, but I guess this means something else.  

just reused mlx5 naming scheme here :)

Well, IMHO the naming in mlx5 should not automatically be seen as the
correct way ;-)

sure, I have no prefernces actually :)

quoted

quoted

quoted

+	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },  

Okay, maybe.  I guess, this will still be valid for when we add an
XDP egress/TX-hook point.  

same here

quoted

quoted

 };
 
 struct mvneta_stats {

@@ -414,6 +416,7 @@ struct mvneta_stats {
 	u64	xdp_redirect;
 	u64	xdp_pass;
 	u64	xdp_drop;
+	u64	xdp_xmit;
 	u64	xdp_tx;
 };
 

@@ -2050,7 +2053,10 @@ mvneta_xdp_submit_frame(struct mvneta_port

*pp, struct mvneta_tx_queue *txq,
u64_stats_update_begin(&stats->syncp); stats->es.ps.tx_bytes +=
xdpf->len; stats->es.ps.tx_packets++;
-	stats->es.ps.xdp_tx++;
+	if (buf->type == MVNETA_TYPE_XDP_NDO)
+		stats->es.ps.xdp_xmit++;
+	else
+		stats->es.ps.xdp_tx++;  

I don't like that you add a branch (if-statement) in this fast-path
code.

Do we really need to account in the xmit frame function, if this
was a XDP_REDIRECT or XDP_TX that started the xmit?  I mean we
already have action counters for XDP_REDIRECT and XDP_TX (but I
guess you skipped the XDP_TX action counter).   

ack, good point..I think we can move the code in
mvneta_xdp_xmit_back/mvneta_xdp_xmit in order to avoid the if()
condition. Moreover we can move it out the for loop in
mvneta_xdp_xmit().

Sure, but I want the "xmit" counter (or what every we call it) to only
increment if the Ethernet frame was successfully queued. For me that is
an important property of the counter.  As I want a sysadm to be able to
use this counter to see if frames are getting dropped due to TX-queue
overflow by comparing/correlating counters.

yes, it is just a matter of using "num_frame - drops" as counter in
mvneta_xdp_xmit()

This also begs the question: Should we have a counter for TX-queue
overflows?  That will make it even easier to diagnose problems from a
sysadm perspective?

not yet. Do you want to add it?

quoted

I will fix in a formal patch

quoted

quoted

quoted

 	u64_stats_update_end(&stats->syncp);
 
 	mvneta_txq_inc_put(txq);

@@ -4484,6 +4490,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 		u64 xdp_redirect;
 		u64 xdp_pass;
 		u64 xdp_drop;
+		u64 xdp_xmit;
 		u64 xdp_tx;
 
 		stats = per_cpu_ptr(pp->stats, cpu);

@@ -4494,6 +4501,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 			xdp_redirect = stats->es.ps.xdp_redirect;
 			xdp_pass = stats->es.ps.xdp_pass;
 			xdp_drop = stats->es.ps.xdp_drop;
+			xdp_xmit = stats->es.ps.xdp_xmit;
 			xdp_tx = stats->es.ps.xdp_tx;
 		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
 

@@ -4502,6 +4510,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
 		es->ps.xdp_redirect += xdp_redirect;
 		es->ps.xdp_pass += xdp_pass;
 		es->ps.xdp_drop += xdp_drop;
+		es->ps.xdp_xmit += xdp_xmit;
 		es->ps.xdp_tx += xdp_tx;
 	}
 }

@@ -4555,6 +4564,9 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
 			case ETHTOOL_XDP_TX:
 				pp->ethtool_stats[i] = stats.ps.xdp_tx;
 				break;
+			case ETHTOOL_XDP_XMIT:
+				pp->ethtool_stats[i] = stats.ps.xdp_xmit;
+				break;
 			}
 			break;
 		}  

It doesn't look like you have an action counter for XDP_TX, but we have
one for XDP_REDIRECT?  

I did not get you here sorry, I guess they should be accounted in two
separated counters.

Checking code that got applied, you have xdp "action" counters for:
 - XDP_PASS     => stats->xdp_pass++;
 - XDP_REDIRECT => stats->xdp_redirect++ (on xdp_do_redirect == 0)
 - XDP_TX       => no-counter

nope, we have a counter for it...it is "rx_xdp_tx_xmit".
Moreover we have "tx_xdp_xmit" for ndo_xdp_xmit

- XDP_TX -> stats->xdp_tx++
- ndo_xdp_xmit -> stats->xdp_xmit++

Regards,
Lorenzo

 - XDP_ABORTED  => fall-through (to stats->xdp_drop++);
 - XDP_DROP     => stats->xdp_drop++

Notice the action XDP_TX is not accounted, that was my point.  While
all other XDP "actions" have a counter.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer