Re: [PATCH V2 3/4] hugetlbfs: accept subpool min_size mount option and setup accordingly

From: Mike Kravetz
Date: Wed Mar 18 2015 - 21:34:48 EST

On 03/18/2015 02:40 PM, Andrew Morton wrote:
On Mon, 16 Mar 2015 16:53:28 -0700 Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:

Make 'min_size=' be an option when mounting a hugetlbfs. This option
takes the same value as the 'size' option. min_size can be specified
with specifying size. If both are specified, min_size must be less
that or equal to size else the mount will fail. If min_size is
specified, then at mount time an attempt is made to reserve min_size
pages. If the reservation fails, the mount fails. At umount time,
the reserved pages are released.

...

@@ -761,14 +763,32 @@ static const struct super_operations hugetlbfs_ops = {
.show_options = generic_show_options,
};

+enum { NO_SIZE, SIZE_STD, SIZE_PERCENT };
+
+static bool
+hugetlbfs_options_setsize(struct hstate *h, long long *size, int setsize)
+{
+ if (setsize == NO_SIZE)
+ return false;
+
+ if (setsize == SIZE_PERCENT) {
+ *size <<= huge_page_shift(h);
+ *size *= h->max_huge_pages;
+ do_div(*size, 100);

I suppose do_div() takes a long long. u64 would be more conventional.
I don't *think* all this code needed to use signed types.

+ }
+
+ *size >>= huge_page_shift(h);
+ return true;
+}
+
static int
hugetlbfs_parse_options(char *options, struct hugetlbfs_config *pconfig)
{
char *p, *rest;
substring_t args[MAX_OPT_ARGS];
int option;
- unsigned long long size = 0;
- enum { NO_SIZE, SIZE_STD, SIZE_PERCENT } setsize = NO_SIZE;
+ unsigned long long max_size = 0, min_size = 0;
+ int max_setsize = NO_SIZE, min_setsize = NO_SIZE;

if (!options)
return 0;
@@ -806,10 +826,10 @@ hugetlbfs_parse_options(char *options, struct hugetlbfs_config *pconfig)
/* memparse() will accept a K/M/G without a digit */
if (!isdigit(*args[0].from))
goto bad_val;
- size = memparse(args[0].from, &rest);
- setsize = SIZE_STD;
+ max_size = memparse(args[0].from, &rest);
+ max_setsize = SIZE_STD;
if (*rest == '%')
- setsize = SIZE_PERCENT;
+ max_setsize = SIZE_PERCENT;
break;
}

@@ -832,6 +852,17 @@ hugetlbfs_parse_options(char *options, struct hugetlbfs_config *pconfig)
break;
}

+ case Opt_min_size: {
+ /* memparse() will accept a K/M/G without a digit */
+ if (!isdigit(*args[0].from))
+ goto bad_val;
+ min_size = memparse(args[0].from, &rest);
+ min_setsize = SIZE_STD;
+ if (*rest == '%')
+ min_setsize = SIZE_PERCENT;
+ break;
+ }
+
default:
pr_err("Bad mount option: \"%s\"\n", p);
return -EINVAL;
@@ -839,15 +870,17 @@ hugetlbfs_parse_options(char *options, struct hugetlbfs_config *pconfig)
}
}

- /* Do size after hstate is set up */
- if (setsize > NO_SIZE) {
- struct hstate *h = pconfig->hstate;
- if (setsize == SIZE_PERCENT) {
- size <<= huge_page_shift(h);
- size *= h->max_huge_pages;
- do_div(size, 100);
- }
- pconfig->nr_blocks = (size >> huge_page_shift(h));
+ /* Calculate number of huge pages based on hstate */
+ if (hugetlbfs_options_setsize(pconfig->hstate, &max_size, max_setsize))
+ pconfig->nr_blocks = max_size;

So hugetlbfs_options_setsize takes an arg whichis in units of bytes,
modifies it in-place to b in units of pages and then copies it into
something which is in units of nr_blocks.


+ if (hugetlbfs_options_setsize(pconfig->hstate, &min_size, min_setsize))
+ pconfig->min_size = min_size;
+
+ /* If max_size specified, then min_size must be smaller */
+ if (max_setsize > NO_SIZE && min_setsize > NO_SIZE &&
+ pconfig->min_size > pconfig->nr_blocks) {
+ pr_err("minimum size can not be greater than maximum size\n");
+ return -EINVAL;
}

return 0;
@@ -872,6 +905,7 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
config.gid = current_fsgid();
config.mode = 0755;
config.hstate = &default_hstate;
+ config.min_size = 0; /* No default minimum size */
ret = hugetlbfs_parse_options(data, &config);
if (ret)
return ret;
@@ -885,8 +919,15 @@ hugetlbfs_fill_super(struct super_block *sb, void *data, int silent)
sbinfo->max_inodes = config.nr_inodes;
sbinfo->free_inodes = config.nr_inodes;
sbinfo->spool = NULL;
- if (config.nr_blocks != -1) {
- sbinfo->spool = hugepage_new_subpool(config.nr_blocks);
+ /*
+ * Allocate and initialize subpool if maximum or minimum size is
+ * specified. Any needed reservations (for minimim size) are taken
+ * taken when the subpool is created.
+ */
+ if (config.nr_blocks != -1 || config.min_size != 0) {
+ sbinfo->spool = hugepage_new_subpool(config.hstate,
+ config.nr_blocks,
+ config.min_size);

And hugepage_new_subpool() takes something in units of nr_blocks and
copies it into something whcih has units of nr-hugepages.

And it takes an arg called "size" which is no longer number-of-bytes
but is actually number-of-hpages.


It's all rather confusing and unclear. A good philosophy would be
never to use a variable called "size", because the reader doesn't know
what units that size is measured in. Instead, make sure that the name
reflects the variable's units. max_bytes, min_hpages, nr_blocks, etc.


Thanks for the comments.

I didn't want to cut/paste/duplicate the code used to parse the existing
size option. But, it looks like I made it harder to understand. I'll
take a pass as cleaning this up and making it more clear.

--
Mike Kravetz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/