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

[Andrew Morton]

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.

--
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/