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

HPET clocksource is forbidden #82

Closed
wants to merge 1 commit into from
Closed

HPET clocksource is forbidden #82

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions bash/library/bench-base
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,29 +95,29 @@ function validate_sw_prereqs() {
}

function validate_clocksource() {
local clocksource clocksource_dir available_clocksources
local clocksource clocksource_dir available_clocksources forbidden_clocksources

clocksource_dir="/sys/devices/system/clocksource/clocksource0"

if pushd ${clocksource_dir} > /dev/null; then
clocksource=$(cat current_clocksource)
available_clocksources=$(cat available_clocksource)
supported_clocksources="tsc kvm-clock hyperv_clocksource_tsc_page"
clocksource="$(cat current_clocksource)"
available_clocksources="$(cat available_clocksource)"
forbidden_clocksources="hpet"
Comment on lines -103 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how I feel about switching from a list of explicitly supported clocksources to a list of explicitly forbidden ones. I suppose that explicity forbidden is easier to maintain, but it also means we could be running with a clocksource that we have not validated to actually be acceptable. Thoughts @atheurer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the purpose of checking what is available ?
If we have a hard coded list for supported_clocksources, there is no point in checking available ones in the clocksource dir.
The current code allows kvm and hyperv clocksources and even doesn't check if they are available.


popd > /dev/null

case "${clocksource}" in
"tsc"|"kvm-clock"|"hyperv_clocksource_tsc_page")
echo "Verified clocksource is ${clocksource}"
if [[ "${available_clocksources}" == *"${clocksource}"* ]]; then
if [[ "${forbidden_clocksources}" != *"${clocksource}"* ]]; then
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is good logic here.

The first line is making sure that the current clocksource is one of the available clock sources? That is a given, right? This just seems unnecessary.

The second line allows for partial matches which might not be a good thing at some point:

list="foo bar zorg"
item="mzorg"
if [[ "${list}" != *"${item}"* ]]; then echo match; else echo "no match"; fi

emits this when run: match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is good logic here.

The first line is making sure that the current clocksource is one of the available clock sources? That is a given, right? This just seems unnecessary.

The second line allows for partial matches which might not be a good thing at some point:

list="foo bar zorg"
item="mzorg"
if [[ "${list}" != *"${item}"* ]]; then echo match; else echo "no match"; fi

emits this when run: match

The two lines should check:
if available && !forbidden

But yes, you are right. It is giving false positive on partial matches for the forbidden list. I'll change that logic.

The tests go well until the substring test....

# forbidden="hpet"
# available="tsc foo bar"
# clock="tsc"

# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
ok
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
ok

# clock="hpet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
forbidden

# clock="new"
# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
unavailable

# clock="Xhpet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
ok
# if [[ "${available}" == *"${clock}"* ]]; then echo "ok"; else echo "unavailable"; fi
unavailable

The partial match problem (substring): clock=pet and forbidden=hpet

# clock="pet"
# if [[ "${forbidden}" != *"${clock}"* ]]; then echo "ok"; else echo "forbidden"; fi
forbidden

I'll fix this.

echo "Verified clocksource is ${clocksource}."
return 0
;;
*)
echo "ERROR: Unsupported clocksource. Current clocksource is ${clocksource}. Available clocksources are: ${available_clocksources}. Supported clocksources are: ${supported_clocksources}"
return 2
;;
esac
fi
fi
echo "ERROR: Unsupported clocksource. Current clocksource is ${clocksource}."
echo "Available clocksources are: ${available_clocksources}."
echo "Forbidden clocksources are: ${forbidden_clocksources}."
return 2
else
echo "ERROR: Could not pushd to ${clocksource_dir}"
echo "ERROR: Could not pushd to ${clocksource_dir} and verify clocksource."
return 1
fi
}
Loading