Fix PerfBuffer shutdown

[eyakubovich]

This is anlogous to 09b2b47 (which fixed RingBuffer).
It does a clean PerfBuffer stop by waiting for the
polling goroutine to finish its work.

Fixes #640

[yanivagman]

Thanks @eyakubovich!
LGTM
only two small comments

[yanivagman]

if
$MAJOR_CURRENT_VERSION > $MAJOR_VERSION_LIMIT -
there is no reason to check for minor, otherwise, version like 6.0 will not pass this test, right?

[yanivagman]

don't we want to exit with a value different from 0? If we do want to use exit 0 here, why not for the above major test as well?

[grantseltzer]

@yanivagman the minor/major checks look like they're just copied from the script I have for running the ringbuffer selftest so I can answer for them.

For your first comment on checking minor version after major, you're absolutely right. I'm going to have to fix this.

The reason we exit 0 is for the sake of the github action not failing. I picture having multiple environments where the github actions run, including older kernels. We should just not run the test if we don't expect the kernel to support it. I suppose we can return a particular exit code and have the test runner make exceptions for this scenario but we're not there yet.

[grantseltzer]

Suggested change

	for _ = range eventsChan {
	for range eventsChan {

[eyakubovich]

very nice! I didn't know about this variant of for.

[grantseltzer]

Suggested change

	for _ = range lostChan {
	for range lostChan {

[grantseltzer]

Suggested change

	MINOR_VERSION_LIMIT=$(echo $VERSION_LIMIT | cut -d '.' -f2)
	MINOR_CURRENT_VERSION=$(echo $CURRENT_VERSION | cut -d '.' -f2)
	if (( $(echo "$MINOR_CURRENT_VERSION < $MINOR_VERSION_LIMIT") |bc -l)); then
	echo -e "${RED}[*] OUTDATED MINOR KERNEL VERSION${NC}"
	exit 0
	else
	echo -e "${GREEN}[*] SUFFICIENT KERNEL VERSION${NC}"
	fi
	# Check minor version, only if MAJOR_CURRENT_VERSION is same as MAJOR_VERSION_LIMIT
	MINOR_VERSION_LIMIT=$(echo $VERSION_LIMIT | cut -d '.' -f2)
	MINOR_CURRENT_VERSION=$(echo $CURRENT_VERSION | cut -d '.' -f2)
	if (( $(echo "$MAJOR_CURRENT_VERSION == $MAJOR_VERSION_LIMIT") |bc -l)); then
	if (( $(echo "$MINOR_CURRENT_VERSION < $MINOR_VERSION_LIMIT") |bc -l)); then
	echo -e "${RED}[*] OUTDATED MINOR KERNEL VERSION${NC}"
	exit 1
	else
	echo -e "${GREEN}[*] SUFFICIENT KERNEL VERSION${NC}"
	fi
	fi

[grantseltzer]

@yanivagman @eyakubovich

How does this look? We only need to check minor version if the major version is equal to the major version limit.

[eyakubovich]

Yes, I copied the whole file (as well as most of the files in the directory) as is from the ringbuffer test and didn't look too much into it. Since we're relying on bc anyway, you can just compare both major and minor in one shot:

if [ $(echo "$CURRENT_VERSION < $VERSION_LIMIT" | bc) -eq "1" ]; then
    echo -e "${RED}[*] OUTDATED KERNEL VERSION${NC}"
    exit 1
fi

Also perf buffer was added in 4.3 so I need to adjust the VERSION_LIMIT value.

[itaysk]

I'm not sure bc is good for version comparison:

❯ echo "1.30 < 1.3" | bc
0

I'd recommend sort -v for version aware comparisons

[grantseltzer]

Got it, that works too, once you push that change i'll go ahead and approve!

[eyakubovich]

@itaysk you're right, of course, can't compare them as decimals. How do I do it with sort? All I can come up with is:

min=$(sort -V <<EOF | head -n 1
$CURRENT_VERSION
$VERSION_LIMIT
EOF
)
if [ "$min" -ne "$VERSION_LIMIT" ]; then
  echo "too low"
fi

Is there anything cleaner?

[itaysk]

maybe something like

test $(echo "$CURRENT_VERSION\n$VERSION_LIMIT" | sort -V | head -n1) = "$VERSION_LIMIT"

(feel free to use Grant's solution as well)

[grantseltzer]

LGTM! Thanks so much @eyakubovich, much appreciated!