XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Stanislav Fomichev <sdf@google.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org,
	David Ahern <dsahern@gmail.com>,
	Willem de Bruijn <willemb@google.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>,
	Alexander Lobakin <alexandr.lobakin@intel.com>,
	Magnus Karlsson <magnus.karlsson@gmail.com>,
	Maryam Tahhan <mtahhan@redhat.com>,
	xdp-hints@xdp-project.net, netdev@vger.kernel.org
Subject: [xdp-hints] Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs
Date: Wed, 7 Dec 2022 21:00:19 -0800	[thread overview]
Message-ID: <20221207210019.41dc9b6b@kernel.org> (raw)
In-Reply-To: <20221206024554.3826186-4-sdf@google.com>

The offload tests still pass after this, right?
TBH I don't remember this code well enough to spot major issues.

On Mon,  5 Dec 2022 18:45:45 -0800 Stanislav Fomichev wrote:
> There is an ndo handler per kfunc, the verifier replaces a call to the
> generic kfunc with a call to the per-device one.
> 
> For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
> implements all possible metatada kfuncs. Not all devices have to
> implement them. If kfunc is not supported by the target device,
> the default implementation is called instead.
> 
> Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags,
> we treat prog_index as target device for kfunc resolution.

> @@ -2476,10 +2477,18 @@ void bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
>  				       struct net_device *netdev);
>  bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev);
>  
> +void *bpf_offload_resolve_kfunc(struct bpf_prog *prog, u32 func_id);

There seems to be some mis-naming going on. I expected:

  offloaded =~ nfp
  dev_bound == XDP w/ funcs

*_offload_resolve_kfunc looks misnamed? Unless you want to resolve 
for HW offload?

>  void unpriv_ebpf_notify(int new_state);
>  
>  #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL)
>  int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr);
> +void bpf_offload_bound_netdev_unregister(struct net_device *dev);

ditto: offload_bound is a mix of terms no?

> @@ -1611,6 +1612,10 @@ struct net_device_ops {
>  	ktime_t			(*ndo_get_tstamp)(struct net_device *dev,
>  						  const struct skb_shared_hwtstamps *hwtstamps,
>  						  bool cycles);
> +	bool			(*ndo_xdp_rx_timestamp_supported)(const struct xdp_md *ctx);
> +	u64			(*ndo_xdp_rx_timestamp)(const struct xdp_md *ctx);
> +	bool			(*ndo_xdp_rx_hash_supported)(const struct xdp_md *ctx);
> +	u32			(*ndo_xdp_rx_hash)(const struct xdp_md *ctx);
>  };

Is this on the fast path? Can we do an indirection?
Put these ops in their own struct and add a pointer to that struct 
in net_device_ops? Purely for grouping reasons because the netdev
ops are getting orders of magnitude past the size where you can
actually find stuff in this struct.

>  	bpf_free_used_maps(aux);
>  	bpf_free_used_btfs(aux);
> -	if (bpf_prog_is_offloaded(aux))
> +	if (bpf_prog_is_dev_bound(aux))
>  		bpf_prog_offload_destroy(aux->prog);

This also looks a touch like a mix of terms (condition vs function
called).

> +static int __bpf_offload_init(void);
> +static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
> +					     struct net_device *netdev);
> +static void __bpf_offload_dev_netdev_unregister(struct bpf_offload_dev *offdev,
> +						struct net_device *netdev);

fwd declarations are yuck

>  static int bpf_dev_offload_check(struct net_device *netdev)
>  {
>  	if (!netdev)
> @@ -87,13 +93,17 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
>  	    attr->prog_type != BPF_PROG_TYPE_XDP)
>  		return -EINVAL;
>  
> -	if (attr->prog_flags)
> +	if (attr->prog_flags & ~BPF_F_XDP_HAS_METADATA)
>  		return -EINVAL;
>  
>  	offload = kzalloc(sizeof(*offload), GFP_USER);
>  	if (!offload)
>  		return -ENOMEM;
>  
> +	err = __bpf_offload_init();
> +	if (err)
> +		return err;

leaks offload

> @@ -209,6 +233,19 @@ bpf_prog_offload_remove_insns(struct bpf_verifier_env *env, u32 off, u32 cnt)
>  	up_read(&bpf_devs_lock);
>  }
>  
> +static void maybe_remove_bound_netdev(struct net_device *dev)
> +{

func name prefix ?

> -struct bpf_offload_dev *
> -bpf_offload_dev_create(const struct bpf_prog_offload_ops *ops, void *priv)
> +static int __bpf_offload_init(void)
>  {
> -	struct bpf_offload_dev *offdev;
>  	int err;
>  
>  	down_write(&bpf_devs_lock);
> @@ -680,12 +740,25 @@ bpf_offload_dev_create(const struct bpf_prog_offload_ops *ops, void *priv)
>  		err = rhashtable_init(&offdevs, &offdevs_params);
>  		if (err) {
>  			up_write(&bpf_devs_lock);
> -			return ERR_PTR(err);
> +			return err;
>  		}
>  		offdevs_inited = true;
>  	}
>  	up_write(&bpf_devs_lock);
>  
> +	return 0;
> +}

