Re: [PATCH net-next 09/10] bnxt_en: Add basic XDP support.

[Jakub Kicinski <kubakici@wp.pl>]

On Mon, 30 Jan 2017 20:49:34 -0500, Michael Chan wrote:
> +#ifdef CONFIG_BNXT_XDP
> +/* returns the following:
> + * true    - packet consumed by XDP and new buffer is allocated.
> + * false   - packet should be passed to the stack.
> + */
> +bool bnxt_rx_xdp(struct bnxt_rx_ring_info *rxr, u16 cons, void *data,
> +		 u8 *data_ptr, unsigned int len, dma_addr_t dma_addr,
> +		 u8 *event)
> +{
> +	struct bpf_prog *xdp_prog = READ_ONCE(rxr->xdp_prog);
> +	struct xdp_buff xdp;
> +	u32 act;
> +
> +	if (!xdp_prog)
> +		return false;
> +
> +	xdp.data = data_ptr;
> +	xdp.data_end = xdp.data + len;
> +	rcu_read_lock();
> +	act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +	rcu_read_unlock();
> +
> +	switch (act) {
> +	case XDP_PASS:
> +		return false;
> +
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +		/* Fall thru */
> +	case XDP_DROP:
> +	case XDP_ABORTED:
> +		bnxt_reuse_rx_data(rxr, cons, data);
> +		break;
> +	}
> +	return true;
> +}

It would be cool if you could populate the appropriate paths with
tracepoints Daniel added in a67edbf4fb6d ("bpf: add initial bpf
tracepoints").

> +	if (netif_running(dev))
> +		bnxt_close_nic(bp, true, false);
> +
> +	old = xchg(&bp->xdp_prog, prog);
> +	if (old)
> +		bpf_prog_put(old);
> +
> +	if (prog) {
> +		bnxt_set_rx_skb_mode(bp, true);
> +	} else {
> +		bool sh = (bp->flags & BNXT_FLAG_SHARED_RINGS) ? true : false;
> +		int rx, tx;
> +
> +		bnxt_set_rx_skb_mode(bp, false);
> +		bnxt_get_max_rings(bp, &rx, &tx, sh);
> +		if (rx > 1) {
> +			bp->flags &= ~BNXT_FLAG_NO_AGG_RINGS;
> +			bp->dev->hw_features |= NETIF_F_LRO;
> +		}
> +	}
> +	bp->tx_nr_rings_xdp = tx_xdp;
> +	bp->tx_nr_rings = bp->tx_nr_rings_per_tc * tc + tx_xdp;
> +	bp->cp_nr_rings = max_t(int, bp->tx_nr_rings, bp->rx_nr_rings);
> +	bp->num_stat_ctxs = bp->cp_nr_rings;
> +	bnxt_set_tpa_flags(bp);
> +	bnxt_set_ring_params(bp);
> +
> +	if (netif_running(dev))
> +		return bnxt_open_nic(bp, true, false);

Mm..  I thought doing open/close like this and risking you won't be
able to come back up was frowned upon [1].  I must be misunderstanding
things...

[1] https://www.spinics.net/lists/netdev/msg365229.html

> +int bnxt_xdp(struct net_device *dev, struct netdev_xdp *xdp)
> +{
> +	struct bnxt *bp = netdev_priv(dev);
> +	int rc;
> +
> +	switch (xdp->command) {
> +	case XDP_SETUP_PROG:
> +		rc = bnxt_xdp_set(bp, xdp->prog);
> +		break;
> +	case XDP_QUERY_PROG:
> +		xdp->prog_attached = !!bp->xdp_prog;
> +		rc = 0;
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +	return rc;
> +}

Nit: why not simply return?