Skip to content

Remove command-line options and configuration fields#1263

Merged
stevenengler merged 6 commits intoshadow:devfrom
stevenengler:remove-cpufreq-option
Apr 6, 2021
Merged

Remove command-line options and configuration fields#1263
stevenengler merged 6 commits intoshadow:devfrom
stevenengler:remove-cpufreq-option

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Apr 5, 2021

Removes the following:

Command-line options:

  • --preload
  • --valgrind
  • --test
  • --tgen
  • --cpu-precision
  • --cpu-threshold
  • --tcp-ssthresh
  • --tcp-windows

Configuration fields:

  • cpufrequency

Part of #1220.

@stevenengler stevenengler added Type: Enhancement New functionality or improved design Component: Main Composing the core Shadow executable labels Apr 5, 2021
@stevenengler stevenengler self-assigned this Apr 5, 2021
@stevenengler stevenengler force-pushed the remove-cpufreq-option branch from 6f28196 to 5bfccd4 Compare April 5, 2021 20:12
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 5, 2021

Codecov Report

Merging #1263 (c3716e9) into dev (a0ef5ac) will increase coverage by 0.06%.
The diff coverage is 47.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1263      +/-   ##
==========================================
+ Coverage   55.67%   55.74%   +0.06%     
==========================================
  Files         142      141       -1     
  Lines       20562    20479      -83     
  Branches     5053     5032      -21     
==========================================
- Hits        11448    11416      -32     
+ Misses       5988     5949      -39     
+ Partials     3126     3114      -12     
Flag Coverage Δ
tests 55.74% <47.36%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/core/main.c 56.84% <ø> (+12.57%) ⬆️
src/main/core/support/configuration.c 48.07% <ø> (+0.24%) ⬆️
src/main/core/support/options.c 62.44% <0.00%> (+0.54%) ⬆️
src/main/core/controller.c 60.00% <50.00%> (+1.63%) ⬆️
src/main/host/descriptor/tcp.c 78.58% <100.00%> (ø)
src/main/core/worker.c 75.37% <0.00%> (-1.00%) ⬇️
src/main/host/thread_ptrace.c 53.56% <0.00%> (-0.29%) ⬇️

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 a0ef5ac...c3716e9. Read the comment docs.

@stevenengler stevenengler requested a review from robgjansen April 5, 2021 20:39
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always happy to see a bunch of old code removed :)

params->cpuThreshold = defaultCPUThreshold > 0 ? defaultCPUThreshold : 0;
gint defaultCPUPrecision = options_getCPUPrecision(controller->options);
params->cpuPrecision = defaultCPUPrecision > 0 ? defaultCPUPrecision : 0;
params->cpuThreshold = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 0 or -1? It looks like -1 was the default before to disable cpu delays?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The params->cpuThreshold = defaultCPUThreshold > 0 ? defaultCPUThreshold : 0 line sets a lower limit of 0, so the -1 would end up as 0 here.

{ "socket-send-buffer", 0, 0, G_OPTION_ARG_INT, &(options->initialSocketSendBufferSize), socksend->str, "N" },
{ "tcp-congestion-control", 0, 0, G_OPTION_ARG_STRING, &(options->tcpCongestionControl), "Congestion control algorithm to use for TCP ('aimd', 'reno', 'cubic') ['reno']", "TCPCC" },
{ "tcp-ssthresh", 0, 0, G_OPTION_ARG_INT, &(options->tcpSlowStartThreshold), "Set TCP ssthresh value instead of discovering it via packet loss or hystart [0]", "N" },
{ "tcp-windows", 0, 0, G_OPTION_ARG_INT, &(options->initialTCPWindow), "Initialize the TCP send, receive, and congestion windows to N packets [10]", "N" },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getting started tutorial doc has an example that tweaks the --tcp-windows option. We should update that example experiment so it no longer references a non-existent option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this example to use different queuing disciplines instead. Let me know if you think there's a better option to use.

return options->shouldExitAfterShmCleanup;
}

const gchar* options_getPreloadString(Options* options) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, this wasn't called anywhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it looks like all uses were removed in the initial e41c4a9.

@stevenengler stevenengler force-pushed the remove-cpufreq-option branch from 52eea99 to c3716e9 Compare April 6, 2021 18:38
@stevenengler stevenengler merged commit a7419c2 into shadow:dev Apr 6, 2021
@stevenengler stevenengler deleted the remove-cpufreq-option branch April 6, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants