Re: [PATCH v8] vmxnet3: Add XDP support.

From: William Tu <redacted>
Date: Mon,  2 Jan 2023 18:22:32 -0800

The patch adds native-mode XDP support: XDP DROP, PASS, TX, and REDIRECT.

[...]

Have you looked at xdp_return_frame_bulk()? It flushes frames in
bulks of 16, so it can be faster here.

Also, those 2 branches do the same stuff, so just squash them into
one with ||.

 	else
 		BUG_ON(tbi->map_type != VMXNET3_MAP_NONE);
-
 	tbi->map_type = VMXNET3_MAP_NONE; /* to help debugging */
 }

[...]

const?

+		.order = 0,
+		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.pool_size = size,
+		.nid = NUMA_NO_NODE,
+		.dev = &adapter->pdev->dev,
+		.offset = XDP_PACKET_HEADROOM,
+		.max_len = VMXNET3_XDP_MAX_MTU,
+		.dma_dir = DMA_BIDIRECTIONAL,
+	};
+	struct page_pool *pp;
+	int err;
+
+	pp = page_pool_create(&pp_params);
+	if (IS_ERR(pp))
+		return PTR_ERR(pp);

[...]

+static void
+vmxnet3_pp_put_buff(struct vmxnet3_rx_queue *rq, struct page *page)
+{
+	if (rq->page_pool)
+		page_pool_put_full_page(rq->page_pool, page,
+					rq->napi.napi_id);

Wait, ::napi_id as `bool allow_direct`? Is that intentional (if so,
a short comment would be useful here)?

+}
 
 static void
 vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,

[...]

??? Why are those 2 nl removals here? The code looks prettier with
them and they are not related to the patch subject.

 				ctx->skb = new_skb;
 				sz = rcd->rxdIdx * rq->data_ring.desc_size;
 				memcpy(new_skb->data,

[...]

From [0]:

"do not use xdp_do_flush_map() in new code!"
"#define xdp_do_flush_map xdp_do_flush"

Also '???'.

 			if (rxd->btype == VMXNET3_RXD_BTYPE_HEAD &&
+			    rq->buf_info[ring_idx][i].pp_page &&
+			    rq->buf_info[ring_idx][i].buf_type ==
+							VMXNET3_RX_BUF_XDP) {

Please align that line to the opening brace, just like the previous
ones.

+				vmxnet3_pp_put_buff(rq,
+						    rq->buf_info[ring_idx][i].pp_page);
+				rq->buf_info[ring_idx][i].pp_page = NULL;
+			} else if (rxd->btype == VMXNET3_RXD_BTYPE_HEAD &&
 					rq->buf_info[ring_idx][i].skb) {

Could you maybe align that one too while at it?

 				dma_unmap_single(&adapter->pdev->dev, rxd->addr,
 						 rxd->len, DMA_FROM_DEVICE);

[...]

That assignment to zero is redundant I believe, you use that var
only to store the return value of vmxnet3_create_pp() and return
it in place, so there are no ways to use that zero value.

 
 	/* initialize buf_info */
 	for (i = 0; i < rq->rx_ring[0].size; i++) {

[...]

I'd recommend to enclose bitwise ops in their own set of braces ()
when there are logical ops nearby, some compilers even warn about
that in some cases.

+	    !adapter->xdp_enabled) {
 		devRead->misc.uptFeatures |= UPT1_F_LRO;
 		devRead->misc.maxNumRxSG = cpu_to_le16(1 + MAX_SKB_FRAGS);
 	}

[...]

alloc_etherdev_mq() (-> alloc_netdev_mqs()) allocates a zeroed
memory chunk, so this is redundant.

 	vmxnet3_check_link(adapter, false);
 	return 0;

[...]

How about a union with those two? They're mutually exclusive, aren't
they?

 };
 
 struct vmxnet3_tq_driver_stats {

[...]

&xdp_rxq_info is cacheline-aligned and is big enough, they are
usually placed at the end of a queue structure.

+	struct page_pool *page_pool;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));

(unrelated) -> ____cacheline_aligned

Nit: consider introducing something like

	u32 xdp_enabled:1;

or

	u32 flags;
#define VMXNET3_XDP_ENABLED BIT(0)

or whatever prefer. bools are slippery to use in structures (you'd
also save 31 bits for more flags in future instead of occupying 8-32
bits for just one flag).

