| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200218180336.GB13376@localhost.localdomain>
Date: Tue, 18 Feb 2020 19:03:36 +0100
From: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To: Jesper Dangaard Brouer <brouer@...hat.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
ilias.apalodimas@...aro.org, davem@...emloft.net, andrew@...n.ch,
dsahern@...nel.org, bpf@...r.kernel.org,
Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [RFC net-next] net: mvneta: align xdp stats naming scheme to
mlx5 driver
> On Tue, 18 Feb 2020 18:17:16 +0100
> Lorenzo Bianconi <lorenzo@...nel.org> wrote:
>
> > > On Tue, 18 Feb 2020 01:14:29 +0100
> > > Lorenzo Bianconi <lorenzo@...nel.org> 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 <lorenzo@...nel.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 :)
>
> > >
> > > > + { 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
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists