Re: [PATCH bpf-next v4 03/15] bpf: Introduce device-bound XDP programs

On 12/12/22 6:35 PM, Stanislav Fomichev wrote:

New flag BPF_F_XDP_DEV_BOUND_ONLY plus all the infra to have a way
to associate a netdev with a BPF program at load time.

Some existing 'offloaded' routines are renamed to 'dev_bound' for
consistency with the rest.

Also moved a bunch of code around to avoid forward declarations.

There are too many things in one patch.  It becomes quite hard to follow, eg. I 
have to go back-and-forth a few times within this patch to confirm what change 
is just a move.  Please put the "moved a bunch of code around to avoid forward 
declarations" in one individual patch and also the 
"late_initcall(bpf_offload_init)" change in another individual patch.

[ ... ]

-int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
+static int __bpf_offload_dev_netdev_register(struct bpf_offload_dev *offdev,
+					     struct net_device *netdev)
+{
+	struct bpf_offload_netdev *ondev;
+	int err;
+
+	ondev = kzalloc(sizeof(*ondev), GFP_KERNEL);
+	if (!ondev)
+		return -ENOMEM;
+
+	ondev->netdev = netdev;
+	ondev->offdev = offdev;
+	INIT_LIST_HEAD(&ondev->progs);
+	INIT_LIST_HEAD(&ondev->maps);
+
+	err = rhashtable_insert_fast(&offdevs, &ondev->l, offdevs_params);
+	if (err) {
+		netdev_warn(netdev, "failed to register for BPF offload\n");
+		goto err_unlock_free;
+	}
+
+	if (offdev)
+		list_add(&ondev->offdev_netdevs, &offdev->netdevs);
+	return 0;
+
+err_unlock_free:
+	up_write(&bpf_devs_lock);

No need to handle bpf_devs_lock in the "__" version of the register() helper? 
The goto label probably also needs another name, eg. "err_free".

+	kfree(ondev);
+	return err;
+}
+

[ ... ]

nit.  A bit confusing because the "ondev = bpf_offload_find_netdev(...)" above 
should not fail but "!ondev" is tested again here.  I think the intention is to 
fail on the 'bpf_prog_is_offloaded() == true' case. May be:

		if (bpf_prog_is_offloaded(prog->aux)) {
			err = -EINVAL;
			goto err_unlock;
		}
		/* When only binding to the device, explicitly
		 * ...
		 */
		err = __bpf_offload_dev_netdev_register(NULL, offload->netdev);
		if (err)
			goto err_unlock;
		ondev = bpf_offload_find_netdev(offload->netdev);

hmm....list_empty(&ondev->progs) is tested here but will it be empty? ...

+		__bpf_offload_dev_netdev_unregister(NULL, dev);
  }
  
-void bpf_prog_offload_destroy(struct bpf_prog *prog)
+void bpf_prog_dev_bound_destroy(struct bpf_prog *prog)
  {
+	rtnl_lock();
  	down_write(&bpf_devs_lock);
-	if (prog->aux->offload)
-		__bpf_prog_offload_destroy(prog);
+	if (prog->aux->offload) {
+		bpf_dev_bound_try_remove_netdev(prog->aux->offload->netdev);

... the "prog" here is still linked to ondev->progs, right?
because __bpf_prog_dev_bound_destroy() is called later below.

nit. May be the bpf_dev_bound_try_remove_netdev() should be folded/merged back 
into bpf_prog_dev_bound_destroy() to make things more clear.

+		__bpf_prog_dev_bound_destroy(prog); > +	}
  	up_write(&bpf_devs_lock);
+	rtnl_unlock();
  }

[ ... ]

+static int __init bpf_offload_init(void)
+{
+	int err;
+
+	down_write(&bpf_devs_lock);

lock is probably not needed.

+	err = rhashtable_init(&offdevs, &offdevs_params);
+	up_write(&bpf_devs_lock);
+
+	return err;
+}
+
+late_initcall(bpf_offload_init);

[ ... ]

Does it matter if bpf_dev_bound_netdev_unregister(dev) is called before 
dev_xdp_uninstall(dev)?  Asking because it seems more logic to unregister dev 
after detaching xdp progs.

  		dev_xdp_uninstall(dev);
  
  		netdev_offload_xstats_disable_all(dev);