+	struct bpf_prog __rcu *xdp_bpf_prog;

Or maybe you could omit the flag above completely and check whether
XDP is enabled via `!!rcu_access_pointer(ring->xdp_bpf_prog)`?

 };
 
 #define VMXNET3_WRITE_BAR0_REG(adapter, reg, val)  \

[...]

+static void
+vmxnet3_xdp_exchange_program(struct vmxnet3_adapter *adapter,
+			     struct bpf_prog *prog)
+{
+	rcu_assign_pointer(adapter->xdp_bpf_prog, prog);
+	if (prog)
+		adapter->xdp_enabled = true;
+	else
+		adapter->xdp_enabled = false;

	adapter->xdp_enabled = !!prog;

+}
+
+static int
+vmxnet3_xdp_set(struct net_device *netdev, struct netdev_bpf *bpf,
+		struct netlink_ext_ack *extack)
+{
+	struct vmxnet3_adapter *adapter = netdev_priv(netdev);
+	struct bpf_prog *new_bpf_prog = bpf->prog;
+	struct bpf_prog *old_bpf_prog;
+	bool need_update;
+	bool running;
+	int err = 0;
+
+	if (new_bpf_prog && netdev->mtu > VMXNET3_XDP_MAX_MTU) {
+		NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
+		return -EOPNOTSUPP;
+	}
+
+	old_bpf_prog = READ_ONCE(adapter->rx_queue[0].adapter->xdp_bpf_prog);

Doesn't the @adapter variable above points to the same struct you
are accessing here?

+	if (!new_bpf_prog && !old_bpf_prog) {
+		adapter->xdp_enabled = false;
+		return 0;
+	}
+	running = netif_running(netdev);
+	need_update = !!old_bpf_prog != !!new_bpf_prog;
+
+	if (running && need_update)
+		vmxnet3_quiesce_dev(adapter);
+
+	vmxnet3_xdp_exchange_program(adapter, new_bpf_prog);
+	if (old_bpf_prog)
+		bpf_prog_put(old_bpf_prog);
+
+	if (running && need_update) {

	if (!running || !need_update)
		return 0;

-1 indent level.

+		vmxnet3_reset_dev(adapter);
+		vmxnet3_rq_destroy_all(adapter);
+		vmxnet3_adjust_rx_ring_size(adapter);
+		err = vmxnet3_rq_create_all(adapter);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+				"failed to re-create rx queues for XDP.");
+			err = -EOPNOTSUPP;
+			goto out;
+		}
+		err = vmxnet3_activate_dev(adapter);
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+				"failed to activate device for XDP.");
+			err = -EOPNOTSUPP;
+			goto out;
+		}
+		clear_bit(VMXNET3_STATE_BIT_RESETTING, &adapter->state);
+	}
+out:
+	return err;

Please avoid oneliner labels, you can do `return err` right where
you goto here.

