On Wed, 5 Jan 2022 07:52:21 -0800 Dimitris Michailidis wrote:

On Tue, Jan 4, 2022 at 6:07 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Mon,  3 Jan 2022 22:46:52 -0800 Dimitris Michailidis wrote:  

quoted

This is the first part of the Fungible ethernet driver. It deals with
device probing, net_device creation, and netdev ops.  

quoted

+static int fun_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
+{
+     struct bpf_prog *old_prog, *prog = xdp->prog;
+     struct funeth_priv *fp = netdev_priv(dev);
+     bool reconfig;
+     int rc, i;
+
+     /* XDP uses at most one buffer */
+     if (prog && dev->mtu > XDP_MAX_MTU) {
+             netdev_err(dev, "device MTU %u too large for XDP\n", dev->mtu);
+             NL_SET_ERR_MSG_MOD(xdp->extack,
+                                "Device MTU too large for XDP");
+             return -EINVAL;
+     }
+
+     reconfig = netif_running(dev) && (!!fp->xdp_prog ^ !!prog);
+     if (reconfig) {
+             rc = funeth_close(dev);  

Please rework runtime reconfig to not do the close and then open thing.
This will prevent users from reconfiguring their NICs at runtime.
You should allocate the resources first, then take the datapath down,
reconfigure, swap and free the old resources.  

I imagine you have in mind something like nfp_net_ring_reconfig() but that
doesn't work as well here. We have the linux part of the data path (ring memory,
interrupts, etc) and the device part, handled by FW. I can't clone the device
portion for a quick swap during downtime. Since it involves messages to FW
updating the device portion is by far the bulk of the work and it needs to be
during the downtime. Doing Linux allocations before downtime offers little
improvement I think.

It does - real machines running real workloads will often be under
memory pressure. I've even seen XDP enable / disable fail just due 
to memory fragmentation, with plenty free memory when device rings
are large.

There is ongoing work for FW to be able to modify live queues. When that
is available I expect this function will be able to move in and out of XDP with
no downtime.

quoted

quoted

+static void fun_destroy_netdev(struct net_device *netdev)
+{
+     if (likely(netdev)) {  

defensive programming?  

Looks that way but I'd rather have this function work with any input.

There's way too much defensive programming in this driver. Unless there
is a legit code path which can pass netdev == NULL you should remove
the check.