Re: [PATCH net-next v4 3/8] net/funeth: probing and netdev ops

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 5 Jan 2022 07:52:21 -0800 Dimitris Michailidis wrote:
> On Tue, Jan 4, 2022 at 6:07 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon,  3 Jan 2022 22:46:52 -0800 Dimitris Michailidis wrote:  
> > > This is the first part of the Fungible ethernet driver. It deals with
> > > device probing, net_device creation, and netdev ops.  
> >  
> > > +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.

> > > +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.