+}
+
+/* This is the main xdp call used by kernel to set/unset eBPF program. */
+int
+vmxnet3_xdp(struct net_device *netdev, struct netdev_bpf *bpf)
+{
+	switch (bpf->command) {
+	case XDP_SETUP_PROG:
+		netdev_dbg(netdev, "XDP: set program to ");

Is it still relevant?

+		return vmxnet3_xdp_set(netdev, bpf, bpf->extack);
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+int
+vmxnet3_xdp_headroom(struct vmxnet3_adapter *adapter)
+{
+	if (adapter->xdp_enabled)
+		return VMXNET3_XDP_ROOM;

This is confusing. %VMXNET3_XDP_ROOM includes &skb_shared_info's
size. So the function returns not a headroom, rather kernel (XDP +
skb) overhead.

+	else
+		return 0;

	return adapter->xdp_enabled ? VMXNET3_XDP_ROOM : 0;

+}
+

Those two functions:

/start

+void
+vmxnet3_unregister_xdp_rxq(struct vmxnet3_rx_queue *rq)
+{
+	xdp_rxq_info_unreg_mem_model(&rq->xdp_rxq);
+	xdp_rxq_info_unreg(&rq->xdp_rxq);
+}
+
+int
+vmxnet3_register_xdp_rxq(struct vmxnet3_rx_queue *rq,
+			 struct vmxnet3_adapter *adapter)
+{
+	int err;
+
+	err = xdp_rxq_info_reg(&rq->xdp_rxq, adapter->netdev, rq->qid, 0);
+	if (err < 0)
+		return err;
+
+	err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq, MEM_TYPE_PAGE_SHARED,
+					 NULL);
+	if (err < 0) {
+		xdp_rxq_info_unreg(&rq->xdp_rxq);
+		return err;
+	}
+	return 0;
+}

/end

are not used anywhere, probably leftovers.

+
+static int
+vmxnet3_xdp_xmit_frame(struct vmxnet3_adapter *adapter,
+		       struct xdp_frame *xdpf,
+		       struct vmxnet3_tx_queue *tq, bool dma_map)
+{
+	struct vmxnet3_tx_buf_info *tbi = NULL;
+	union Vmxnet3_GenericDesc *gdesc;
+	struct vmxnet3_tx_ctx ctx;
+	int tx_num_deferred;
+	struct page *page;
+	u32 buf_size;
+	int ret = 0;
+	u32 dw2;
+
+	dw2 = (tq->tx_ring.gen ^ 0x1) << VMXNET3_TXD_GEN_SHIFT;
+	dw2 |= xdpf->len;

Consider using FIELD_{GET,PREP}() in future for bitfields.

+	ctx.sop_txd = tq->tx_ring.base + tq->tx_ring.next2fill;
+	gdesc = ctx.sop_txd;
+
+	buf_size = xdpf->len;
+	tbi = tq->buf_info + tq->tx_ring.next2fill;
+
+	if (vmxnet3_cmd_ring_desc_avail(&tq->tx_ring) == 0) {
+		tq->stats.tx_ring_full++;
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	if (dma_map) { /* ndo_xdp_xmit */
+		tbi->map_type = VMXNET3_MAP_XDP_NDO;
+		tbi->dma_addr = dma_map_single(&adapter->pdev->dev,
+				       xdpf->data, buf_size,
+				       DMA_TO_DEVICE);

Bad indent, pls align to the opening braces.

+		if (dma_mapping_error(&adapter->pdev->dev, tbi->dma_addr)) {
+			ret = -EFAULT;
+			goto exit;
+		}
+		tbi->xdpf = xdpf;
+	} else { /* XDP buffer from page pool */
+		tbi->map_type = VMXNET3_MAP_PP_PAGE;
+		page = virt_to_head_page(xdpf->data);
+		tbi->dma_addr = page_pool_get_dma_addr(page) +
+				XDP_PACKET_HEADROOM;
+		dma_sync_single_for_device(&adapter->pdev->dev,
+					   tbi->dma_addr, buf_size,
+					   DMA_BIDIRECTIONAL);
+		tbi->skb = NULL;
+		tbi->xdpf = xdpf;
+	}
+	tbi->len = buf_size;
+
+	gdesc = tq->tx_ring.base + tq->tx_ring.next2fill;
+	WARN_ON_ONCE(gdesc->txd.gen == tq->tx_ring.gen);
+
+	gdesc->txd.addr = cpu_to_le64(tbi->dma_addr);
+	gdesc->dword[2] = cpu_to_le32(dw2);
+
+	/* Setup the EOP desc */
+	gdesc->dword[3] = cpu_to_le32(VMXNET3_TXD_CQ | VMXNET3_TXD_EOP);
+
+	gdesc->txd.om = 0;
+	gdesc->txd.msscof = 0;
+	gdesc->txd.hlen = 0;
+	gdesc->txd.ti = 0;
+
+	tx_num_deferred = le32_to_cpu(tq->shared->txNumDeferred);
+	tq->shared->txNumDeferred += 1;

::txNum_Deferred is __le32, are you sure you can just do += 1
without any conversion?

+	tx_num_deferred++;
+
+	vmxnet3_cmd_ring_adv_next2fill(&tq->tx_ring);
+
+	/* set the last buf_info for the pkt */
+	tbi->sop_idx = ctx.sop_txd - tq->tx_ring.base;
+
+	dma_wmb();
+	gdesc->dword[2] = cpu_to_le32(le32_to_cpu(gdesc->dword[2]) ^
+						  VMXNET3_TXD_GEN);

le32_replace_bits()

(it's generated by a macro, so Elixir doesn't index it. Just look at
 the end of <linux/bitfield.h> if you'd like to use it)

+	if (tx_num_deferred >= le32_to_cpu(tq->shared->txThreshold)) {
+		tq->shared->txNumDeferred = 0;
+		VMXNET3_WRITE_BAR0_REG(adapter,
+				       VMXNET3_REG_TXPROD + tq->qid * 8,
+				       tq->tx_ring.next2fill);
+	}
+exit:
+	return ret;

Same as a bunch of lines above for oneliner labels.

+}
+
+static int
+vmxnet3_xdp_xmit_back(struct vmxnet3_adapter *adapter,
+		      struct xdp_frame *xdpf)
+{
+	struct vmxnet3_tx_queue *tq;
+	struct netdev_queue *nq;
+	int err = 0, cpu;
+	int tq_number;
+
+	tq_number = adapter->num_tx_queues;
+	cpu = smp_processor_id();
+	tq = &adapter->tx_queue[cpu % tq_number];
+	if (tq->stopped)
+		return -ENETDOWN;
+
+	nq = netdev_get_tx_queue(adapter->netdev, tq->qid);
+
+	__netif_tx_lock(nq, cpu);
+	err = vmxnet3_xdp_xmit_frame(adapter, xdpf, tq, false);
+	__netif_tx_unlock(nq);
+	return err;
+}
+
+/* ndo_xdp_xmit */
+int
+vmxnet3_xdp_xmit(struct net_device *dev,
+		 int n, struct xdp_frame **frames, u32 flags)
+{
+	struct vmxnet3_adapter *adapter;
+	struct vmxnet3_tx_queue *tq;
+	struct netdev_queue *nq;
+	int i, err, cpu;
+	int tq_number;
+
+	adapter = netdev_priv(dev);
+
+	if (unlikely(test_bit(VMXNET3_STATE_BIT_QUIESCED, &adapter->state)))
+		return -ENETDOWN;
+	if (unlikely(test_bit(VMXNET3_STATE_BIT_RESETTING, &adapter->state)))
+		return -EINVAL;
+
+	tq_number = adapter->num_tx_queues;
+	cpu = smp_processor_id();
+	tq = &adapter->tx_queue[cpu % tq_number];
+	if (tq->stopped)
+		return -ENETDOWN;
+
+	nq = netdev_get_tx_queue(adapter->netdev, tq->qid);
+
+	for (i = 0; i < n; i++) {
+		err = vmxnet3_xdp_xmit_frame(adapter, frames[i], tq, true);
+		if (err) {
+			tq->stats.xdp_xmit_err++;
+			break;
+		}
+	}
+	tq->stats.xdp_xmit += i;
+
+	return i;
+}
+
+static int
+vmxnet3_run_xdp(struct vmxnet3_rx_queue *rq, struct xdp_buff *xdp)
+{
+	struct xdp_frame *xdpf;
+	int err;
+	u32 act;
+
+	act = bpf_prog_run_xdp(rq->adapter->xdp_bpf_prog, xdp);
+	rq->stats.xdp_packets++;
+
+	switch (act) {
+	case XDP_PASS:
+		goto out;

Same. Just `return act` here.

+	case XDP_REDIRECT:
+		err = xdp_do_redirect(rq->adapter->netdev, xdp,
+				      rq->adapter->xdp_bpf_prog);
+		if (!err)
+			rq->stats.xdp_redirects++;
+		else
+			rq->stats.xdp_drops++;
+		goto out;

Same here and probably a couple times below as well.

+	case XDP_TX:
+		xdpf = xdp_convert_buff_to_frame(xdp);
+		if (!xdpf || vmxnet3_xdp_xmit_back(rq->adapter, xdpf)) {
+			rq->stats.xdp_drops++;
+			goto out_recycle;

So that label is used only once and you could probably inline it
into here.

+		}
+		rq->stats.xdp_tx++;
+		goto out;
+	default:
+		bpf_warn_invalid_xdp_action(rq->adapter->netdev,
+					    rq->adapter->xdp_bpf_prog, act);
+		fallthrough;
+	case XDP_ABORTED:
+		trace_xdp_exception(rq->adapter->netdev,
+				    rq->adapter->xdp_bpf_prog, act);
+		rq->stats.xdp_aborted++;
+		break;
+	case XDP_DROP:
+		rq->stats.xdp_drops++;
+		break;
+	}
+
+out_recycle:
+	page_pool_recycle_direct(rq->page_pool,
+				 virt_to_head_page(xdp->data_hard_start));
+out:
+	return act;
+}
+
+static struct sk_buff *
+vmxnet3_build_skb(struct vmxnet3_rx_queue *rq, struct page *page)
+{
+	struct sk_buff *skb;
+
+	if (!page)
+		return NULL;
+
+	skb = build_skb(page_address(page), PAGE_SIZE);
+	if (unlikely(!skb)) {
+		page_pool_put_full_page(rq->page_pool, page, true);
+		rq->stats.rx_buf_alloc_failure++;
+		return NULL;
+	}
+	return skb;
+}
+
+int
+vmxnet3_process_xdp(struct vmxnet3_adapter *adapter,
+		    struct vmxnet3_rx_queue *rq,
+		    struct Vmxnet3_RxCompDesc *rcd,
+		    struct vmxnet3_rx_buf_info *rbi,
+		    struct Vmxnet3_RxDesc *rxd,
+		    struct sk_buff **skb_xdp_pass)
+{
+	struct bpf_prog *xdp_prog;
+	dma_addr_t new_dma_addr;
+	struct sk_buff *skb;
+	struct xdp_buff xdp;
+	struct page *page;
+	void *new_data;
+	int act;
+
+	act = XDP_DROP;
+	if (unlikely(rcd->len == 0))
+		goto refill;
+
+	page = rbi->pp_page;
+	if (!page)
+		goto refill;
+
+	dma_sync_single_for_cpu(&adapter->pdev->dev,
+				page_pool_get_dma_addr(page) +
+				XDP_PACKET_HEADROOM,
+				rcd->len,

Can fit into the previous line.

+				page_pool_get_dma_dir(rq->page_pool));
+
+	xdp_init_buff(&xdp, rbi->len, &rq->xdp_rxq);
+	xdp_prepare_buff(&xdp, page_address(page), XDP_PACKET_HEADROOM,
+			 rcd->len, false);
+	xdp_buff_clear_frags_flag(&xdp);
+
+	rcu_read_lock();
+	xdp_prog = rcu_dereference(rq->adapter->xdp_bpf_prog);
+	if (!xdp_prog) {
+		rcu_read_unlock();
+		act = XDP_PASS;
+		goto refill;
+	}
+	act = vmxnet3_run_xdp(rq, &xdp);
+	rcu_read_unlock();
+
+	if (act == XDP_PASS) {
+		skb = vmxnet3_build_skb(rq, page);
+		if (!skb) {
+			rq->stats.rx_buf_alloc_failure++;
+			page_pool_recycle_direct(rq->page_pool, page);
+			goto refill;
+		}
+
+		/* bpf prog might change len and data position. */
+		skb_reserve(skb, xdp.data - xdp.data_hard_start);
+		skb_put(skb, xdp.data_end - xdp.data);
+		skb_mark_for_recycle(skb);
+		/* Pass this skb to the main rx loop. */
+		*skb_xdp_pass = skb;
+	}
+
+refill:
+	new_data = vmxnet3_pp_get_buff(rq->page_pool, &new_dma_addr,
+				       GFP_KERNEL);

This code is being run in the softirq context, you can't use
%GFP_KERNEL here, only %GFP_ATOMIC.

Please aling to the opening braces.

+#define VMXNET3_XDP_MAX_MTU (PAGE_SIZE - VMXNET3_XDP_ROOM)
+
+int vmxnet3_xdp(struct net_device *netdev, struct netdev_bpf *bpf);
+void vmxnet3_unregister_xdp_rxq(struct vmxnet3_rx_queue *rq);
+int vmxnet3_register_xdp_rxq(struct vmxnet3_rx_queue *rq,
+			     struct vmxnet3_adapter *adapter);
+int vmxnet3_xdp_headroom(struct vmxnet3_adapter *adapter);
+int vmxnet3_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
+		     u32 flags);
+int vmxnet3_process_xdp(struct vmxnet3_adapter *adapter,
+			struct vmxnet3_rx_queue *rq,
+			struct Vmxnet3_RxCompDesc *rcd,
+			struct vmxnet3_rx_buf_info *rbi,
+			struct Vmxnet3_RxDesc *rxd,
+			struct sk_buff **skb_xdp_pass);
+void *vmxnet3_pp_get_buff(struct page_pool *pp, dma_addr_t *dma_addr,
+			  gfp_t gfp_mask);
+#endif
-- 
2.25.1

[0] https://elixir.bootlin.com/linux/v6.2-rc2/source/include/linux/filter.h#L992

Thanks,
Olek