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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]