Date:   Mon, 16 Mar 2020 22:07:26 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        ilias.apalodimas@...aro.org, davem@...emloft.net, brouer@...hat.com
Subject: Re: [RFC/RFT net-next] net: ethernet: ti: fix netdevice stats for XDP

> 
> 
> On 21/02/2020 19:05, Lorenzo Bianconi wrote:
> > Align netdevice statistics when the device is running in XDP mode
> > to other upstream drivers. In particular reports to user-space rx
> > packets even if they are not forwarded to the networking stack
> > (XDP_PASS) but they are redirected (XDP_REDIRECT), dropped (XDP_DROP)
> > or sent back using the same interface (XDP_TX). This patch allows the
> > system administrator to very the device is receiving data correctly.
> 
> I've tested it with xdp-tutorial:
>  drop: ip link set dev eth0 xdp obj ./packet01-parsing/xdp_prog_kern.o sec xdp_packet_parser
>  tx: ip link set dev eth0 xdp obj ./packet-solutions/xdp_prog_kern_03.o sec xdp_icmp_echo
> 
> And see statistic incremented.
> 
> In my opinion, it looks a little bit inconsistent if RX/TX packet/bytes is updated,
> but ndev->stats.rx_dropped is not.

Hi Grygorii,

thanks a lot for testing this patch.
The main idea here is to allow the sysadmin to undertand the device is
receiving frames. We will need to add xdp stats to ethtool to give more details
about why we are dropping packets

> 
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > ---
> > - this patch is compile-only tested
> > ---
> >   drivers/net/ethernet/ti/cpsw.c      |  4 +---
> >   drivers/net/ethernet/ti/cpsw_new.c  |  5 ++---
> >   drivers/net/ethernet/ti/cpsw_priv.c | 13 +++++++++++--
> >   drivers/net/ethernet/ti/cpsw_priv.h |  2 +-
> >   4 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 6ae4a72e6f43..fe3fd33f56f7 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -408,12 +408,10 @@ static void cpsw_rx_handler(void *token, int len, int status)
> >   		xdp.rxq = &priv->xdp_rxq[ch];
> >   		port = priv->emac_port + cpsw->data.dual_emac;
> > -		ret = cpsw_run_xdp(priv, ch, &xdp, page, port);
> > +		ret = cpsw_run_xdp(priv, ch, &xdp, page, port, &len);
> >   		if (ret != CPSW_XDP_PASS)
> >   			goto requeue;
> > -		/* XDP prog might have changed packet data and boundaries */
> > -		len = xdp.data_end - xdp.data;
> >   		headroom = xdp.data - xdp.data_hard_start;
> >   		/* XDP prog can modify vlan tag, so can't use encap header */
> > diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
> > index 71215db7934b..050496e814c3 100644
> > --- a/drivers/net/ethernet/ti/cpsw_new.c
> > +++ b/drivers/net/ethernet/ti/cpsw_new.c
> > @@ -349,12 +349,11 @@ static void cpsw_rx_handler(void *token, int len, int status)
> >   		xdp.data_hard_start = pa;
> >   		xdp.rxq = &priv->xdp_rxq[ch];
> > -		ret = cpsw_run_xdp(priv, ch, &xdp, page, priv->emac_port);
> > +		ret = cpsw_run_xdp(priv, ch, &xdp, page,
> > +				   priv->emac_port, &len);
> >   		if (ret != CPSW_XDP_PASS)
> >   			goto requeue;
> > -		/* XDP prog might have changed packet data and boundaries */
> > -		len = xdp.data_end - xdp.data;
> >   		headroom = xdp.data - xdp.data_hard_start;
> >   		/* XDP prog can modify vlan tag, so can't use encap header */
> > diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
> > index 97a058ca60ac..a41da48db40b 100644
> > --- a/drivers/net/ethernet/ti/cpsw_priv.c
> > +++ b/drivers/net/ethernet/ti/cpsw_priv.c
> > @@ -1317,7 +1317,7 @@ int cpsw_xdp_tx_frame(struct cpsw_priv *priv, struct xdp_frame *xdpf,
> >   }
> >   int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> > -		 struct page *page, int port)
> > +		 struct page *page, int port, int *len)
> >   {
> >   	struct cpsw_common *cpsw = priv->cpsw;
> >   	struct net_device *ndev = priv->ndev;
> > @@ -1335,10 +1335,13 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> >   	}
> >   	act = bpf_prog_run_xdp(prog, xdp);
> > +	/* XDP prog might have changed packet data and boundaries */
> > +	*len = xdp.data_end - xdp.data;
> 
> it should be
> 
> +       *len = xdp->data_end - xdp->data;

ops, right sorry

Regards,
Lorenzo

> 
> 
> > +
> >   	switch (act) {
> >   	case XDP_PASS:
> >   		ret = CPSW_XDP_PASS;
> > -		break;
> > +		goto out;
> >   	case XDP_TX:
> >   		xdpf = convert_to_xdp_frame(xdp);
> >   		if (unlikely(!xdpf))
> > @@ -1364,8 +1367,14 @@ int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> >   		trace_xdp_exception(ndev, prog, act);
> >   		/* fall through -- handle aborts by dropping packet */
> >   	case XDP_DROP:
> > +		ndev->stats.rx_bytes += *len;
> > +		ndev->stats.rx_packets++;
> >   		goto drop;
> >   	}
> > +
> > +	ndev->stats.rx_bytes += *len;
> > +	ndev->stats.rx_packets++;
> > +
> >   out:
> >   	rcu_read_unlock();
> >   	return ret;
> > diff --git a/drivers/net/ethernet/ti/cpsw_priv.h b/drivers/net/ethernet/ti/cpsw_priv.h
> > index b8d7b924ee3d..54efd773e033 100644
> > --- a/drivers/net/ethernet/ti/cpsw_priv.h
> > +++ b/drivers/net/ethernet/ti/cpsw_priv.h
> > @@ -439,7 +439,7 @@ int cpsw_ndo_bpf(struct net_device *ndev, struct netdev_bpf *bpf);
> >   int cpsw_xdp_tx_frame(struct cpsw_priv *priv, struct xdp_frame *xdpf,
> >   		      struct page *page, int port);
> >   int cpsw_run_xdp(struct cpsw_priv *priv, int ch, struct xdp_buff *xdp,
> > -		 struct page *page, int port);
> > +		 struct page *page, int port, int *len);
> >   irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id);
> >   irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id);
> >   int cpsw_tx_mq_poll(struct napi_struct *napi_tx, int budget);
> > 
> 
> -- 
> Best regards,
> grygorii
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists