Re: [RFC net-next 13/15] eth: fbnic: Support AF_XDP zero-copy receive

["Björn Töpel" <bjorn@kernel.org>]

Stanislav Fomichev <sdf.kernel@gmail.com> writes:

> On 10/02, Björn Töpel wrote:
>> fbnic gets RX buffers from page_pool. Use the XSK buffer pool as the
>> RX queue's page-pool memory provider, so allocation, DMA sync,
>> recycling and refill stay in page_pool. Only aligned 4 KiB UMEM
>> chunks are supported.
>> 
>> Each UMEM frame on the header queue must hold one packet. Raise the
>> header-split threshold to at least 3072 bytes, more than half of a
>> 4 KiB page, so the device finishes the frame after one header. Cap
>> it to the space left in the frame after the headroom. An MTU above
>> the threshold needs a socket with XDP_USE_SG and an XDP program with
>> frags support. With XDP_USE_SG, the payload queue also uses UMEM
>> frames, and the device uses each frame for one descriptor only.
>> Without it, the payload queue uses a normal page pool. If the device
>> does not finish a provider frame after one descriptor, drop the
>> packet.
>> 
>> The BDQ refill loop now alternates header and payload buffers on all
>> queues, so two rings that share one finite provider stay balanced.
>> It keeps NAPI scheduled while the provider still has buffers.
>> 
>> Provider frames are never split, so they get no fragment bias. A
>> redirected frame can be recycled before its old BDQ slot is cleaned,
>> and a stale bias would corrupt the new allocation.
>> 
>> The headroom comes from the queue configuration. It must be at least
>> XDP_PACKET_HEADROOM, a multiple of 128 bytes, and fit the hardware
>> field; otherwise queue validation fails. MTU, ring parameter and XDP
>> program changes that do not fit an active provider are rejected.
>> 
>> fbnic did not support XDP_REDIRECT. Add it, with xdp_do_flush(), and
>> advertise it. This also enables copy-mode AF_XDP and the other
>> redirect targets on fbnic. Do not advertise AF_XDP zero copy until
>> TX support is added.
>> 
>> Signed-off-by: Björn Töpel <bjorn@kernel.org>
>> ---
>>  .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |   5 +
>>  .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 140 ++++++-
>>  .../net/ethernet/meta/fbnic/fbnic_netdev.h    |   4 +
>>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 350 +++++++++++++-----
>>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |   6 +
>>  5 files changed, 402 insertions(+), 103 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> index e774740fcb5d..188abe0ead14 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
>> @@ -385,6 +385,11 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
>>  		return -EINVAL;
>>  	}
>>  
>> +	err = fbnic_xsk_validate(fbn, fbn->xdp_prog, netdev->mtu,
>> +				 kernel_ring->hds_thresh, extack);
>> +	if (err)
>> +		return err;
>> +
>>  	if (!netif_running(netdev)) {
>>  		fbnic_set_rings(fbn, ring, kernel_ring);
>>  		return 0;
>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
>> index 8a6703afca38..038e91ef14e4 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
>> @@ -5,6 +5,8 @@
>>  #include <linux/ipv6.h>
>>  #include <linux/types.h>
>>  #include <net/netdev_queues.h>
>> +#include <net/netdev_rx_queue.h>
>> +#include <net/xdp_sock_drv.h>
>>  
>>  #include "fbnic.h"
>>  #include "fbnic_netdev.h"
>> @@ -276,6 +278,8 @@ static int fbnic_set_mac(struct net_device *netdev, void *p)
>>  static int fbnic_change_mtu(struct net_device *dev, int new_mtu)
>>  {
>>  	struct fbnic_net *fbn = netdev_priv(dev);
>> +	struct netlink_ext_ack extack = {};
>> +	int err;
>>  
>>  	if (fbnic_check_split_frames(fbn->xdp_prog, new_mtu, fbn->hds_thresh)) {
>>  		dev_err(&dev->dev,
>> @@ -285,6 +289,15 @@ static int fbnic_change_mtu(struct net_device *dev, int new_mtu)
>>  		return -EINVAL;
>>  	}
>>  
>> +	err = fbnic_xsk_validate(fbn, fbn->xdp_prog, new_mtu,
>> +				 fbn->hds_thresh, &extack);
>> +	if (err) {
>> +		if (extack._msg)
>> +			netdev_err(dev, "%s\n", extack._msg);
>> +
>> +		return err;
>> +	}
>> +
>>  	WRITE_ONCE(dev->mtu, new_mtu);
>>  
>>  	return 0;
>> @@ -534,26 +547,123 @@ bool fbnic_check_split_frames(struct bpf_prog *prog, unsigned int mtu,
>>  	return mtu + ETH_HLEN > hds_thresh;
>>  }
>>  
>> +u32 fbnic_xsk_hds_thresh(u32 headroom, u32 hds_thresh)
>> +{
>> +	u32 max_hds;
>> +
>> +	/* Reserve more than half a device page for each header so the device
>> +	 * retires the provider frame after one descriptor.
>> +	 */
>> +	static_assert(2 * FBNIC_XSK_HDS_THRESH > FBNIC_BD_PAGE_SIZE);
>> +
>> +	max_hds = FBNIC_BD_PAGE_SIZE - headroom - FBNIC_RX_TROOM -
>> +		  FBNIC_RX_PAD;
>> +
>> +	return min(max_t(u32, hds_thresh, FBNIC_XSK_HDS_THRESH), max_hds);
>> +}
>> +
>> +static int fbnic_xsk_validate_frames(struct xsk_buff_pool *pool,
>> +				     struct bpf_prog *prog,
>> +				     unsigned int mtu, u32 hds_thresh,
>> +				     struct netlink_ext_ack *extack)
>> +{
>> +	u32 xsk_hds_thresh;
>> +
>> +	xsk_hds_thresh = fbnic_xsk_hds_thresh(xsk_pool_get_headroom(pool),
>> +					      hds_thresh);
>> +
>> +	if (mtu + ETH_HLEN <= xsk_hds_thresh)
>> +		return 0;
>> +
>> +	if (!xsk_pool_uses_sg(pool)) {
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "AF_XDP zero-copy socket requires XDP_USE_SG");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (fbnic_check_split_frames(prog, mtu, xsk_hds_thresh)) {
>> +		NL_SET_ERR_MSG_MOD(extack,
>> +				   "AF_XDP zero-copy socket requires XDP frags");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int fbnic_xsk_validate(struct fbnic_net *fbn, struct bpf_prog *prog,
>> +		       unsigned int mtu, u32 hds_thresh,
>> +		       struct netlink_ext_ack *extack)
>> +{
>> +	struct net_device *netdev = fbn->netdev;
>> +	struct xsk_buff_pool *pool;
>> +	unsigned int qid;
>> +	int err;
>> +
>> +	for (qid = 0; qid < fbn->num_rx_queues; qid++) {
>> +		pool = xsk_get_pool_from_rxq(netdev, qid);
>> +		if (!pool)
>> +			continue;
>> +
>> +		err = fbnic_xsk_validate_frames(pool, prog, mtu, hds_thresh,
>> +						extack);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int fbnic_xsk_setup_pool(struct net_device *netdev,
>> +				struct xsk_buff_pool *pool, u16 queue_id,
>> +				struct netlink_ext_ack *extack)
>> +{
>> +	struct fbnic_net *fbn = netdev_priv(netdev);
>> +	int err;
>> +
>> +	if (queue_id >= fbn->num_rx_queues)
>> +		return -EINVAL;
>> +
>> +	if (!pool)
>> +		return xsk_pool_setup_page_pool(netdev, NULL, queue_id, extack);
>> +
>> +	err = fbnic_xsk_validate_frames(pool, fbn->xdp_prog, netdev->mtu,
>> +					fbn->hds_thresh, extack);
>> +	if (err)
>> +		return err;
>> +
>> +	return xsk_pool_setup_page_pool(netdev, pool, queue_id, extack);
>> +}
>> +
>>  static int fbnic_bpf(struct net_device *netdev, struct netdev_bpf *bpf)
>>  {
>>  	struct bpf_prog *prog = bpf->prog, *prev_prog;
>>  	struct fbnic_net *fbn = netdev_priv(netdev);
>> +	int err;
>>  
>> -	if (bpf->command != XDP_SETUP_PROG)
>> +	switch (bpf->command) {
>> +	case XDP_SETUP_PROG:
>> +		if (fbnic_check_split_frames(prog, netdev->mtu,
>> +					     fbn->hds_thresh)) {
>> +			NL_SET_ERR_MSG_MOD(bpf->extack,
>> +					   "MTU too high, or HDS threshold is too low for single buffer XDP");
>> +			return -EOPNOTSUPP;
>> +		}
>> +
>> +		err = fbnic_xsk_validate(fbn, prog, netdev->mtu,
>> +					 fbn->hds_thresh, bpf->extack);
>> +		if (err)
>> +			return err;
>> +
>> +		prev_prog = xchg(&fbn->xdp_prog, prog);
>> +		if (prev_prog)
>> +			bpf_prog_put(prev_prog);
>> +		return 0;
>
> [..]
>
>> +	case XDP_SETUP_XSK_POOL:
>> +		return fbnic_xsk_setup_pool(netdev, bpf->xsk.pool,
>> +					    bpf->xsk.queue_id, bpf->extack);
>
> Is there a good reason to ask drivers to call us back with
> xsk_pool_setup_page_pool? For the PP mode, maybe we should add new
> XSK_VALIDATE_POOL or something? So most of that fbnic_xsk_setup_pool stays
> in core?

Makes sense. I'll try core-owned setup with driver opt-in and
validation. Existing direct drivers can keep their setup.

> I guess I'm mentally trying to have a clear separation between
> pp vs non-pp modes. Even that whole xsk_buff_pool might be legacy non-pp
> artifact? New drivers shouldn't bother with it? Or is it too much?

Hmm. New RX code shouldn't need allocator internals, but rings, DMA and
TX state are still shared. I need to noodle a bit on it...