* [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's @ 2022-04-07 19:39 Daniel Latypov 2022-04-08 3:16 ` David Gow 0 siblings, 1 reply; 3+ messages in thread From: Daniel Latypov @ 2022-04-07 19:39 UTC (permalink / raw) To: brendanhiggins, davidgow Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov 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> --- 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'], 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']) if filter_glob: args.append('kunit.filter_glob='+filter_glob) base-commit: b04d1a8dc7e7ff7ca91a20bef053bcc04265d83a -- 2.35.1.1178.g4f1659d476-goog ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's 2022-04-07 19:39 [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's Daniel Latypov @ 2022-04-08 3:16 ` David Gow 2022-04-08 17:38 ` Daniel Latypov 0 siblings, 1 reply; 3+ messages in thread From: David Gow @ 2022-04-08 3:16 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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. (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. 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. And console=tty is overridden by console=ttyS0 on x86_64 anyway, so that also seems like it should be taken out. I tried commenting this line out entirely, and at least x86_64 still worked. > if filter_glob: > args.append('kunit.filter_glob='+filter_glob) > > > base-commit: b04d1a8dc7e7ff7ca91a20bef053bcc04265d83a > -- > 2.35.1.1178.g4f1659d476-goog > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's 2022-04-08 3:16 ` David Gow @ 2022-04-08 17:38 ` Daniel Latypov 0 siblings, 0 replies; 3+ messages in thread From: Daniel Latypov @ 2022-04-08 17:38 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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. ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-08 17:38 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-07 19:39 [PATCH] kunit: tool: cosmetic: don't specify duplicate kunit_shutdown's Daniel Latypov 2022-04-08 3:16 ` David Gow 2022-04-08 17:38 ` Daniel Latypov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.