fix: address code review findings (HIGH + MEDIUM)
- Replace GNU \b with portable word-boundary sed patterns in kernel-params - Warn on unknown CLI arguments instead of silently swallowing - Add floor check on recommended_gttsize_mib to prevent negative values - Fix Python operator precedence in benchmark log parser - Add root checks to tuned-profile.sh and rollback.sh - Remove redundant sudo calls (scripts already require root at entry) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -186,6 +186,8 @@ recommended_gttsize_mib() {
|
|||||||
total_kb="$(detect_total_physical_ram_kb)"
|
total_kb="$(detect_total_physical_ram_kb)"
|
||||||
local total_gib=$(( total_kb / 1024 / 1024 ))
|
local total_gib=$(( total_kb / 1024 / 1024 ))
|
||||||
local gtt_gib=$(( total_gib - 4 ))
|
local gtt_gib=$(( total_gib - 4 ))
|
||||||
|
# Floor at 1 GiB to avoid negative or zero values
|
||||||
|
(( gtt_gib < 1 )) && gtt_gib=1
|
||||||
echo $(( gtt_gib * 1024 ))
|
echo $(( gtt_gib * 1024 ))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -155,7 +155,7 @@ for logfile in sorted(result_dir.glob("*.log")):
|
|||||||
# Parse the pipe-delimited llama-bench table
|
# Parse the pipe-delimited llama-bench table
|
||||||
for line in content.splitlines():
|
for line in content.splitlines():
|
||||||
line = line.strip()
|
line = line.strip()
|
||||||
if not line.startswith("|") or "model" in line.lower() and "size" in line.lower():
|
if not line.startswith("|") or ("model" in line.lower() and "size" in line.lower()):
|
||||||
continue
|
continue
|
||||||
if "---" in line:
|
if "---" in line:
|
||||||
continue
|
continue
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ while [[ $# -gt 0 ]]; do
|
|||||||
--tag|-t) TAG="$2"; shift 2 ;;
|
--tag|-t) TAG="$2"; shift 2 ;;
|
||||||
--backends|-b) BACKENDS_FILTER="$2"; shift 2 ;;
|
--backends|-b) BACKENDS_FILTER="$2"; shift 2 ;;
|
||||||
--models|-m) MODELS_FILTER="$2"; shift 2 ;;
|
--models|-m) MODELS_FILTER="$2"; shift 2 ;;
|
||||||
*) shift ;;
|
*) log_warn "Unknown argument: $1"; shift ;;
|
||||||
esac
|
esac
|
||||||
done
|
done
|
||||||
|
|
||||||
@@ -147,7 +147,7 @@ for logfile in sorted(result_dir.glob("*.log")):
|
|||||||
continue
|
continue
|
||||||
for line in content.splitlines():
|
for line in content.splitlines():
|
||||||
line = line.strip()
|
line = line.strip()
|
||||||
if not line.startswith("|") or "model" in line.lower() and "size" in line.lower():
|
if not line.startswith("|") or ("model" in line.lower() and "size" in line.lower()):
|
||||||
continue
|
continue
|
||||||
if "---" in line:
|
if "---" in line:
|
||||||
continue
|
continue
|
||||||
|
|||||||
@@ -13,7 +13,7 @@ while [[ $# -gt 0 ]]; do
|
|||||||
case "$1" in
|
case "$1" in
|
||||||
--simple|-s) SIMPLE=true; shift ;;
|
--simple|-s) SIMPLE=true; shift ;;
|
||||||
--with-logging|-l) WITH_LOG=true; shift ;;
|
--with-logging|-l) WITH_LOG=true; shift ;;
|
||||||
*) shift ;;
|
*) log_warn "Unknown argument: $1"; shift ;;
|
||||||
esac
|
esac
|
||||||
done
|
done
|
||||||
|
|
||||||
|
|||||||
@@ -93,12 +93,12 @@ with open(os.environ["GRUB_PATH"]) as f:
|
|||||||
print("")
|
print("")
|
||||||
')"
|
')"
|
||||||
|
|
||||||
# Remove any existing values of these params
|
# Remove any existing values of these params (portable, no GNU \b)
|
||||||
new_cmdline="$current_cmdline"
|
new_cmdline="$current_cmdline"
|
||||||
new_cmdline="$(echo "$new_cmdline" | sed -E 's/\biommu=[^ ]*//g')"
|
new_cmdline="$(echo " $new_cmdline " | sed -E 's/ iommu=[^ ]*/ /g')"
|
||||||
new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bamd_iommu=[^ ]*//g')"
|
new_cmdline="$(echo " $new_cmdline " | sed -E 's/ amd_iommu=[^ ]*/ /g')"
|
||||||
new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bamdgpu\.gttsize=[^ ]*//g')"
|
new_cmdline="$(echo " $new_cmdline " | sed -E 's/ amdgpu\.gttsize=[^ ]*/ /g')"
|
||||||
new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bttm\.pages_limit=[^ ]*//g')"
|
new_cmdline="$(echo " $new_cmdline " | sed -E 's/ ttm\.pages_limit=[^ ]*/ /g')"
|
||||||
# Clean up extra spaces
|
# Clean up extra spaces
|
||||||
new_cmdline="$(echo "$new_cmdline" | xargs)"
|
new_cmdline="$(echo "$new_cmdline" | xargs)"
|
||||||
|
|
||||||
|
|||||||
@@ -10,6 +10,11 @@ BACKUP_DIR="$(data_dir backups)"
|
|||||||
|
|
||||||
log_header "Rollback Optimizations"
|
log_header "Rollback Optimizations"
|
||||||
|
|
||||||
|
if [[ $EUID -ne 0 ]]; then
|
||||||
|
log_error "This script requires root. Re-run with: sudo make rollback"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
# ── 1. GRUB rollback ────────────────────────────────────
|
# ── 1. GRUB rollback ────────────────────────────────────
|
||||||
log_info "GRUB backups:"
|
log_info "GRUB backups:"
|
||||||
mapfile -t grub_backups < <(find "$BACKUP_DIR" -name 'grub-*.bak' -print 2>/dev/null | sort -r)
|
mapfile -t grub_backups < <(find "$BACKUP_DIR" -name 'grub-*.bak' -print 2>/dev/null | sort -r)
|
||||||
@@ -23,7 +28,7 @@ else
|
|||||||
echo ""
|
echo ""
|
||||||
|
|
||||||
if confirm "Restore most recent GRUB backup?"; then
|
if confirm "Restore most recent GRUB backup?"; then
|
||||||
require_root
|
# root already checked at script start
|
||||||
backup="${grub_backups[0]}"
|
backup="${grub_backups[0]}"
|
||||||
cp "$backup" "$GRUB_FILE"
|
cp "$backup" "$GRUB_FILE"
|
||||||
log_success "GRUB restored from: $backup"
|
log_success "GRUB restored from: $backup"
|
||||||
@@ -54,7 +59,7 @@ if [[ -f "$prev_profile_file" ]]; then
|
|||||||
log_info "Tuned profile: $current (previous: $prev_profile)"
|
log_info "Tuned profile: $current (previous: $prev_profile)"
|
||||||
|
|
||||||
if [[ "$current" != "$prev_profile" ]] && confirm "Restore tuned profile to $prev_profile?"; then
|
if [[ "$current" != "$prev_profile" ]] && confirm "Restore tuned profile to $prev_profile?"; then
|
||||||
sudo tuned-adm profile "$prev_profile"
|
tuned-adm profile "$prev_profile"
|
||||||
log_success "Tuned profile restored to: $prev_profile"
|
log_success "Tuned profile restored to: $prev_profile"
|
||||||
fi
|
fi
|
||||||
else
|
else
|
||||||
|
|||||||
@@ -10,6 +10,11 @@ RECOMMENDED="accelerator-performance"
|
|||||||
|
|
||||||
log_header "Tuned Profile Optimization"
|
log_header "Tuned Profile Optimization"
|
||||||
|
|
||||||
|
if [[ $EUID -ne 0 ]]; then
|
||||||
|
log_error "This script requires root. Re-run with: sudo make optimize-tuned"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
if ! is_cmd tuned-adm; then
|
if ! is_cmd tuned-adm; then
|
||||||
log_error "tuned is not installed. Install with: sudo dnf install tuned"
|
log_error "tuned is not installed. Install with: sudo dnf install tuned"
|
||||||
exit 1
|
exit 1
|
||||||
@@ -46,7 +51,7 @@ fi
|
|||||||
# Save current for rollback
|
# Save current for rollback
|
||||||
echo "$current" > "$(data_dir backups)/tuned-previous-profile.txt"
|
echo "$current" > "$(data_dir backups)/tuned-previous-profile.txt"
|
||||||
|
|
||||||
sudo tuned-adm profile "$RECOMMENDED"
|
tuned-adm profile "$RECOMMENDED"
|
||||||
|
|
||||||
new_profile="$(detect_tuned_profile)"
|
new_profile="$(detect_tuned_profile)"
|
||||||
if [[ "$new_profile" == "$RECOMMENDED" ]]; then
|
if [[ "$new_profile" == "$RECOMMENDED" ]]; then
|
||||||
|
|||||||
Reference in New Issue
Block a user