Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework CLI interface #263

Merged
merged 6 commits into from
Jan 21, 2022
Merged

Rework CLI interface #263

merged 6 commits into from
Jan 21, 2022

Conversation

mkroening
Copy link
Member

@mkroening mkroening commented Jan 1, 2022 •
edited
Loading

Reopens #243, which was closed accidentally.

This heavily reworks the CLI interface using StructOpt as well as Params for creating uhyve instances programmatically.

See the commit messages for details.

@codecov
Copy link

codecov bot commented Jan 1, 2022 •
edited
Loading

Codecov Report

Merging #263 (2612691) into master (eae6749) will decrease coverage by 0.70%.
The diff coverage is 24.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   64.15%   63.44%   -0.71%     
==========================================
  Files          16       16              
  Lines        2427     2380      -47     
==========================================
- Hits         1557     1510      -47     
  Misses        870      870
Impacted Files Coverage Δ
src/bin/uhyve.rs 0.50% <0.00%> (+0.03%) ⬆️
src/lib.rs 100.00% <ø> (ø)
src/vm.rs 76.17% <33.33%> (-2.62%) ⬇️
src/params.rs 67.16% <67.16%> (ø)
src/linux/uhyve.rs 66.47% <94.11%> (-0.85%) ⬇️
src/linux/vcpu.rs 70.35% <100.00%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eae6749...2612691. Read the comment docs.

@mkroening mkroening force-pushed the structopt branch 9 times, most recently from 80b368c to 246ba1c Compare January 2, 2022 16:27
@mkroening mkroening marked this pull request as ready for review January 2, 2022 18:52
@mkroening mkroening mentioned this pull request Jan 3, 2022
Closed
@mkroening mkroening force-pushed the structopt branch 7 times, most recently from 7f10994 to 3167651 Compare January 3, 2022 20:04
@stlankes
Copy link
Collaborator

stlankes commented Jan 3, 2022

@mkroening @jbreitbart @jounathaen are missing on the list of authors. Why do we enable "Kernel Samepage Merging" on default?

@mkroening mkroening mentioned this pull request Jan 3, 2022
Merged
@mkroening
Copy link
Member Author

I rebased the PR and opened #277.

We can discuss KSM with the rest of the CLI changes on friday, if you don't mind. :)

bors bot added a commit that referenced this pull request Jan 3, 2022
@bors @mkroening
Merge #277
eae6749 
277: Cargo.toml: Add authors r=stlankes a=mkroening

I added `@mkroening,` `@jbreitbart` and `@jounathaen` as requested in #263 (comment).

Co-authored-by: Martin Kröning <mkroening@posteo.net>
@stlankes
Copy link
Collaborator

stlankes commented Jan 3, 2022

I am on vacation :-)

@mkroening
Copy link
Member Author

Ah, I see. Have a nice vacation then! :D

Maybe I'll find some motivation in the next days to write some prose on all of these changes then. This PR should not be very urgent, right?

@mkroening
linux/uhyve: Use cmp::min
b6b1193
@mkroening
Remove memory size unit tests
80f9c2a 
These tests are obsolete once the guest memory minimum is enforced.

Replacing these tests is tracked in hermit-os#265.
@mkroening mkroening force-pushed the structopt branch 3 times, most recently from 82e75e2 to fbbeecb Compare January 7, 2022 21:09
@mkroening
Copy link
Member Author

mkroening commented Jan 7, 2022 •
edited
Loading

I expanded the commit messages. See those for details on each commit.

Regarding turning on KSM by default: As far as I know this matches QEMU's default behavior. Also as KSM has been designed especially for hypervisors, I see no reason not to enable KSM by default. Our --no-thp matches -redhat-disable-THP, --no-ksm matches -redhat-disable-KSM.

@mkroening mkroening mentioned this pull request Jan 10, 2022
Closed
@stlankes
Copy link
Collaborator

It takes time... For large VMs is this useful. For small binaries with small memory footprints, I would say, this is just overhead with nearly no benefit. But this is more my feeling....

@jbreitbart
Copy link
Collaborator

It takes time... For large VMs is this useful. For small binaries with small memory footprints, I would say, this is just overhead with nearly no benefit. But this is more my feeling....

Now if only someone would have added a way to benchmark functionality 😜

@mkroening
Copy link
Member Author

Very good point. I'll be doing some benchmarks then. :D

@mkroening
Copy link
Member Author

@stlankes, I did some benchmarks and further investigation into KSM.

The KSM daemon is disabled by default. Without having activated the KSM daemon, no overhead is measurable. In that case, the kernel only registers the memory range for whenever the KSM daemon is started in the future.

Activating the KSM daemon had no effect on hello_world. rusty-demo's runtime changed from 223.74 ms to 226.37 ms (an increase of 2.63 ms or 1,1 %).

Since the KSM daemon has to be enabled explicitly system-wide before having any effect, us advising KSM regions is a sensible default in my opinion. The user could still opt-out application-wise via --no-ksm.

What do you think?

@mkroening mkroening mentioned this pull request Jan 17, 2022
Closed
@mkroening
Copy link
Member Author

I'll compare the default KSM setting with Firecracker and QEMU's microvm.

@mkroening
Rework CLI interface with structopt
0384c1d 
* Have structs for args to modularize arg handling

* Improve argument docs

* Improve error messages on faulty args

* Colorize help output

* Rename `--disable-hugepages` to `--no-thp`
    * Corresponds to QEMU's `-redhat-disable-THP`
    * Remove `HERMIT_HUGEPAGES`

