> On Tue, 18 Feb 2020 18:17:16 +0100 > Lorenzo Bianconi wrote: > > > > On Tue, 18 Feb 2020 01:14:29 +0100 > > > Lorenzo Bianconi wrote: > > > > > > > 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 > > > > --- > > > > 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 :) > > > > > > > > + { 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 > > > > > > > > > }; > > > > > > > > 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? > > > > I will fix in a formal patch > > > > > > > > > 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 >