* [mmtests RFT v2 0/2] mmtests: fix debian dependencies
@ 2024-05-23 22:55 Luis Chamberlain
2024-05-23 22:55 ` [mmtests RFT v2 1/2] bin/install-depends: call ldconfig Luis Chamberlain
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Luis Chamberlain @ 2024-05-23 22:55 UTC (permalink / raw)
To: mgorman, jack, vbabka, dave, ziy, yuzhao, jhubbard, ryan.roberts
Cc: kdevops, p.raghav, da.gomez, gost.dev, Luis Chamberlain
As mentioned at LSFMM 2024, I'm interested in helping expand
thpchallenge-fio and automation fragmentation metrics on mmtests [0]. on
kdevops. My first bump on the road with mmtests was the old perl library
on debian which no longer has a package. This provides a suitable
replacement for it.
Changes on this v2 since v1:
- Moved routines to Stat.pm as suggested by Jan Kara
- Removed unit tests, in favor for having unit tests and verifying
correctness of the replacement in its own separate two git trees.
One git tree which let's us expand unit tests which will run the
tests against both libraries easily. And the other git tree to help
us run the same tests against an unmodified library.
- Fix a corner case Jan Kara picked up on, and added that case and more
to the unit tests.
PS. Should we have an mmtests vger list?
[0] https://lore.kernel.org/all/ZkUOXQvVjXP1T6Nk@bombadil.infradead.org/
Luis Chamberlain (2):
bin/install-depends: call ldconfig
MMTests::Stat: replace List::BinarySearch
README.md | 5 ++---
bin/install-depends | 6 ++++--
bin/lib/MMTests/Stat.pm | 48 +++++++++++++++++++++++++++++++++++++++--
compare-kernels.sh | 5 -----
4 files changed, 52 insertions(+), 12 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [mmtests RFT v2 1/2] bin/install-depends: call ldconfig 2024-05-23 22:55 [mmtests RFT v2 0/2] mmtests: fix debian dependencies Luis Chamberlain @ 2024-05-23 22:55 ` Luis Chamberlain 2024-05-24 12:45 ` Jan Kara 2024-05-23 22:55 ` [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch Luis Chamberlain 2024-05-24 15:55 ` [mmtests RFT v2 0/2] mmtests: fix debian dependencies Davidlohr Bueso 2 siblings, 1 reply; 6+ messages in thread From: Luis Chamberlain @ 2024-05-23 22:55 UTC (permalink / raw) To: mgorman, jack, vbabka, dave, ziy, yuzhao, jhubbard, ryan.roberts Cc: kdevops, p.raghav, da.gomez, gost.dev, Luis Chamberlain Some packages may have bugs and forgot to run ldconfig, work around these bugs by ensuring we call ldconfig after installation. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- bin/install-depends | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/install-depends b/bin/install-depends index e0052f274027..4539350868c3 100755 --- a/bin/install-depends +++ b/bin/install-depends @@ -251,7 +251,10 @@ foreach my $package (@ARGV) { } } - print "Installed $package\n"; + if (system("ldconfig") != 0) { + print "ERROR: ldconfig failed\n"; + exit(-1); + } } if ($ENV{"MMTESTS_SESSION_ID"} ne "") { -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [mmtests RFT v2 1/2] bin/install-depends: call ldconfig 2024-05-23 22:55 ` [mmtests RFT v2 1/2] bin/install-depends: call ldconfig Luis Chamberlain @ 2024-05-24 12:45 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2024-05-24 12:45 UTC (permalink / raw) To: Luis Chamberlain Cc: mgorman, jack, vbabka, dave, ziy, yuzhao, jhubbard, ryan.roberts, kdevops, p.raghav, da.gomez, gost.dev On Thu 23-05-24 15:55:54, Luis Chamberlain wrote: > Some packages may have bugs and forgot to run ldconfig, work around > these bugs by ensuring we call ldconfig after installation. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Some notes below: > diff --git a/bin/install-depends b/bin/install-depends > index e0052f274027..4539350868c3 100755 > --- a/bin/install-depends > +++ b/bin/install-depends > @@ -251,7 +251,10 @@ foreach my $package (@ARGV) { > } > } > > - print "Installed $package\n"; > + if (system("ldconfig") != 0) { > + print "ERROR: ldconfig failed\n"; > + exit(-1); > + } Do we need to call 'ldconfig' after *each* installed package? I'd expect it is enough to call it once after all packages are installed? Also why did you remove the 'Installed $package' message? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch 2024-05-23 22:55 [mmtests RFT v2 0/2] mmtests: fix debian dependencies Luis Chamberlain 2024-05-23 22:55 ` [mmtests RFT v2 1/2] bin/install-depends: call ldconfig Luis Chamberlain @ 2024-05-23 22:55 ` Luis Chamberlain 2024-05-24 12:52 ` Jan Kara 2024-05-24 15:55 ` [mmtests RFT v2 0/2] mmtests: fix debian dependencies Davidlohr Bueso 2 siblings, 1 reply; 6+ messages in thread From: Luis Chamberlain @ 2024-05-23 22:55 UTC (permalink / raw) To: mgorman, jack, vbabka, dave, ziy, yuzhao, jhubbard, ryan.roberts Cc: kdevops, p.raghav, da.gomez, gost.dev, Luis Chamberlain List::BinarySearch is ancient and has a few issues: a) The project hasnt't received any updates since 2018 b) It's licensed under GPLv1 or Artistica v1 license which are not compatible with GPLv2 c) Some distributions don't carry a package for it Debian in particular doesn't have it anymore. I looked at the code and was bewildered with the amount of insane hacks to support ancient versions of perl and to also support running our custom comparison routine. All the above challenges seem silly to deal with on users if we can just implement what we need and call it a day. So let's do that. Instead of piling a set of unit tests on mmtests the approach taken to ensure a proper replacement for the library is to use a separate repository that proves a replacement for the library, which has its own unit tests [0]. The work we do here is then to not only show how we have simplified the old code, but also can use the same tests in the kill-List-BinarySearch against the exact old library without breaking things. That gives us two ways to test for correctness. While at it use SPDX license identifiers to simplify license clarity just as we have in the kernel. The same SPDX tag is already upstream on Linux. We can expand on this to keep things simpler later. [0] https://github.com/mcgrof/kill-List-BinarySearch Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- README.md | 5 ++--- bin/install-depends | 1 - bin/lib/MMTests/Stat.pm | 48 +++++++++++++++++++++++++++++++++++++++-- compare-kernels.sh | 5 ----- 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ae9b119d150d..98c5975e4197 100644 --- a/README.md +++ b/README.md @@ -44,9 +44,8 @@ $ ../../compare-kernels.sh --format html --output-dir /tmp/html > /tmp/html/inde The first step is optional. Some configurations are auto-generated from a template, particularly the filesystem-specific ones. -Note that [`List::BinarySearch`](https://metacpan.org/pod/List::BinarySearch) -and maybe even [`Math::Gradient`](https://metacpan.org/pod/Math::Gradient) may -need to be installed from [CPAN](https://www.cpan.org/) for the reporting to +Note that perhaps [`Math::Gradient`](https://metacpan.org/pod/Math::Gradient) +may need to be installed from [CPAN](https://www.cpan.org/) for the reporting to work. Similarly, [`R`](https://www.r-project.org/) should be installed if attempting to highlight whether performance differences are statistically relevant. diff --git a/bin/install-depends b/bin/install-depends index 4539350868c3..860995488680 100755 --- a/bin/install-depends +++ b/bin/install-depends @@ -61,7 +61,6 @@ my %package_map = ( "debian::xz" => "xz-utils", "debian::zeromq-devel" => "libzmq3-dev", "debian::zlib-devel" => "zlib1g-dev", - "debian::perl-List-BinarySearch" => "liblist-binarysearch-perl", "debian::perl-GD" => "libgd-perl", "debian::sysvinit-tools" => "sysvinit-utils", "debian::gettext-runtime" => "gettext", diff --git a/bin/lib/MMTests/Stat.pm b/bin/lib/MMTests/Stat.pm index 41e6b79267c9..d14b51bb36c3 100644 --- a/bin/lib/MMTests/Stat.pm +++ b/bin/lib/MMTests/Stat.pm @@ -1,16 +1,23 @@ +# SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1 # # Stat.pm # # Basic stats module +# +# Copyright (c) 2002-2024 Mel Gorman <mgorman@techsingularity.net> +# Copyright (c) 2017-2019 Jan Kara <jack@suse.cz> +# Copyright (c) 2017-2023 Andreas Herrmann <aherrmann@suse.de> +# Copyright (c) 2024 Luis Chamberlain <mcgrof@kernel.org> package MMTests::Stat; require Exporter; use vars qw (@ISA @EXPORT); use MMTests::Report; use strict; +use feature 'signatures'; +no warnings 'experimental::signatures'; use POSIX qw(floor); use FindBin qw($Bin); -use List::BinarySearch qw(binsearch_range); use Scalar::Util qw(looks_like_number); @ISA = qw(Exporter); @@ -479,6 +486,43 @@ sub calc_submean_ci { return ($parsedrow[1], $parsedrow[2]); } +sub binsearch_pos_num($target, $aref) { + die "Only numbers allowed" unless looks_like_number($target); + die "Expected an array reference!" unless ref $aref eq 'ARRAY'; + + my ($low, $high) = (0, scalar @$aref - 1); + + while ($low <= $high) { + my $mid = int(($low + $high) / 2); + if ($aref->[$mid] < $target) { + $low = $mid + 1; + } elsif ($aref->[$mid] > $target) { + $high = $mid - 1; + } else { + return $mid; + } + } + + return $low; +} + +# Returns an inclusive range for both, if you only use one return +# value, you consume the last value. +sub binsearch_range_num ($low_target, $high_target, $aref) { + die "Only numbers allowed" unless looks_like_number($low_target); + die "Only numbers allowed" unless looks_like_number($high_target); + die "Expected an array reference!" unless ref $aref eq 'ARRAY'; + + my $index_low = binsearch_pos_num($low_target, $aref); + my $index_high = binsearch_pos_num($high_target, $aref); + + if($index_high == scalar @$aref or $aref->[$index_high] > $high_target) { + $index_high--; + } + + return ($index_low, $index_high); +} + sub calc_samples { my ($dataref, $arg) = @_; my $elements = @{$dataref}; @@ -495,7 +539,7 @@ sub calc_samples { } elsif ($high eq "max") { $high = $dataref->[$elements - 1]; } - my ($lowidx, $highidx) = binsearch_range { $a <=> $b } $low, $high, @{$dataref}; + my ($lowidx, $highidx) = binsearch_range_num($low, $high, $dataref); return $highidx - $lowidx + 1; } diff --git a/compare-kernels.sh b/compare-kernels.sh index 7c49f2d95044..428de060c553 100755 --- a/compare-kernels.sh +++ b/compare-kernels.sh @@ -83,11 +83,6 @@ if [ ! -t 1 ]; then OUT=$(mktemp ~/.compare-kernel-XXXX.out) fi -perldoc -l List::BinarySearch &>/dev/null -if [ $? -ne 0 ]; then - install-depends perl-List-BinarySearch &> $OUT -fi - if [ "$FORMAT" = "html" ]; then install-depends gnuplot &>> $OUT install-depends perl-GD &>> $OUT -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch 2024-05-23 22:55 ` [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch Luis Chamberlain @ 2024-05-24 12:52 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2024-05-24 12:52 UTC (permalink / raw) To: Luis Chamberlain Cc: mgorman, jack, vbabka, dave, ziy, yuzhao, jhubbard, ryan.roberts, kdevops, p.raghav, da.gomez, gost.dev On Thu 23-05-24 15:55:55, Luis Chamberlain wrote: > List::BinarySearch is ancient and has a few issues: > > a) The project hasnt't received any updates since 2018 > b) It's licensed under GPLv1 or Artistica v1 license which are > not compatible with GPLv2 > c) Some distributions don't carry a package for it > > Debian in particular doesn't have it anymore. I looked at the code > and was bewildered with the amount of insane hacks to support ancient > versions of perl and to also support running our custom comparison > routine. > > All the above challenges seem silly to deal with on users if we can > just implement what we need and call it a day. So let's do that. > Instead of piling a set of unit tests on mmtests the approach taken > to ensure a proper replacement for the library is to use a separate > repository that proves a replacement for the library, which has its > own unit tests [0]. The work we do here is then to not only show how > we have simplified the old code, but also can use the same tests in > the kill-List-BinarySearch against the exact old library without > breaking things. That gives us two ways to test for correctness. > > While at it use SPDX license identifiers to simplify license clarity > just as we have in the kernel. The same SPDX tag is already upstream > on Linux. We can expand on this to keep things simpler later. > > [0] https://github.com/mcgrof/kill-List-BinarySearch > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > README.md | 5 ++--- > bin/install-depends | 1 - > bin/lib/MMTests/Stat.pm | 48 +++++++++++++++++++++++++++++++++++++++-- > compare-kernels.sh | 5 ----- > 4 files changed, 48 insertions(+), 11 deletions(-) > > diff --git a/README.md b/README.md > index ae9b119d150d..98c5975e4197 100644 > --- a/README.md > +++ b/README.md > @@ -44,9 +44,8 @@ $ ../../compare-kernels.sh --format html --output-dir /tmp/html > /tmp/html/inde > The first step is optional. Some configurations are auto-generated from > a template, particularly the filesystem-specific ones. > > -Note that [`List::BinarySearch`](https://metacpan.org/pod/List::BinarySearch) > -and maybe even [`Math::Gradient`](https://metacpan.org/pod/Math::Gradient) may > -need to be installed from [CPAN](https://www.cpan.org/) for the reporting to > +Note that perhaps [`Math::Gradient`](https://metacpan.org/pod/Math::Gradient) > +may need to be installed from [CPAN](https://www.cpan.org/) for the reporting to > work. Similarly, [`R`](https://www.r-project.org/) should be installed if > attempting to highlight whether performance differences are statistically > relevant. > diff --git a/bin/install-depends b/bin/install-depends > index 4539350868c3..860995488680 100755 > --- a/bin/install-depends > +++ b/bin/install-depends > @@ -61,7 +61,6 @@ my %package_map = ( > "debian::xz" => "xz-utils", > "debian::zeromq-devel" => "libzmq3-dev", > "debian::zlib-devel" => "zlib1g-dev", > - "debian::perl-List-BinarySearch" => "liblist-binarysearch-perl", > "debian::perl-GD" => "libgd-perl", > "debian::sysvinit-tools" => "sysvinit-utils", > "debian::gettext-runtime" => "gettext", > diff --git a/bin/lib/MMTests/Stat.pm b/bin/lib/MMTests/Stat.pm > index 41e6b79267c9..d14b51bb36c3 100644 > --- a/bin/lib/MMTests/Stat.pm > +++ b/bin/lib/MMTests/Stat.pm > @@ -1,16 +1,23 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later OR copyleft-next-0.3.1 > # > # Stat.pm > # > # Basic stats module > +# > +# Copyright (c) 2002-2024 Mel Gorman <mgorman@techsingularity.net> > +# Copyright (c) 2017-2019 Jan Kara <jack@suse.cz> > +# Copyright (c) 2017-2023 Andreas Herrmann <aherrmann@suse.de> > +# Copyright (c) 2024 Luis Chamberlain <mcgrof@kernel.org> > > package MMTests::Stat; > require Exporter; > use vars qw (@ISA @EXPORT); > use MMTests::Report; > use strict; > +use feature 'signatures'; > +no warnings 'experimental::signatures'; > use POSIX qw(floor); > use FindBin qw($Bin); > -use List::BinarySearch qw(binsearch_range); > use Scalar::Util qw(looks_like_number); > > @ISA = qw(Exporter); > @@ -479,6 +486,43 @@ sub calc_submean_ci { > return ($parsedrow[1], $parsedrow[2]); > } > > +sub binsearch_pos_num($target, $aref) { > + die "Only numbers allowed" unless looks_like_number($target); > + die "Expected an array reference!" unless ref $aref eq 'ARRAY'; > + > + my ($low, $high) = (0, scalar @$aref - 1); > + > + while ($low <= $high) { > + my $mid = int(($low + $high) / 2); > + if ($aref->[$mid] < $target) { > + $low = $mid + 1; > + } elsif ($aref->[$mid] > $target) { > + $high = $mid - 1; > + } else { > + return $mid; > + } > + } > + > + return $low; > +} > + > +# Returns an inclusive range for both, if you only use one return > +# value, you consume the last value. > +sub binsearch_range_num ($low_target, $high_target, $aref) { > + die "Only numbers allowed" unless looks_like_number($low_target); > + die "Only numbers allowed" unless looks_like_number($high_target); > + die "Expected an array reference!" unless ref $aref eq 'ARRAY'; > + > + my $index_low = binsearch_pos_num($low_target, $aref); > + my $index_high = binsearch_pos_num($high_target, $aref); > + > + if($index_high == scalar @$aref or $aref->[$index_high] > $high_target) { > + $index_high--; > + } > + > + return ($index_low, $index_high); > +} > + > sub calc_samples { > my ($dataref, $arg) = @_; > my $elements = @{$dataref}; > @@ -495,7 +539,7 @@ sub calc_samples { > } elsif ($high eq "max") { > $high = $dataref->[$elements - 1]; > } > - my ($lowidx, $highidx) = binsearch_range { $a <=> $b } $low, $high, @{$dataref}; > + my ($lowidx, $highidx) = binsearch_range_num($low, $high, $dataref); > return $highidx - $lowidx + 1; > } > > diff --git a/compare-kernels.sh b/compare-kernels.sh > index 7c49f2d95044..428de060c553 100755 > --- a/compare-kernels.sh > +++ b/compare-kernels.sh > @@ -83,11 +83,6 @@ if [ ! -t 1 ]; then > OUT=$(mktemp ~/.compare-kernel-XXXX.out) > fi > > -perldoc -l List::BinarySearch &>/dev/null > -if [ $? -ne 0 ]; then > - install-depends perl-List-BinarySearch &> $OUT > -fi > - > if [ "$FORMAT" = "html" ]; then > install-depends gnuplot &>> $OUT > install-depends perl-GD &>> $OUT > -- > 2.43.0 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [mmtests RFT v2 0/2] mmtests: fix debian dependencies 2024-05-23 22:55 [mmtests RFT v2 0/2] mmtests: fix debian dependencies Luis Chamberlain 2024-05-23 22:55 ` [mmtests RFT v2 1/2] bin/install-depends: call ldconfig Luis Chamberlain 2024-05-23 22:55 ` [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch Luis Chamberlain @ 2024-05-24 15:55 ` Davidlohr Bueso 2 siblings, 0 replies; 6+ messages in thread From: Davidlohr Bueso @ 2024-05-24 15:55 UTC (permalink / raw) To: Luis Chamberlain Cc: mgorman, jack, vbabka, ziy, yuzhao, jhubbard, ryan.roberts, kdevops, p.raghav, da.gomez, gost.dev On Thu, 23 May 2024, Luis Chamberlain wrote: >As mentioned at LSFMM 2024, I'm interested in helping expand >thpchallenge-fio and automation fragmentation metrics on mmtests [0]. on >kdevops. My first bump on the road with mmtests was the old perl library >on debian which no longer has a package. This provides a suitable >replacement for it. fwiw these changes don't break my usual battery of tests (futex, hackbench, sysv ipc, etc). Feel free to add my: Tested-by: Davidlohr Bueso <dave@stgolabs.net> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-24 16:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-05-23 22:55 [mmtests RFT v2 0/2] mmtests: fix debian dependencies Luis Chamberlain 2024-05-23 22:55 ` [mmtests RFT v2 1/2] bin/install-depends: call ldconfig Luis Chamberlain 2024-05-24 12:45 ` Jan Kara 2024-05-23 22:55 ` [mmtests RFT v2 2/2] MMTests::Stat: replace List::BinarySearch Luis Chamberlain 2024-05-24 12:52 ` Jan Kara 2024-05-24 15:55 ` [mmtests RFT v2 0/2] mmtests: fix debian dependencies Davidlohr Bueso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox