DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
@ 2021-03-17 14:44 Aaron Conole
  2021-03-17 14:57 ` Thomas Monjalon
  2021-03-19 13:41 ` David Marchand
  0 siblings, 2 replies; 12+ messages in thread
From: Aaron Conole @ 2021-03-17 14:44 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, Thomas Monjalon

The hugepage test really needs to check multiple things on Linux:

1. Are hugepages reserved in the system?

2. Is the hugepage mountpoint available so that we can allocate them?

3. Do we have permissions to write into the hugepage mountpoint?

The existing hugepage check only verifies the first.  On some setups,
a non-root user won't have access to the mountpoint for hugepages to
be allocated and that needs to be reflected in the test as well.  Add
such checks for Linux OS to give a more check when running test suites.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/has-hugepage.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
index d600fad319..1c3cfb665a 100755
--- a/app/test/has-hugepage.sh
+++ b/app/test/has-hugepage.sh
@@ -3,7 +3,17 @@
 # Copyright 2020 Mellanox Technologies, Ltd
 
 if [ "$(uname)" = "Linux" ] ; then
-	cat /proc/sys/vm/nr_hugepages || echo 0
+	nr_hugepages=$(cat /proc/sys/vm/nr_hugepages)
+	# Need to check if we have permissions to access hugepages
+	perm=""
+	for mount in `mount | grep hugetlbfs | awk '{ print $3; }'`; do
+		test ! -w $mount/. || perm="$mount"
+	done
+	if [ "$perm" = "" -o "$nr_hugepages" = "0" ]; then
+		echo 0
+	else
+		echo $nr_hugepages
+	fi
 elif [ "$(uname)" = "FreeBSD" ] ; then
 	echo 1 # assume FreeBSD always has hugepages
 else
-- 
2.25.4


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-03-17 14:44 [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux Aaron Conole
@ 2021-03-17 14:57 ` Thomas Monjalon
  2021-04-06 12:33   ` Aaron Conole
  2021-03-19 13:41 ` David Marchand
  1 sibling, 1 reply; 12+ messages in thread
From: Thomas Monjalon @ 2021-03-17 14:57 UTC (permalink / raw)
  To: Aaron Conole; +Cc: dev, David Marchand, anatoly.burakov

17/03/2021 15:44, Aaron Conole:
> The hugepage test really needs to check multiple things on Linux:
> 
> 1. Are hugepages reserved in the system?
> 
> 2. Is the hugepage mountpoint available so that we can allocate them?
> 
> 3. Do we have permissions to write into the hugepage mountpoint?
> 
> The existing hugepage check only verifies the first.  On some setups,
> a non-root user won't have access to the mountpoint for hugepages to
> be allocated and that needs to be reflected in the test as well.  Add
> such checks for Linux OS to give a more check when running test suites.

Requirements 2 & 3 are optional.
You don't need a mount point if using the option --in-memory.

[...]
> +	perm=""

perm= should do the same.

> +	for mount in `mount | grep hugetlbfs | awk '{ print $3; }'`; do

Please prefer $() syntax.
Are spaces in awk required?

> +		test ! -w $mount/. || perm="$mount"

Why /. ?

> +	done
> +	if [ "$perm" = "" -o "$nr_hugepages" = "0" ]; then

= "" can be replaced with -z
"0" can be simply 0

> +		echo 0
> +	else
> +		echo $nr_hugepages
> +	fi




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-03-17 14:44 [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux Aaron Conole
  2021-03-17 14:57 ` Thomas Monjalon
@ 2021-03-19 13:41 ` David Marchand
  2021-03-19 14:34   ` Aaron Conole
  1 sibling, 1 reply; 12+ messages in thread
From: David Marchand @ 2021-03-19 13:41 UTC (permalink / raw)
  To: Aaron Conole; +Cc: dev, Thomas Monjalon, Burakov, Anatoly, Bruce Richardson

On Wed, Mar 17, 2021 at 3:44 PM Aaron Conole <aconole@redhat.com> wrote:
> diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
> index d600fad319..1c3cfb665a 100755
> --- a/app/test/has-hugepage.sh
> +++ b/app/test/has-hugepage.sh
> @@ -3,7 +3,17 @@
>  # Copyright 2020 Mellanox Technologies, Ltd
>
>  if [ "$(uname)" = "Linux" ] ; then
> -       cat /proc/sys/vm/nr_hugepages || echo 0
> +       nr_hugepages=$(cat /proc/sys/vm/nr_hugepages)
> +       # Need to check if we have permissions to access hugepages
> +       perm=""
> +       for mount in `mount | grep hugetlbfs | awk '{ print $3; }'`; do
> +               test ! -w $mount/. || perm="$mount"
> +       done
> +       if [ "$perm" = "" -o "$nr_hugepages" = "0" ]; then
> +               echo 0
> +       else
> +               echo $nr_hugepages
> +       fi
>  elif [ "$(uname)" = "FreeBSD" ] ; then
>         echo 1 # assume FreeBSD always has hugepages
>  else

The check is evaluated first when configuring as normal user and not
reevaluated when running the tests as root in Travis/GHA, so the tests
are started with no hugepage.

Then, in Travis and GHA, we can see (sigbus o_O) crashes in no-huge mode.
I tried to reproduce but did not manage.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-03-19 13:41 ` David Marchand
@ 2021-03-19 14:34   ` Aaron Conole
  0 siblings, 0 replies; 12+ messages in thread
From: Aaron Conole @ 2021-03-19 14:34 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Thomas Monjalon, Burakov\, Anatoly, Bruce Richardson

David Marchand <david.marchand@redhat.com> writes:

> On Wed, Mar 17, 2021 at 3:44 PM Aaron Conole <aconole@redhat.com> wrote:
>> diff --git a/app/test/has-hugepage.sh b/app/test/has-hugepage.sh
>> index d600fad319..1c3cfb665a 100755
>> --- a/app/test/has-hugepage.sh
>> +++ b/app/test/has-hugepage.sh
>> @@ -3,7 +3,17 @@
>>  # Copyright 2020 Mellanox Technologies, Ltd
>>
>>  if [ "$(uname)" = "Linux" ] ; then
>> -       cat /proc/sys/vm/nr_hugepages || echo 0
>> +       nr_hugepages=$(cat /proc/sys/vm/nr_hugepages)
>> +       # Need to check if we have permissions to access hugepages
>> +       perm=""
>> +       for mount in `mount | grep hugetlbfs | awk '{ print $3; }'`; do
>> +               test ! -w $mount/. || perm="$mount"
>> +       done
>> +       if [ "$perm" = "" -o "$nr_hugepages" = "0" ]; then
>> +               echo 0
>> +       else
>> +               echo $nr_hugepages
>> +       fi
>>  elif [ "$(uname)" = "FreeBSD" ] ; then
>>         echo 1 # assume FreeBSD always has hugepages
>>  else
>
> The check is evaluated first when configuring as normal user and not
> reevaluated when running the tests as root in Travis/GHA, so the tests
> are started with no hugepage.
>
> Then, in Travis and GHA, we can see (sigbus o_O) crashes in no-huge mode.
> I tried to reproduce but did not manage.

I noticed that as well.  I am equally as confused, but I'm working on
it (along with folding in Thomas' suggestions).


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-03-17 14:57 ` Thomas Monjalon
@ 2021-04-06 12:33   ` Aaron Conole
  2021-04-06 12:58     ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Aaron Conole @ 2021-04-06 12:33 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, David Marchand, anatoly.burakov, Bruce Richardson

Thomas Monjalon <thomas@monjalon.net> writes:

> 17/03/2021 15:44, Aaron Conole:
>> The hugepage test really needs to check multiple things on Linux:
>> 
>> 1. Are hugepages reserved in the system?
>> 
>> 2. Is the hugepage mountpoint available so that we can allocate them?
>> 
>> 3. Do we have permissions to write into the hugepage mountpoint?
>> 
>> The existing hugepage check only verifies the first.  On some setups,
>> a non-root user won't have access to the mountpoint for hugepages to
>> be allocated and that needs to be reflected in the test as well.  Add
>> such checks for Linux OS to give a more check when running test suites.
>
> Requirements 2 & 3 are optional.
> You don't need a mount point if using the option --in-memory.

That's true, but it seems to break a few of the unit tests without.
I'll clarify the commit message.

Additionally, I thought it would be simple to just incorporate your
suggestions - but it seems that meson / ninja doesn't have cascading
dependencies the way 'make' does (or, I haven't figured out from the
syntax how to do that) - a 'run_command' gets resolved at configure
time and it doesn't seem that we can make a run_target depend on another
run_target since dependencies are on file outputs.  Maybe we do some
kind of trickery here where we write a file that the build script reads?

I am trying to figure out how best to accomplish this - suggestions
welcome.

> [...]
>> +	perm=""
>
> perm= should do the same.
>
>> +	for mount in `mount | grep hugetlbfs | awk '{ print $3; }'`; do
>
> Please prefer $() syntax.

Okay

> Are spaces in awk required?

I'm not sure - I don't think so.

>> +		test ! -w $mount/. || perm="$mount"
>
> Why /. ?

Habit.  I will remove it.

>> +	done
>> +	if [ "$perm" = "" -o "$nr_hugepages" = "0" ]; then
>
> = "" can be replaced with -z
> "0" can be simply 0

Done.

>> +		echo 0
>> +	else
>> +		echo $nr_hugepages
>> +	fi


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-06 12:33   ` Aaron Conole
@ 2021-04-06 12:58     ` Bruce Richardson
  2021-04-06 14:20       ` Aaron Conole
  0 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2021-04-06 12:58 UTC (permalink / raw)
  To: Aaron Conole; +Cc: Thomas Monjalon, dev, David Marchand, anatoly.burakov

On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
> > 17/03/2021 15:44, Aaron Conole:
> >> The hugepage test really needs to check multiple things on Linux:
> >> 
> >> 1. Are hugepages reserved in the system?
> >> 
> >> 2. Is the hugepage mountpoint available so that we can allocate them?
> >> 
> >> 3. Do we have permissions to write into the hugepage mountpoint?
> >> 
> >> The existing hugepage check only verifies the first.  On some setups,
> >> a non-root user won't have access to the mountpoint for hugepages to
> >> be allocated and that needs to be reflected in the test as well.  Add
> >> such checks for Linux OS to give a more check when running test suites.
> >
> > Requirements 2 & 3 are optional.
> > You don't need a mount point if using the option --in-memory.
> 
> That's true, but it seems to break a few of the unit tests without.
> I'll clarify the commit message.
> 
> Additionally, I thought it would be simple to just incorporate your
> suggestions - but it seems that meson / ninja doesn't have cascading
> dependencies the way 'make' does (or, I haven't figured out from the
> syntax how to do that) - a 'run_command' gets resolved at configure
> time and it doesn't seem that we can make a run_target depend on another
> run_target since dependencies are on file outputs.  Maybe we do some
> kind of trickery here where we write a file that the build script reads?
> 
> I am trying to figure out how best to accomplish this - suggestions
> welcome.
> 
Sorry that I'm late to this thread. Can you perhaps explain what you mean
by cascading dependencies in this instance, or what you are trying to do
exactly that is not supported?

In terms of output files that build script reads, yes that is possible -
the file just needs to be in the standard .d format that gcc produces and
then ninja can use that to track file dependencies. The script
"call-sphinx-build.py" does this, for example, and doc/guides/meson.build
tells meson/ninja to look for dependencies in file ".html.d" generated by
that script.

/Bruce

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-06 12:58     ` Bruce Richardson
@ 2021-04-06 14:20       ` Aaron Conole
  2021-04-06 14:50         ` Bruce Richardson
  0 siblings, 1 reply; 12+ messages in thread
From: Aaron Conole @ 2021-04-06 14:20 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Thomas Monjalon, dev, David Marchand, anatoly.burakov

Bruce Richardson <bruce.richardson@intel.com> writes:

> On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
>> Thomas Monjalon <thomas@monjalon.net> writes:
>> 
>> > 17/03/2021 15:44, Aaron Conole:
>> >> The hugepage test really needs to check multiple things on Linux:
>> >> 
>> >> 1. Are hugepages reserved in the system?
>> >> 
>> >> 2. Is the hugepage mountpoint available so that we can allocate them?
>> >> 
>> >> 3. Do we have permissions to write into the hugepage mountpoint?
>> >> 
>> >> The existing hugepage check only verifies the first.  On some setups,
>> >> a non-root user won't have access to the mountpoint for hugepages to
>> >> be allocated and that needs to be reflected in the test as well.  Add
>> >> such checks for Linux OS to give a more check when running test suites.
>> >
>> > Requirements 2 & 3 are optional.
>> > You don't need a mount point if using the option --in-memory.
>> 
>> That's true, but it seems to break a few of the unit tests without.
>> I'll clarify the commit message.
>> 
>> Additionally, I thought it would be simple to just incorporate your
>> suggestions - but it seems that meson / ninja doesn't have cascading
>> dependencies the way 'make' does (or, I haven't figured out from the
>> syntax how to do that) - a 'run_command' gets resolved at configure
>> time and it doesn't seem that we can make a run_target depend on another
>> run_target since dependencies are on file outputs.  Maybe we do some
>> kind of trickery here where we write a file that the build script reads?
>> 
>> I am trying to figure out how best to accomplish this - suggestions
>> welcome.
>> 
> Sorry that I'm late to this thread. Can you perhaps explain what you mean
> by cascading dependencies in this instance, or what you are trying to do
> exactly that is not supported?

I want to conditionally invoke the test suite with the hugepage tests,
and support the case that the machine has hugepages enabled, but not
accessible.

Right now, if a user runs:

  meson build && ninja -C build test

with hugepages allocated as a non-root user, they will see 'FAIL'
messages.  This isn't very friendly, since the user would be confused.

Right now, hugepage detection is done only at configure time (the
'meson' step), and then the target is always run.

For now, I will continue modifying the below script, but that will be a
detection at configure time, still.  So the case where the user runs
'meson' when they have hugepages, but those hugepages go away and then
they run 'ninja -C build test' will still be FAIL instead of SKIP (maybe
we need a more descriptive error when FAIL due to hugepages happen?)

> In terms of output files that build script reads, yes that is possible -
> the file just needs to be in the standard .d format that gcc produces and
> then ninja can use that to track file dependencies. The script
> "call-sphinx-build.py" does this, for example, and doc/guides/meson.build
> tells meson/ninja to look for dependencies in file ".html.d" generated by
> that script.
>
> /Bruce


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-06 14:20       ` Aaron Conole
@ 2021-04-06 14:50         ` Bruce Richardson
  2021-04-09 15:06           ` Aaron Conole
  0 siblings, 1 reply; 12+ messages in thread
From: Bruce Richardson @ 2021-04-06 14:50 UTC (permalink / raw)
  To: Aaron Conole; +Cc: Thomas Monjalon, dev, David Marchand, anatoly.burakov

On Tue, Apr 06, 2021 at 10:20:37AM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
> >> Thomas Monjalon <thomas@monjalon.net> writes:
> >> 
> >> > 17/03/2021 15:44, Aaron Conole:
> >> >> The hugepage test really needs to check multiple things on Linux:
> >> >> 
> >> >> 1. Are hugepages reserved in the system?
> >> >> 
> >> >> 2. Is the hugepage mountpoint available so that we can allocate them?
> >> >> 
> >> >> 3. Do we have permissions to write into the hugepage mountpoint?
> >> >> 
> >> >> The existing hugepage check only verifies the first.  On some setups,
> >> >> a non-root user won't have access to the mountpoint for hugepages to
> >> >> be allocated and that needs to be reflected in the test as well.  Add
> >> >> such checks for Linux OS to give a more check when running test suites.
> >> >
> >> > Requirements 2 & 3 are optional.
> >> > You don't need a mount point if using the option --in-memory.
> >> 
> >> That's true, but it seems to break a few of the unit tests without.
> >> I'll clarify the commit message.
> >> 
> >> Additionally, I thought it would be simple to just incorporate your
> >> suggestions - but it seems that meson / ninja doesn't have cascading
> >> dependencies the way 'make' does (or, I haven't figured out from the
> >> syntax how to do that) - a 'run_command' gets resolved at configure
> >> time and it doesn't seem that we can make a run_target depend on another
> >> run_target since dependencies are on file outputs.  Maybe we do some
> >> kind of trickery here where we write a file that the build script reads?
> >> 
> >> I am trying to figure out how best to accomplish this - suggestions
> >> welcome.
> >> 
> > Sorry that I'm late to this thread. Can you perhaps explain what you mean
> > by cascading dependencies in this instance, or what you are trying to do
> > exactly that is not supported?
> 
> I want to conditionally invoke the test suite with the hugepage tests,
> and support the case that the machine has hugepages enabled, but not
> accessible.
> 
> Right now, if a user runs:
> 
>   meson build && ninja -C build test
> 
> with hugepages allocated as a non-root user, they will see 'FAIL'
> messages.  This isn't very friendly, since the user would be confused.
> 
> Right now, hugepage detection is done only at configure time (the
> 'meson' step), and then the target is always run.
> 
> For now, I will continue modifying the below script, but that will be a
> detection at configure time, still.  So the case where the user runs
> 'meson' when they have hugepages, but those hugepages go away and then
> they run 'ninja -C build test' will still be FAIL instead of SKIP (maybe
> we need a more descriptive error when FAIL due to hugepages happen?)
> 

This seems to me like the test binary itself should be checking the
presence of hugepages, and reporting skips if necessary. It's not just when
run through ninja that this functionality would be useful.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-06 14:50         ` Bruce Richardson
@ 2021-04-09 15:06           ` Aaron Conole
  2021-04-09 15:33             ` Thomas Monjalon
  2021-04-09 15:40             ` Bruce Richardson
  0 siblings, 2 replies; 12+ messages in thread
From: Aaron Conole @ 2021-04-09 15:06 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Thomas Monjalon, dev, David Marchand, anatoly.burakov

Bruce Richardson <bruce.richardson@intel.com> writes:

> On Tue, Apr 06, 2021 at 10:20:37AM -0400, Aaron Conole wrote:
>> Bruce Richardson <bruce.richardson@intel.com> writes:
>> 
>> > On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
>> >> Thomas Monjalon <thomas@monjalon.net> writes:
>> >> 
>> >> > 17/03/2021 15:44, Aaron Conole:
>> >> >> The hugepage test really needs to check multiple things on Linux:
>> >> >> 
>> >> >> 1. Are hugepages reserved in the system?
>> >> >> 
>> >> >> 2. Is the hugepage mountpoint available so that we can allocate them?
>> >> >> 
>> >> >> 3. Do we have permissions to write into the hugepage mountpoint?
>> >> >> 
>> >> >> The existing hugepage check only verifies the first.  On some setups,
>> >> >> a non-root user won't have access to the mountpoint for hugepages to
>> >> >> be allocated and that needs to be reflected in the test as well.  Add
>> >> >> such checks for Linux OS to give a more check when running test suites.
>> >> >
>> >> > Requirements 2 & 3 are optional.
>> >> > You don't need a mount point if using the option --in-memory.
>> >> 
>> >> That's true, but it seems to break a few of the unit tests without.
>> >> I'll clarify the commit message.
>> >> 
>> >> Additionally, I thought it would be simple to just incorporate your
>> >> suggestions - but it seems that meson / ninja doesn't have cascading
>> >> dependencies the way 'make' does (or, I haven't figured out from the
>> >> syntax how to do that) - a 'run_command' gets resolved at configure
>> >> time and it doesn't seem that we can make a run_target depend on another
>> >> run_target since dependencies are on file outputs.  Maybe we do some
>> >> kind of trickery here where we write a file that the build script reads?
>> >> 
>> >> I am trying to figure out how best to accomplish this - suggestions
>> >> welcome.
>> >> 
>> > Sorry that I'm late to this thread. Can you perhaps explain what you mean
>> > by cascading dependencies in this instance, or what you are trying to do
>> > exactly that is not supported?
>> 
>> I want to conditionally invoke the test suite with the hugepage tests,
>> and support the case that the machine has hugepages enabled, but not
>> accessible.
>> 
>> Right now, if a user runs:
>> 
>>   meson build && ninja -C build test
>> 
>> with hugepages allocated as a non-root user, they will see 'FAIL'
>> messages.  This isn't very friendly, since the user would be confused.
>> 
>> Right now, hugepage detection is done only at configure time (the
>> 'meson' step), and then the target is always run.
>> 
>> For now, I will continue modifying the below script, but that will be a
>> detection at configure time, still.  So the case where the user runs
>> 'meson' when they have hugepages, but those hugepages go away and then
>> they run 'ninja -C build test' will still be FAIL instead of SKIP (maybe
>> we need a more descriptive error when FAIL due to hugepages happen?)
>> 
>
> This seems to me like the test binary itself should be checking the
> presence of hugepages, and reporting skips if necessary. It's not just when
> run through ninja that this functionality would be useful.

Either way, there needs to be a rework - if we do it in the test binary,
then the tests that require hugepages need to be worked so that they
correctly detect lack of hugepage support before starting.  If we keep
that knowledge in the meson system, then we need to change the way we
call the test binary script to support a more robust detection.

I guess, I don't care too much which one is the one we choose.  My $.02
opinion is that we already have most of the logic and whatnot done in
the build system, so I'd prefer to do as small a change as possible
(leaving that logic in the meson system).  Then again, maybe it makes
more sense to just rip the bandaid off and move it all into the test
framework.

WDYT?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-09 15:06           ` Aaron Conole
@ 2021-04-09 15:33             ` Thomas Monjalon
  2021-04-12 11:33               ` David Marchand
  2021-04-09 15:40             ` Bruce Richardson
  1 sibling, 1 reply; 12+ messages in thread
From: Thomas Monjalon @ 2021-04-09 15:33 UTC (permalink / raw)
  To: Aaron Conole; +Cc: Bruce Richardson, dev, David Marchand, anatoly.burakov

09/04/2021 17:06, Aaron Conole:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Tue, Apr 06, 2021 at 10:20:37AM -0400, Aaron Conole wrote:
> >> Bruce Richardson <bruce.richardson@intel.com> writes:
> >> 
> >> > On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
> >> >> Thomas Monjalon <thomas@monjalon.net> writes:
> >> >> 
> >> >> > 17/03/2021 15:44, Aaron Conole:
> >> >> >> The hugepage test really needs to check multiple things on Linux:
> >> >> >> 
> >> >> >> 1. Are hugepages reserved in the system?
> >> >> >> 
> >> >> >> 2. Is the hugepage mountpoint available so that we can allocate them?
> >> >> >> 
> >> >> >> 3. Do we have permissions to write into the hugepage mountpoint?
> >> >> >> 
> >> >> >> The existing hugepage check only verifies the first.  On some setups,
> >> >> >> a non-root user won't have access to the mountpoint for hugepages to
> >> >> >> be allocated and that needs to be reflected in the test as well.  Add
> >> >> >> such checks for Linux OS to give a more check when running test suites.
> >> >> >
> >> >> > Requirements 2 & 3 are optional.
> >> >> > You don't need a mount point if using the option --in-memory.
> >> >> 
> >> >> That's true, but it seems to break a few of the unit tests without.
> >> >> I'll clarify the commit message.
> >> >> 
> >> >> Additionally, I thought it would be simple to just incorporate your
> >> >> suggestions - but it seems that meson / ninja doesn't have cascading
> >> >> dependencies the way 'make' does (or, I haven't figured out from the
> >> >> syntax how to do that) - a 'run_command' gets resolved at configure
> >> >> time and it doesn't seem that we can make a run_target depend on another
> >> >> run_target since dependencies are on file outputs.  Maybe we do some
> >> >> kind of trickery here where we write a file that the build script reads?
> >> >> 
> >> >> I am trying to figure out how best to accomplish this - suggestions
> >> >> welcome.
> >> >> 
> >> > Sorry that I'm late to this thread. Can you perhaps explain what you mean
> >> > by cascading dependencies in this instance, or what you are trying to do
> >> > exactly that is not supported?
> >> 
> >> I want to conditionally invoke the test suite with the hugepage tests,
> >> and support the case that the machine has hugepages enabled, but not
> >> accessible.
> >> 
> >> Right now, if a user runs:
> >> 
> >>   meson build && ninja -C build test
> >> 
> >> with hugepages allocated as a non-root user, they will see 'FAIL'
> >> messages.  This isn't very friendly, since the user would be confused.
> >> 
> >> Right now, hugepage detection is done only at configure time (the
> >> 'meson' step), and then the target is always run.
> >> 
> >> For now, I will continue modifying the below script, but that will be a
> >> detection at configure time, still.  So the case where the user runs
> >> 'meson' when they have hugepages, but those hugepages go away and then
> >> they run 'ninja -C build test' will still be FAIL instead of SKIP (maybe
> >> we need a more descriptive error when FAIL due to hugepages happen?)
> >> 
> >
> > This seems to me like the test binary itself should be checking the
> > presence of hugepages, and reporting skips if necessary. It's not just when
> > run through ninja that this functionality would be useful.
> 
> Either way, there needs to be a rework - if we do it in the test binary,
> then the tests that require hugepages need to be worked so that they
> correctly detect lack of hugepage support before starting.  If we keep
> that knowledge in the meson system, then we need to change the way we
> call the test binary script to support a more robust detection.
> 
> I guess, I don't care too much which one is the one we choose.  My $.02
> opinion is that we already have most of the logic and whatnot done in
> the build system, so I'd prefer to do as small a change as possible
> (leaving that logic in the meson system).  Then again, maybe it makes
> more sense to just rip the bandaid off and move it all into the test
> framework.
> 
> WDYT?

I think the test application should adapt to its environment.
If no hugepage, then mark the tests requiring hugepages as skipped.
For the other tests, we could use --in-memory.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-09 15:06           ` Aaron Conole
  2021-04-09 15:33             ` Thomas Monjalon
@ 2021-04-09 15:40             ` Bruce Richardson
  1 sibling, 0 replies; 12+ messages in thread
From: Bruce Richardson @ 2021-04-09 15:40 UTC (permalink / raw)
  To: Aaron Conole; +Cc: Thomas Monjalon, dev, David Marchand, anatoly.burakov

On Fri, Apr 09, 2021 at 11:06:20AM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Tue, Apr 06, 2021 at 10:20:37AM -0400, Aaron Conole wrote:
> >> Bruce Richardson <bruce.richardson@intel.com> writes:
> >> 
> >> > On Tue, Apr 06, 2021 at 08:33:07AM -0400, Aaron Conole wrote:
> >> >> Thomas Monjalon <thomas@monjalon.net> writes:
> >> >> 
> >> >> > 17/03/2021 15:44, Aaron Conole:
> >> >> >> The hugepage test really needs to check multiple things on Linux:
> >> >> >> 
> >> >> >> 1. Are hugepages reserved in the system?
> >> >> >> 
> >> >> >> 2. Is the hugepage mountpoint available so that we can allocate them?
> >> >> >> 
> >> >> >> 3. Do we have permissions to write into the hugepage mountpoint?
> >> >> >> 
> >> >> >> The existing hugepage check only verifies the first.  On some setups,
> >> >> >> a non-root user won't have access to the mountpoint for hugepages to
> >> >> >> be allocated and that needs to be reflected in the test as well.  Add
> >> >> >> such checks for Linux OS to give a more check when running test suites.
> >> >> >
> >> >> > Requirements 2 & 3 are optional.
> >> >> > You don't need a mount point if using the option --in-memory.
> >> >> 
> >> >> That's true, but it seems to break a few of the unit tests without.
> >> >> I'll clarify the commit message.
> >> >> 
> >> >> Additionally, I thought it would be simple to just incorporate your
> >> >> suggestions - but it seems that meson / ninja doesn't have cascading
> >> >> dependencies the way 'make' does (or, I haven't figured out from the
> >> >> syntax how to do that) - a 'run_command' gets resolved at configure
> >> >> time and it doesn't seem that we can make a run_target depend on another
> >> >> run_target since dependencies are on file outputs.  Maybe we do some
> >> >> kind of trickery here where we write a file that the build script reads?
> >> >> 
> >> >> I am trying to figure out how best to accomplish this - suggestions
> >> >> welcome.
> >> >> 
> >> > Sorry that I'm late to this thread. Can you perhaps explain what you mean
> >> > by cascading dependencies in this instance, or what you are trying to do
> >> > exactly that is not supported?
> >> 
> >> I want to conditionally invoke the test suite with the hugepage tests,
> >> and support the case that the machine has hugepages enabled, but not
> >> accessible.
> >> 
> >> Right now, if a user runs:
> >> 
> >>   meson build && ninja -C build test
> >> 
> >> with hugepages allocated as a non-root user, they will see 'FAIL'
> >> messages.  This isn't very friendly, since the user would be confused.
> >> 
> >> Right now, hugepage detection is done only at configure time (the
> >> 'meson' step), and then the target is always run.
> >> 
> >> For now, I will continue modifying the below script, but that will be a
> >> detection at configure time, still.  So the case where the user runs
> >> 'meson' when they have hugepages, but those hugepages go away and then
> >> they run 'ninja -C build test' will still be FAIL instead of SKIP (maybe
> >> we need a more descriptive error when FAIL due to hugepages happen?)
> >> 
> >
> > This seems to me like the test binary itself should be checking the
> > presence of hugepages, and reporting skips if necessary. It's not just when
> > run through ninja that this functionality would be useful.
> 
> Either way, there needs to be a rework - if we do it in the test binary,
> then the tests that require hugepages need to be worked so that they
> correctly detect lack of hugepage support before starting.  If we keep
> that knowledge in the meson system, then we need to change the way we
> call the test binary script to support a more robust detection.
> 
For putting the detection in the app, it's should just be a one-time
detection at init, right? There aren't differing hugepage requiresments per
test? If not, then any tests requiring hugepages just need to
check a global var and return skipped if not.

/Bruce

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux
  2021-04-09 15:33             ` Thomas Monjalon
@ 2021-04-12 11:33               ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2021-04-12 11:33 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Aaron Conole, Bruce Richardson, dev, Burakov, Anatoly

On Fri, Apr 9, 2021 at 5:33 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > This seems to me like the test binary itself should be checking the
> > > presence of hugepages, and reporting skips if necessary. It's not just when
> > > run through ninja that this functionality would be useful.
> >
> > Either way, there needs to be a rework - if we do it in the test binary,
> > then the tests that require hugepages need to be worked so that they
> > correctly detect lack of hugepage support before starting.  If we keep
> > that knowledge in the meson system, then we need to change the way we
> > call the test binary script to support a more robust detection.
> >
> > I guess, I don't care too much which one is the one we choose.  My $.02
> > opinion is that we already have most of the logic and whatnot done in
> > the build system, so I'd prefer to do as small a change as possible
> > (leaving that logic in the meson system).  Then again, maybe it makes
> > more sense to just rip the bandaid off and move it all into the test
> > framework.
> >
> > WDYT?
>
> I think the test application should adapt to its environment.
> If no hugepage, then mark the tests requiring hugepages as skipped.
> For the other tests, we could use --in-memory.

There are tests that rely/test mp.
They would have to be identified and skipped if we want to go with --in-memory.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-04-12 11:34 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 14:44 [dpdk-dev] [PATCH] test: make hugepage check more robust under Linux Aaron Conole
2021-03-17 14:57 ` Thomas Monjalon
2021-04-06 12:33   ` Aaron Conole
2021-04-06 12:58     ` Bruce Richardson
2021-04-06 14:20       ` Aaron Conole
2021-04-06 14:50         ` Bruce Richardson
2021-04-09 15:06           ` Aaron Conole
2021-04-09 15:33             ` Thomas Monjalon
2021-04-12 11:33               ` David Marchand
2021-04-09 15:40             ` Bruce Richardson
2021-03-19 13:41 ` David Marchand
2021-03-19 14:34   ` Aaron Conole

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git