Would late_initcall() or some such not work for this?

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5b221568dfd4..862e03fcffa6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9228,6 +9228,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>  			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");

extack should get updated here, I reckon, maybe in previous patch

>  			return -EINVAL;
>  		}
> +		if (bpf_prog_is_dev_bound(new_prog->aux) && !bpf_offload_dev_match(new_prog, dev)) {

bound_dev_match() ?

> +			NL_SET_ERR_MSG(extack, "Cannot attach to a different target device");

different than.. ?

> +			return -EINVAL;
> +		}
>  		if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) {
>  			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
>  			return -EINVAL;

  parent reply	other threads:[~2022-12-08  5:00 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-06  2:45 [xdp-hints] [PATCH bpf-next v3 00/12] xdp: hints via kfuncs Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 01/12] bpf: Document XDP RX metadata Stanislav Fomichev
2022-12-08  4:25   ` [xdp-hints] " Jakub Kicinski
2022-12-08 19:06     ` Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 02/12] bpf: Rename bpf_{prog,map}_is_dev_bound to is_offloaded Stanislav Fomichev
2022-12-08  4:26   ` [xdp-hints] " Jakub Kicinski
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs Stanislav Fomichev
2022-12-07  4:29   ` [xdp-hints] " Alexei Starovoitov
2022-12-07  4:52     ` Stanislav Fomichev
2022-12-07  7:23       ` Martin KaFai Lau
2022-12-07 18:05         ` Stanislav Fomichev
2022-12-08  2:47   ` Martin KaFai Lau
2022-12-08 19:07     ` Stanislav Fomichev
2022-12-08 22:53       ` Martin KaFai Lau
2022-12-08 23:45         ` Stanislav Fomichev
2022-12-08  5:00   ` Jakub Kicinski [this message]
2022-12-08 19:07     ` Stanislav Fomichev
2022-12-09  1:30       ` Jakub Kicinski
2022-12-09  2:57         ` Stanislav Fomichev
2022-12-08 22:39   ` Toke Høiland-Jørgensen
2022-12-08 23:46     ` Stanislav Fomichev
2022-12-09  0:07       ` Toke Høiland-Jørgensen
2022-12-09  2:57         ` Stanislav Fomichev
2022-12-10  0:42           ` Martin KaFai Lau
2022-12-10  1:12             ` Martin KaFai Lau
2022-12-09 11:10   ` Jesper Dangaard Brouer
2022-12-09 17:47     ` Stanislav Fomichev
2022-12-11 11:09       ` Jesper Dangaard Brouer
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 04/12] veth: Introduce veth_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 05/12] veth: Support RX XDP metadata Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 06/12] selftests/bpf: Verify xdp_metadata xdp->af_xdp path Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 07/12] mlx4: Introduce mlx4_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-12-08  6:11   ` [xdp-hints] " Tariq Toukan
2022-12-08 19:07     ` Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 08/12] mxl4: Support RX XDP metadata Stanislav Fomichev
2022-12-08  6:09   ` [xdp-hints] " Tariq Toukan
2022-12-08 19:07     ` Stanislav Fomichev
2022-12-08 20:23       ` Tariq Toukan
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 09/12] xsk: Add cb area to struct xdp_buff_xsk Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 10/12] mlx5: Introduce mlx5_xdp_buff wrapper for xdp_buff Stanislav Fomichev
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 11/12] mlx5: Support RX XDP metadata Stanislav Fomichev
2022-12-08 22:59   ` [xdp-hints] " Toke Høiland-Jørgensen
2022-12-08 23:45     ` Stanislav Fomichev
2022-12-09  0:02       ` Toke Høiland-Jørgensen
2022-12-09  0:07         ` Alexei Starovoitov
2022-12-09  0:29           ` Toke Høiland-Jørgensen
2022-12-09  0:32             ` Alexei Starovoitov
2022-12-09  0:53               ` Toke Høiland-Jørgensen
2022-12-09  2:57                 ` Stanislav Fomichev
2022-12-09  5:24                   ` Saeed Mahameed
2022-12-09 12:59                     ` Jesper Dangaard Brouer
2022-12-09 14:37                       ` Toke Høiland-Jørgensen
2022-12-09 15:19                       ` Dave Taht
2022-12-09 14:42                   ` Toke Høiland-Jørgensen
2022-12-09 16:45                     ` Jakub Kicinski
2022-12-09 17:46                       ` Stanislav Fomichev
2022-12-09 22:13                         ` Jakub Kicinski
2022-12-06  2:45 ` [xdp-hints] [PATCH bpf-next v3 12/12] selftests/bpf: Simple program to dump XDP RX metadata Stanislav Fomichev
2022-12-08 22:28 ` [xdp-hints] Re: [PATCH bpf-next v3 00/12] xdp: hints via kfuncs Toke Høiland-Jørgensen
2022-12-08 23:47   ` Stanislav Fomichev
2022-12-09  0:14     ` Toke Høiland-Jørgensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221207210019.41dc9b6b@kernel.org \
    --to=kuba@kernel.org \
    --cc=alexandr.lobakin@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=magnus.karlsson@gmail.com \
    --cc=martin.lau@linux.dev \
    --cc=mtahhan@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox