On 01/10, Alex Thorlton wrote:
>
> This patch adds an mm flag (MMF_THP_DISABLE) to disable transparent
> hugepages using prctl. It is based on my original patch to add a
> per-task_struct flag to disable THP:
I leave the "whether we need this feature" to other reviewers, although
personally I think it probably makes sense anyway.
But the patch doesn't look nice imho.
> @@ -373,7 +373,15 @@ extern int get_dumpable(struct mm_struct *mm);
> #define MMF_HAS_UPROBES 19 /* has uprobes */
> #define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +#define MMF_THP_DISABLE 21 /* disable THP for this mm */
> +#define MMF_THP_DISABLE_MASK (1 << MMF_THP_DISABLE)
> +
> +#define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK | MMF_THP_DISABLE_MASK)
> +#else
> #define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
> +#endif
It would be nice to lessen the number of ifdef's. Why we can't define
MMF_THP_DISABLE unconditionally and include it into MMF_INIT_MASK?
Or define it == 0 if !CONFIG_THP. But this is minor.
> +#define PR_SET_THP_DISABLE 41
> +#define PR_CLEAR_THP_DISABLE 42
> +#define PR_GET_THP_DISABLE 43
Why we can't add 2 PR_'s, set and get?
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -818,6 +818,7 @@ struct mm_struct *dup_mm(struct task_struct *tsk)
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && !USE_SPLIT_PMD_PTLOCKS
> mm->pmd_huge_pte = NULL;
> #endif
> +
> if (!mm_init(mm, tsk))
> goto fail_nomem;
Why? looks like the accidental change.
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1835,6 +1835,42 @@ static int prctl_get_tid_address(struct task_struct *me, int __user **tid_addr)
> }
> #endif
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static int prctl_set_thp_disable(struct task_struct *me)
> +{
> + set_bit(MMF_THP_DISABLE, &me->mm->flags);
> + return 0;
> +}
> +
> +static int prctl_clear_thp_disable(struct task_struct *me)
> +{
> + clear_bit(MMF_THP_DISABLE, &me->mm->flags);
> + return 0;
> +}
> +
> +static int prctl_get_thp_disable(struct task_struct *me,
> + int __user *thp_disabled)
> +{
> + return put_user(test_bit(MMF_THP_DISABLE, &me->mm->flags), thp_disabled);
> +}
> +#else
> +static int prctl_set_thp_disable(struct task_struct *me)
> +{
> + return -EINVAL;
> +}
> +
> +static int prctl_clear_thp_disable(struct task_struct *me)
> +{
> + return -EINVAL;
> +}
> +
> +static int prctl_get_thp_disable(struct task_struct *me,
> + int __user *thp_disabled)
> +{
> + return -EINVAL;
> +}
> +#endif
> +
> SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -1998,6 +2034,15 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> if (arg2 || arg3 || arg4 || arg5)
> return -EINVAL;
> return current->no_new_privs ? 1 : 0;
> + case PR_SET_THP_DISABLE:
> + error = prctl_set_thp_disable(me);
> + break;
> + case PR_CLEAR_THP_DISABLE:
> + error = prctl_clear_thp_disable(me);
> + break;
> + case PR_GET_THP_DISABLE:
> + error = prctl_get_thp_disable(me, (int __user *) arg2);
> + break;
> default:
> error = -EINVAL;
> break;
I simply can't understand, this all looks like overkill. Can't you simply add
#idfef CONFIG_TRANSPARENT_HUGEPAGE
case GET:
error = test_bit(MMF_THP_DISABLE);
break;
case PUT:
if (arg2)
set_bit();
else
clear_bit();
break;
#endif
into sys_prctl() ?
Oleg.
--
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>