* Replace `--mergeable` with `--ksm`
    * Corresponds to QEMU's `-redhat-disable-KSM`
    * Remove `HERMIT_MERGEABLE`

* Remove `HERMIT_VERBOSE`

* Rename `-c, --cpus` to `-c, --cpu-count`
    * Rename `HERMIT_CPUS` to `HERMIT_CPU_COUNT`

* Rename `-s, --gdb_port` to `-s, --gdb-port`

* Rename `-m, --memsize` to `-m, --memory-size`
    * Rename `HERMIT_MEM` to `HERMIT_MEMORY_SIZE`

* Disable `--nic` since networking is currently not supported

```
uhyve 0.0.29
Stefan Lankes <slankes@eonerc.rwth-aachen.de>, Martin Kröning <mkroening@posteo.net>, Jens Breitbart
<jbreitbart@gmail.com>, Jonathan Klimt <jonathan.klimt@eonerc.rwth-aachen.de>
A minimal hypervisor for RustyHermit

USAGE:
    uhyve [OPTIONS] <KERNEL> [KERNEL_ARGS]...

ARGS:
    <KERNEL>
            The kernel to execute

    <KERNEL_ARGS>...
            Arguments to forward to the kernel

OPTIONS:
    -h, --help
            Print help information

    -s, --gdb-port <GDB_PORT>
            GDB server port

            Starts a GDB server on the provided port and waits for a connection.

            [env: HERMIT_GDB_PORT=]

    -v, --verbose
            Print kernel messages

    -V, --version
            Print version information

MEMORY:
        --ksm
            Kernel Samepage Merging

            Advise the kernel to enable Kernel Samepage Merging [KSM] on the virtual RAM.

            [KSM]: https://www.kernel.org/doc/html/latest/admin-guide/mm/ksm.html

    -m, --memory-size <MEMORY_SIZE>
            Guest RAM size

            [env: HERMIT_MEMORY_SIZE=]
            [default: "64.00 MiB"]

        --no-thp
            No Transparent Hugepages

            Don't advise the kernel to enable Transparent Hugepages [THP] on the virtual RAM.

            [THP]: https://www.kernel.org/doc/html/latest/admin-guide/mm/transhuge.html

CPU:
    -a, --affinity <CPUs>
            Bind guest vCPUs to host cpus

            A list of host CPU numbers onto which the guest vCPUs should be bound to obtain
            performance benefits. List items may be single numbers or inclusive ranges. List items
            may be separated with commas or spaces.

            # Examples

            * `--affinity "0 1 2"`

            * `--affinity 0-1,2`

    -c, --cpu-count <CPU_COUNT>
            Number of guest CPUs

            [env: HERMIT_CPU_COUNT=]
            [default: 1]
```

```
uhyve 0.0.29
Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Martin Kröning <mkroening@posteo.net>
Jens Breitbart <jbreitbart@gmail.com>
Jonathan Klimt <jonathan.klimt@eonerc.rwth-aachen.de>

USAGE:
    uhyve [FLAGS] [OPTIONS] <KERNEL> [ARGUMENTS]...

FLAGS:
        --disable-hugepages
            Disable the usage of huge pages

        --mergeable
            Enable kernel feature to merge same pages

    -v, --verbose
            Print also kernel messages

    -h, --help
            Prints help information

    -V, --version
            Prints version information

OPTIONS:
    -c, --cpus <CPUS>
            Number of guest processors [env: HERMIT_CPUS=]

    -a, --affinity <cpulist>
            A list of CPUs delimited by commas onto which
            					 the virtual CPUs should be bound. This may improve
            					performance.

    -s, --gdb_port <GDB_PORT>
            Enables GDB-Stub on given port [env: HERMIT_GDB_PORT=]

    -m, --memsize <MEM>
            Memory size of the guest [env: HERMIT_MEM=]

        --nic <NETIF>
            Name of the network interface [env: HERMIT_NETIF=]

ARGS:
    <KERNEL>
            Sets path to the kernel

    <ARGUMENTS>...
            Arguments of the unikernel
```
@mkroening
Rework Uhyve construction params
435b586 
This improves constructing Uhyve instances programmatically.
@mkroening
Params: Enable parameter only on supported OS
542ef80 
This disables GDB and network parameters on macos.
@mkroening
Supply kernel arguments as parameter
2612691 
This allows supplying kernel arguments programatically and unifies
argument parsing in the frontend using clap.
This avoids parsing kernel arguments from the environment
unconditionally.
@mkroening
Copy link
Member Author

I disabled KSM per default.

bors bot added a commit that referenced this pull request Jan 21, 2022
@bors @mkroening
Merge #263
1c6ae86 
263: Rework CLI interface r=mkroening a=mkroening

Reopens #243, which was closed accidentally.

This heavily reworks the CLI interface using StructOpt as well as `Params` for creating uhyve instances programmatically.

See the commit messages for details.

Co-authored-by: Martin Kröning <mkroening@posteo.net>
@hermit-os hermit-os deleted a comment from bors bot Jan 21, 2022
@mkroening
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 21, 2022

Build succeeded:

@bors bors bot merged commit 2d87f9c into hermit-os:master Jan 21, 2022
@mkroening mkroening deleted the structopt branch January 21, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stlankes stlankes Awaiting requested review from stlankes

@jounathaen jounathaen Awaiting requested review from jounathaen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkroening @stlankes @jbreitbart