Re: [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's

[Daniel Latypov <dlatypov@google.com>]

On Thu, Apr 7, 2022 at 10:17 PM David Gow <davidgow@google.com> wrote:
>
> On Fri, Apr 8, 2022 at 3:39 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Context:
> > When using a non-UML arch, kunit.py will boot the test kernel with these
> > options by default:
> > > mem=1G console=tty kunit_shutdown=halt console=ttyS0 kunit_shutdown=reboot
> >
> > For QEMU, we need to use 'reboot', and for UML we need to use 'halt'.
> > If you switch them, kunit.py will hang until the --timeout expires.
> >
> > So the code currently unconditionally adds 'kunit_shutdown=halt' but
> > then appends 'reboot' when using QEMU (which overwrites it).
> >
> > This patch:
> > Having these duplicate options is a bit noisy.
> > Switch so we only add 'halt' for UML.
> >
> > I.e. we now get
> > UML: 'mem=1G console=tty console=ttyS0 kunit_shutdown=halt'
> > QEMU: 'mem=1G console=tty console=ttyS0 kunit_shutdown=reboot'
> >
> > Side effect: you can't overwrite kunit_shutdown on UML w/ --kernel_arg.
> > But you already couldn't for QEMU, and why would you want to?
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Thanks so much for fixing this: it had been quietly bugging me for a while.
>
> This looks pretty good as is, but I have a few suggestions for
> extending it which could be nice to have. I've put them inline below.
>
> Either way,
> Reviewed-by: David Gow <davidgow@google.com>
>
> >  tools/testing/kunit/kunit_kernel.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > index 483f78e15ce9..9731ceb7ad92 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -158,7 +158,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> >         def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> >                 """Runs the Linux UML binary. Must be named 'linux'."""
> >                 linux_bin = os.path.join(build_dir, 'linux')
> > -               return subprocess.Popen([linux_bin] + params,
> > +               return subprocess.Popen([linux_bin] + params + ['kunit_shutdown=halt'],
>
> I'd slightly prefer it if we assigned these extra parameters to a
> separate variable, rather than including them directly in the
> subprocess.Popen call.

I'm not sure I understand the suggestion here.
But PTAL at the diff down below and see if that looks fine.
I'll send a v2 that moves all of the default kernel args into UML only
as they're UML-specific, as you pointed out.

>
> (One thing I'd like to do is to print out the command we're running,
> which we do for Qemu, and having it in a variable that's passed in
> would be convenient. I don't expect this patch to do that, but having
> these parameters separate would make that future diff a little
> smaller.)
>
> >                                            stdin=subprocess.PIPE,
> >                                            stdout=subprocess.PIPE,
> >                                            stderr=subprocess.STDOUT,
> > @@ -332,7 +332,7 @@ class LinuxSourceTree(object):
> >         def run_kernel(self, args=None, build_dir='', filter_glob='', timeout=None) -> Iterator[str]:
> >                 if not args:
> >                         args = []
> > -               args.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
> > +               args.extend(['mem=1G', 'console=tty'])
>
> Does it make sense to also make these options UML only.

Yes, I think that's entirely correct.

mem=1G is redundant w/ the hard-coded '-m 1024' we pass to QEMU.
console=tty is overwritten by every architecture via its qemu_config.

So this patch would be better as

diff --git a/tools/testing/kunit/kunit_kernel.py
b/tools/testing/kunit/kunit_kernel.py
index 483f78e15ce9..d497adcd0684 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -158,6 +158,7 @@ class
LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
        def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
                """Runs the Linux UML binary. Must be named 'linux'."""
                linux_bin = os.path.join(build_dir, 'linux')
+               params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
                return subprocess.Popen([linux_bin] + params,
                                           stdin=subprocess.PIPE,
                                           stdout=subprocess.PIPE,
@@ -332,7 +333,6 @@ class LinuxSourceTree(object):
        def run_kernel(self, args=None, build_dir='', filter_glob='',
timeout=None) -> Iterator[str]:
                if not args:
                        args = []
-               args.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt'])
                if filter_glob:
                        args.append('kunit.filter_glob='+filter_glob)

>
> Under Qemu, the amount of memory is already passed separately to qemu,
> so adding another limit here seems counterproductive. If an
> architecture particularly needs it, we can add it to the
> per-architecture config.
>

On that note, sparc has a mem kernel_cmdline option.
tools/testing/kunit/qemu_configs/alpha.py: kernel_command_line='console=ttyS0',
tools/testing/kunit/qemu_configs/arm64.py:
kernel_command_line='console=ttyAMA0',
tools/testing/kunit/qemu_configs/arm.py: kernel_command_line='console=ttyAMA0',
tools/testing/kunit/qemu_configs/i386.py: kernel_command_line='console=ttyS0',
tools/testing/kunit/qemu_configs/powerpc.py:
kernel_command_line='console=ttyS0',
tools/testing/kunit/qemu_configs/riscv.py: kernel_command_line='console=ttyS0',
tools/testing/kunit/qemu_configs/s390.py: kernel_command_line='console=ttyS0',
tools/testing/kunit/qemu_configs/sparc.py:
kernel_command_line='console=ttyS0 mem=256M',
tools/testing/kunit/qemu_configs/x86_64.py: kernel_command_line='console=ttyS0',

Not sure why we'd do that atm.