Re: [dpdk-dev] [PATCH v2] usertools: add huge page setup script

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Sep 03, 2020 at 03:48:31PM -0700, Stephen Hemminger wrote:
> This is an improved version of the setup of huge pages
> bases on earlier DPDK setup. Differences are:
>    * it autodetects NUMA vs non NUMA
>    * it allows setting different page sizes
>      recent kernels support multiple sizes.
>    * it accepts a parameter in bytes (not pages).
> 
> If necessary the steps of clearing old settings and mounting/umounting
> can be done individually.
> 
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Overall looks really good and readable! Thanks for this. Couple of comments
inline below.

/Bruce

> v2 -- rewrite in python
> 	The script is python3 only because supporting older versions
> 	no longer makes any sense.
> 
>  usertools/hugepage-setup.py | 317 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 317 insertions(+)

<snip>

> +def set_numa_pages(nr_pages, hugepgsz):
> +    for n in glob.glob('/sys/devices/system/node/node*/hugepages'):
> +        path = '{}/hugepages-{}kB'.format(n, hugepgsz)
> +        if not exists(path):
> +            sys.exit(
> +                '{}Kb is not a valid system huge page size'.format(hugepgsz))
> +
> +        with open(path + '/nr_hugepages', 'w') as f:
> +            f.write('{}\n'.format(nr_pages))
> +
> +
> +def set_non_numa_pages(nr_pages, hugepgsz):
> +    path = '/sys/kernel/mm/hugepages/hugepages-{}kB'.format(hugepgsz)
> +    if not exists(path):
> +        sys.exit('{}Kb is not a valid system huge page size'.format(hugepgsz))
> +
> +    with open(path + '/nr_hugepages', 'w') as f:
> +        f.write('{}\n'.format(nr_pages))
> +
> +
> +def set_pages(pages, hugepgsz):
> +    '''Sets the numberof huge pages to be reserved'''
> +    if is_numa():
> +        set_numa_pages(pages, hugepgsz)
> +    else:
> +        set_non_numa_pages(pages, hugepgsz)
> +

I'm not sure I agree with this behaviour for numa nodes. When a size is
specified on a numa system we probably don't want to reserve that size on
all nodes. I think one of two other options actually makes more sense:
1. Divide up the allocation equally between all nodes
2. Require the user to specify a numa node for the allocation.

Option #2 is best, I think.


> +
> +def mount_huge(pagesize):
> +    global hugedir
> +    cmd = "mount -t hugetlbfs" + hugedir
> +    if pagesize:
> +        cmd += ' -o pagesize={}'.format(pagesize)
> +    cmd += ' nodev {}'.format(hugedir)
> +    os.system(cmd)
> +
> +
> +def show_mount():
> +    mounted = None
> +    with open('/proc/mounts') as f:
> +        for line in f:
> +            fields = line.split()
> +            if fields[2] != 'hugetlbfs':
> +                continue
> +            if not mounted:
> +                print("Hugepages mounted on:", end=" ")
> +                mounted = True
> +            print(fields[1], end=" ")
> +    if mounted:
> +        print()
> +    else:
> +        print("Hugepages not mounted")
> +
> +
> +def parse_args():
> +    '''Parses the command-line arguments given by the user and takes the
> +    appropriate action for each'''
> +    global clear_flag
> +    global show_flag
> +    global reserve_kb
> +    global hugepagesize_kb
> +    global args
> +
> +    if len(sys.argv) <= 1:
> +        usage()
> +        sys.exit(0)
> +
> +    try:
> +        opts, args = getopt.getopt(sys.argv[1:], "r:p:csmu", [
> +            "help", "usage", "show", "clear", "setup=", "eserve=", "pagesize=",

Typo -> "eserve"

> +            "mount", "unmount"
> +        ])
> +    except getopt.GetoptError as error:
> +        print(str(error))
> +        print("Run '%s --usage' for further information" % sys.argv[0])
> +        sys.exit(1)
> +
> +    for opt, arg in opts:
> +        if opt == "--help" or opt == "--usage":
> +            usage()
> +            sys.exit(0)
> +        if opt == "--setup":
> +            clear_flag = True
> +            unmount_flag = True
> +            reserve_kb = get_memsize(arg)
> +            mount_flag = True
> +        if opt == "--show" or opt == "-s":
> +            show_flag = True
> +        if opt == "--clear" or opt == "-c":
> +            clear_flag = True
> +        if opt == "--reserve" or opt == "-r":
> +            reserve_kb = get_memsize(arg)
> +        if opt == "--pagesize" or opt == "-p":
> +            hugepagesize_kb = get_memsize(arg)
> +        if opt == "--unmount" or opt == "-u":
> +            unmount_flag = True
> +        if opt == "--mount" or opt == "-m":
> +            mount_flag = True
> +

I think the trend in python is to use argparse rather than getopt, though
personally I don't have strong feelings about the issue.

> +
> +def do_arg_actions():
> +    '''do the actual action requested by the user'''
> +    global clear_flag
> +    global show_flag
> +    global hugepagesize_kb
> +    global reserve_kb
> +
> +    if clear_flag:
> +        clear_pages()
> +    if unmount_flag:
> +        os.system("umount " + hugedir)
> +    if reserve_kb:
> +        if hugepagesize_kb is None:
> +            hugepagesize_kb = default_size()
> +        if reserve_kb % hugepagesize_kb != 0:
> +            sys.exit('{} is not a multiple of page size {}'.format(
> +                reserve_kb, hugepagesize_kb))
> +        nr_pages = int(reserve_kb / hugepagesize_kb)
> +        set_pages(nr_pages, hugepagesize_kb)
> +    if mount_flag:
> +        mount_huge(hugepagesize_kb * 1024)
> +    if show_flag:
> +        show_pages()
> +        print()
> +        show_mount()
> +
> +
> +def main():
> +    parse_args()
> +    do_arg_actions()
> +
> +
> +if __name__ == "__main__":
> +    main()
> --