Re: [PATCH net-next v8 5/6] selftests: net: Add busy_poll_test

[Joe Damato <jdamato@fastly.com>]

On Fri, Nov 08, 2024 at 12:47:16PM -0500, Willem de Bruijn wrote:
> Joe Damato wrote:
> > On Fri, Nov 08, 2024 at 09:57:48AM -0500, Willem de Bruijn wrote:
> > > Joe Damato wrote:
> > > > Add an epoll busy poll test using netdevsim.
> > > > 
> > > > This test is comprised of:
> > > >   - busy_poller (via busy_poller.c)
> > > >   - busy_poll_test.sh which loads netdevsim, sets up network namespaces,
> > > >     and runs busy_poller to receive data and socat to send data.
> > > > 
> > > > The selftest tests two different scenarios:
> > > >   - busy poll (the pre-existing version in the kernel)
> > > >   - busy poll with suspend enabled (what this series adds)
> > > > 
> > > > The data transmit is a 1MiB temporary file generated from /dev/urandom
> > > > and the test is considered passing if the md5sum of the input file to
> > > > socat matches the md5sum of the output file from busy_poller.
> > > 
> > > Nice test.
> > > 
> > > Busy polling does not affect data integrity. Is the goal of this test
> > > mainly to get coverage, maybe observe if the process would stall
> > > indefinitely?
> > 
> > Just to get coverage and make sure data makes it from point A to
> > point B intact despite suspend being enabled.
> > 
> > The last paragraph of the commit message highlights that netdevsim
> > functionality is limited, so the test uses what is available. It can
> > be extended in the future, when netdevsim supports more
> > functionality.
> > 
> > Paolo wanted a test and this is the best test we can provide given
> > the limitations of the testing environment.
> > 
> > > > netdevsim was chosen instead of veth due to netdevsim's support for
> > > > netdev-genl.
> > > > 
> > > > For now, this test uses the functionality that netdevsim provides. In the
> > > > future, perhaps netdevsim can be extended to emulate device IRQs to more
> > > > thoroughly test all pre-existing kernel options (like defer_hard_irqs)
> > > > and suspend.
> > 
> > [...]
> > 
> > The rest of the feedback below seems pretty minor; I don't think
> > it's worth spinning a v9 and re-sending just for this.
> > 
> > If anything this can be handled with a clean up commit in the
> > future.
> 
> FWIW no objections from me.

Thanks.
 
> > Jakub: please let me know if you prefer to see a v9 for this?