Re: [patch] mm, thp: add new background defrag option

[Michal Hocko]

On Thu 05-01-17 14:58:47, Vlastimil Babka wrote:
[...]
> I'm not a fan of either name, so I've tried to implement my own
> suggestion. Turns out it was easier than expected, as there's no kernel
> boot option for "defer", just for "enabled", so that particular worry
> was unfounded.
> 
> And personally I think that it's less confusing when one can enable defer
> and madvise together (and not any other combination), than having to dig
> up the difference between "defer" and "background".
> 
> I have only tested the sysfs manipulation, not actual THP, but seems to me
> that alloc_hugepage_direct_gfpmask() already happens to process the flags
> in a way that it works as expected.

IMHO this looks indeed much simpler implementation wise, more consistent
from the semantic point of view and less confusing from the usage POV.
 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 10eedbf14421..cc5ae86169a8 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -150,7 +150,16 @@ static ssize_t triple_flag_store(struct kobject *kobj,
>  				 enum transparent_hugepage_flag deferred,
>  				 enum transparent_hugepage_flag req_madv)
>  {
> -	if (!memcmp("defer", buf,
> +	if (!memcmp("defer madvise", buf,
> +			min(sizeof("defer madvise")-1, count))
> +	    || !memcmp("madvise defer", buf,
> +			min(sizeof("madvise defer")-1, count))) {
> +		if (enabled == deferred)
> +			return -EINVAL;
> +		clear_bit(enabled, &transparent_hugepage_flags);
> +		set_bit(req_madv, &transparent_hugepage_flags);
> +		set_bit(deferred, &transparent_hugepage_flags);
> +	} else if (!memcmp("defer", buf,
>  		    min(sizeof("defer")-1, count))) {
>  		if (enabled == deferred)
>  			return -EINVAL;
> @@ -251,9 +260,12 @@ static ssize_t defrag_show(struct kobject *kobj,
>  {
>  	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags))
>  		return sprintf(buf, "[always] defer madvise never\n");
> -	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags))
> -		return sprintf(buf, "always [defer] madvise never\n");
> -	else if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
> +	if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags)) {
> +		if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
> +			return sprintf(buf, "always [defer] [madvise] never\n");
> +		else
> +			return sprintf(buf, "always [defer] madvise never\n");
> +	} else if (test_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags))
>  		return sprintf(buf, "always defer [madvise] never\n");
>  	else
>  		return sprintf(buf, "always defer madvise [never]\n");

-- 
Michal Hocko
SUSE Labs