diff mbox series

[v2] usertools: show an error message if unable to reserve requested hugepages

Message ID 20201217111616.24057-1-sarosh.arif@emumba.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] usertools: show an error message if unable to reserve requested hugepages |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Sarosh Arif Dec. 17, 2020, 11:16 a.m. UTC 
Sometimes the system is unable to reserve the requested hugepages because
enough space is not available in the RAM. In that case, currently the
script displays no error message hence the user can be under the delusion
that the hugepages he requested are all successfully reserved. This patch
displays an error message if the pages reserved are different from the
requested pages and shows the actual pages reserved.


Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
---
v2:
use a global variable SHOW_HUGEPAGES to remove linter warnings
---
 usertools/dpdk-hugepages.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger Dec. 17, 2020, 6:15 p.m. UTC | #1 
On Thu, 17 Dec 2020 16:16:16 +0500
Sarosh Arif <sarosh.arif@emumba.com> wrote:

> +    if get_hugepages(path) != pages:
> +        print("Unable to reserve required pages. The pages reserved are:")
> +        global SHOW_HUGEPAGES
> +        SHOW_HUGEPAGES = True


Please don't add global's to this script.

The script is close to being clean according to pylint, and globals
are considered bad style and shouldn't be used.

I would just exit if huge pages could not be setup.
The script should leave it up to the user to do another query about
status if they care about what the result is.
Sarosh Arif Jan. 7, 2021, 8:06 a.m. UTC | #2 
On Thu, Dec 17, 2020 at 11:19 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 17 Dec 2020 16:16:16 +0500
> Sarosh Arif <sarosh.arif@emumba.com> wrote:
>
> > +    if get_hugepages(path) != pages:
> > +        print("Unable to reserve required pages. The pages reserved are:")
> > +        global SHOW_HUGEPAGES
> > +        SHOW_HUGEPAGES = True
>
>
> Please don't add global's to this script.
>
> The script is close to being clean according to pylint, and globals
> are considered bad style and shouldn't be used.
>
> I would just exit if huge pages could not be setup.

How about if we just print a warning message such as "Unable to
reserve required pages" before exiting, in case the pages are not
reserved due to lack of space in RAM? Then leave it upon the user to
query how many pages are actually reserved.
>
> The script should leave it up to the user to do another query about
> status if they care about what the result is.
Stephen Hemminger Jan. 7, 2021, 3:37 p.m. UTC | #3 
On Thu, 7 Jan 2021 13:06:35 +0500
Sarosh Arif <sarosh.arif@emumba.com> wrote:

> On Thu, Dec 17, 2020 at 11:19 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Thu, 17 Dec 2020 16:16:16 +0500
> > Sarosh Arif <sarosh.arif@emumba.com> wrote:
> >  
> > > +    if get_hugepages(path) != pages:
> > > +        print("Unable to reserve required pages. The pages reserved are:")
> > > +        global SHOW_HUGEPAGES
> > > +        SHOW_HUGEPAGES = True  
> >
> >
> > Please don't add global's to this script.
> >
> > The script is close to being clean according to pylint, and globals
> > are considered bad style and shouldn't be used.
> >
> > I would just exit if huge pages could not be setup.  
> 
> How about if we just print a warning message such as "Unable to
> reserve required pages" before exiting, in case the pages are not
> reserved due to lack of space in RAM? Then leave it upon the user to
> query how many pages are actually reserved.
> >
> > The script should leave it up to the user to do another query about
> > status if they care about what the result is.  

Just call sys.exit with a message that is all that is needed.

Or maybe trapping other write errors to sysfs here. Probably the
kernel has already tried to report the error, but the try/except code
is not seeing it.
diff mbox series

Patch

diff --git a/usertools/dpdk-hugepages.py b/usertools/dpdk-hugepages.py
index 1be100ca3..83332c096 100755
--- a/usertools/dpdk-hugepages.py
+++ b/usertools/dpdk-hugepages.py
@@ -16,6 +16,8 @@ 
 # systemd mount point for huge pages
 HUGE_MOUNT = "/dev/hugepages"
 
+# show hugepages flag
+SHOW_HUGEPAGES = False
 
 def fmt_memsize(kb):
     '''Format memory size in kB into conventional format'''
@@ -62,6 +64,10 @@  def set_hugepages(path, pages):
         filename = os.path.basename(path)
         size = filename[10:]
         sys.exit('{} is not a valid system huge page size'.format(size))
+    if get_hugepages(path) != pages:
+        print("Unable to reserve required pages. The pages reserved are:")
+        global SHOW_HUGEPAGES
+        SHOW_HUGEPAGES = True
 
 
 def show_numa_pages():
@@ -233,6 +239,9 @@  def main():
         metavar='SIZE',
         help='setup huge pages by doing clear, unmount, reserve and mount')
     args = parser.parse_args()
+
+    global SHOW_HUGEPAGES
+    SHOW_HUGEPAGES = args.show
 
     if args.setup:
         args.clear = True
@@ -260,7 +269,7 @@  def main():
             int(reserve_kb / pagesize_kb), pagesize_kb, node=args.node)
     if args.mount:
         mount_huge(pagesize_kb, HUGE_MOUNT)
-    if args.show:
+    if SHOW_HUGEPAGES:
         show_pages()
         print()
         show_mount()