On Fri, Mar 20, 2009 at 05:48:25PM -0700, Avantika Mathur wrote:
> Mel Gorman wrote:
>
>> On Thu, Mar 19, 2009 at 09:53:39PM -0700, Avantika Mathur wrote:
>>
>>
>> Patch cleaniness issues. Not a massive deal here but a kernel
>> submission patch would attract complaint and we try and keep to the same
>> standard.
>>
>> Rename subject to something like
>>
>> hugeadm: Retry allocation of hugepages to static pool on failure when --hard
>> is specified
>>
>>
> Thank you for the feedback! I've updated the patch integrating your
> comments. Some comments and the new patch are below.
> Growing the hugepage pool with --pool-pages-min can fail due to the
> fragmentation within the system but one failure is not a guarantee of
> future failure, particularly if a very large amount of memory has to be
> reclaimed to satisfy the allocation. This patch adds a --hard flag to
> retry allocations multiple times. It initially tries to resize the pool
> up to 5 times. It continues to try if progress is being made towards
> the resize.
>
> Signed-off-by: Avantika Mathur <mat...@us.ibm.com>
> ---
> Index: libhugetlbfs/hugeadm.c
> ===================================================================
> --- libhugetlbfs.orig/hugeadm.c 2009-03-20 17:10:02.000000000 -0700
> +++ libhugetlbfs/hugeadm.c 2009-03-20 17:35:29.000000000 -0700
> @@ -67,6 +67,9 @@
>
> OPTION("--list-all-mounts", "List all current hugetlbfs mount points");
> OPTION("--pool-list", "List all pools");
> + OPTION("--hard", "specified before --pool-pages-min to make");
> + CONT("multiple attempts at adjusting the pool size to the");
> + CONT("specified count on failure");
I still think the ordering restriction is weak. It's worth going to the
effort to create an opt_minadj and setting that to 1 when LONG_POOL_MIN_ADJ
specified and doing the resize later when you know whether --hard has been
specified or not. It shouldn't complicate the patch that much more but make
the tool a bit easier to use.
> OPTION("--pool-pages-min <size>:[+|-]<count>", "");
> CONT("Adjust pool 'size' lower bound");
> OPTION("--pool-pages-max <size>:[+|-]<count>", "");
> @@ -97,6 +100,7 @@
> }
>
> int opt_dry_run = 0;
> +int opt_hard = 0;
>
> /*
> * getopts return values for options which are long only.
> @@ -106,6 +110,8 @@
> #define LONG_POOL_MIN_ADJ (LONG_POOL|'m')
> #define LONG_POOL_MAX_ADJ (LONG_POOL|'M')
>
> +#define LONG_HARD ('h' << 8)
> +
> #define LONG_PAGE ('P' << 8)
> #define LONG_PAGE_SIZES (LONG_PAGE|'s')
> #define LONG_PAGE_AVAIL (LONG_PAGE|'a')
> @@ -465,6 +471,7 @@
>
> unsigned long min;
> unsigned long max;
> + unsigned long last_pool_value;
>
> /* Extract the pagesize and adjustment. */
> page_size_str = strtok_r(cmd, ":", &iter);
> @@ -518,16 +525,38 @@
> INFO("setting HUGEPAGES_OC to %ld\n", (max - min));
> set_huge_page_counter(page_size, HUGEPAGES_OC, (max - min));
> }
> - if (pools[pos].minimum != min) {
> - INFO("setting HUGEPAGES_TOTAL to %ld\n", min);
> +
> + if (opt_hard)
> + cnt = 5;
> + else
> + cnt = -1;
> +
> + INFO("setting HUGEPAGES_TOTAL to %ld\n", min);
> + set_huge_page_counter(page_size, HUGEPAGES_TOTAL, min);
> + get_pool_size(page_size, &pools[pos]);
> +
> + /* If we fail to make an allocation, retry if user requests */
> + last_pool_value = pools[pos].minimum;
> + while ((pools[pos].minimum != min) && (cnt > 0)) {
> + /* Make note if progress is being made and sleep for IO */
> + if (last_pool_value == pools[pos].minimum)
> + cnt--;
> + else
> + cnt = 5;
> + sleep(6);
> +
> + last_pool_value = pools[pos].minimum;
> + INFO("Retrying allocation HUGEPAGES_TOTAL to %ld current %ld\n",
> + min,
> pools[pos].minimum);
> set_huge_page_counter(page_size, HUGEPAGES_TOTAL, min);
> + get_pool_size(page_size, &pools[pos]);
> }
Ok, this looks like it does the right thing and tests appear to confirm.
However, I am somewhat aghast to note that you can't set the verbosity
level of hugeadm, only on hugectl so you can't make those INFO messages
display for example. This is a separate problem from your patch though.
> +
> /*
> * HUGEPAGES_TOTAL is not guarenteed to check to exactly the figure
> * requested should there be insufficient pages. Check the new
> * value and adjust HUGEPAGES_OC accordingly.
> */
> - get_pool_size(page_size, &pools[pos]);
> if (pools[pos].minimum != min) {
> WARNING("failed to set pool minimum to %ld became %ld\n",
> min, pools[pos].minimum);
> @@ -574,6 +603,7 @@
> {"pool-list", no_argument, NULL, LONG_POOL_LIST},
> {"pool-pages-min", required_argument, NULL, LONG_POOL_MIN_ADJ},
> {"pool-pages-max", required_argument, NULL, LONG_POOL_MAX_ADJ},
> + {"hard", no_argument, NULL, LONG_HARD},
> {"create-mounts", no_argument, NULL, LONG_CREATE_MOUNTS},
> {"create-user-mounts", required_argument, NULL,
> LONG_CREATE_USER_MOUNTS},
> {"create-group-mounts", required_argument, NULL,
> LONG_CREATE_GROUP_MOUNTS},
> @@ -619,6 +649,10 @@
> case -1:
> break;
>
> + case LONG_HARD:
> + opt_hard = 1;
> + continue;
> +
> case LONG_LIST_ALL_MOUNTS:
> mounts_list_all();
> break;
Fix it up so that --hard can be specified anywhere and I'll be happy.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
------------------------------------------------------------------------------
Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are
powering Web 2.0 with engaging, cross-platform capabilities. Quickly and
easily build your RIAs with Flex Builder, the Eclipse(TM)based development
software that enables intelligent coding and step-through debugging.
Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com
_______________________________________________
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
