Re: [PATCH net v1 2/2] lan743x: boost performance: limit PCIe bandwidth requirement

[Jakub Kicinski]

On Tue, 8 Dec 2020 16:54:33 -0500 Sven Van Asbroeck wrote:
> > > Tested with iperf3 on a freescale imx6 + lan7430, both sides
> > > set to mtu 1500 bytes.
> > >
> > > Before:
> > > [ ID] Interval           Transfer     Bandwidth       Retr
> > > [  4]   0.00-20.00  sec   483 MBytes   203 Mbits/sec    0
> > > After:
> > > [ ID] Interval           Transfer     Bandwidth       Retr
> > > [  4]   0.00-20.00  sec  1.15 GBytes   496 Mbits/sec    0
> > >
> > > And with both sides set to MTU 9000 bytes:
> > > Before:
> > > [ ID] Interval           Transfer     Bandwidth       Retr
> > > [  4]   0.00-20.00  sec  1.87 GBytes   803 Mbits/sec   27
> > > After:
> > > [ ID] Interval           Transfer     Bandwidth       Retr
> > > [  4]   0.00-20.00  sec  1.98 GBytes   849 Mbits/sec    0
> > >
> > > Tested-by: Sven Van Asbroeck <thesven73@xxxxxxxxx> # lan7430
> > > Signed-off-by: Sven Van Asbroeck <thesven73@xxxxxxxxx>  
> >
> > This is a performance improvement, not a fix, it really needs to target
> > net-next.  
> 
> I thought it'd be cool if 'historic' kernels could benefit from this performance
> improvement too, but yeah if it's against policy it should go into net-next.
> 
> What about the other patch in the patchset (ping-pong). Should it go into
> net-next as well?

The jury is out on that one. Using ring size for netif_napi_add() 
and updating RX_TAIL at the end of NAPI is pretty broken. So that
one can qualify as a fix IMHO.

> > > @@ -2632,9 +2633,13 @@ static int lan743x_netdev_change_mtu(struct net_device *netdev, int new_mtu)
> > >       struct lan743x_adapter *adapter = netdev_priv(netdev);
> > >       int ret = 0;
> > >
> > > +     if (netif_running(netdev))
> > > +             return -EBUSY;  
> >
> > That may cause a regression to users of the driver who expect to be
> > able to set the MTU when the device is running. You need to disable
> > the NAPI, pause the device, swap the buffers for smaller / bigger ones
> > and restart the device.  
> 
> That's what I tried first, but I quickly ran into a spot of trouble:
> restarting the device may fail (unlikely but possible). So when the user tries
> to change the mtu and that errors out, they might end up with a stopped device.
> Is that acceptable behaviour? If so, I'll add it to the patch.

Fail because of memory allocation failures?

The best way to work around that is to allocate all the memory for new
configuration before you free the old memory. This also makes the
change a lot less disturbing to the traffic because you can do all the
allocations before the device is disabled, do the swap, start the
device, and then free the old set.