Re: [PATCH v7 1/3] mm: add new api to enable ksm per process

[Stefan Roesch <shr@devkernel.io>]

David Hildenbrand <david@redhat.com> writes:

> Thanks!
>
> In general,
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> Two nits below, after staring at some other prctl implementations.
>
>> +#define PR_SET_MEMORY_MERGE		67
>> +#define PR_GET_MEMORY_MERGE		68
>>   #endif /* _LINUX_PRCTL_H */
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 495cd87d9bf4..8c2e50edeb18 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/highuid.h>
>>   #include <linux/fs.h>
>>   #include <linux/kmod.h>
>> +#include <linux/ksm.h>
>>   #include <linux/perf_event.h>
>>   #include <linux/resource.h>
>>   #include <linux/kernel.h>
>> @@ -2661,6 +2662,30 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>>   	case PR_SET_VMA:
>>   		error = prctl_set_vma(arg2, arg3, arg4, arg5);
>>   		break;
>> +#ifdef CONFIG_KSM
>> +	case PR_SET_MEMORY_MERGE:
>
> Looking at some other code (PR_SET_NO_NEW_PRIVS/ PR_SET_THP_DISABLE) I wonder if
> we also want
>
> if (arg3 || arg4 || arg5)
> 	return -EINVAL;
>
I added the above check. It requires that we always specify all
parameters in the test programs. I also changed them accordingly.

> For PR_GET_MEMORY_MERGE it looks good already.
>
>> +		if (mmap_write_lock_killable(me->mm))
>> +			return -EINTR;
>> +
>> +		if (arg2) {
>> +			error = ksm_enable_merge_any(me->mm);
>> +		} else {
>> +			/*
>> +			 * TODO: we might want disable KSM on all VMAs and
>> +			 * trigger unsharing to completely disable KSM.
>> +			 */
>> +			clear_bit(MMF_VM_MERGE_ANY, &me->mm->flags);
>> +			error = 0;
>> +		}
>> +		mmap_write_unlock(me->mm);
>> +		break;
>> +	case PR_GET_MEMORY_MERGE:
>> +		if (arg2 || arg3 || arg4 || arg5)
>> +			return -EINVAL;
>> +
>> +		error = !!test_bit(MMF_VM_MERGE_ANY, &me->mm->flags);
>> +		break;
>> +#endif
>>   	default:
>>   		error = -EINVAL;
>>   		break;
>
> [...]
>
>> +/**
>> + * ksm_enable_merge_any - Add mm to mm ksm list and enable merging on all
>> + *                        compatible VMA's
>> + *
>> + * @mm:  Pointer to mm
>> + *
>> + * Returns 0 on success, otherwise error code
>> + */
>> +int ksm_enable_merge_any(struct mm_struct *mm)
>> +{
>> +	int err;
>> +
>> +	if (test_bit(MMF_VM_MERGE_ANY, &mm->flags))
>> +		return -EINVAL;
>
>
> I'm curious, why is enabling the prctl() supposed to fail if already enabled?
> (it would not fail if disabling and already disabled)
>

I changed that to not return an error in that case.
>
> For example, PR_SET_THP_DISABLE/PR_SET_NO_NEW_PRIVS doesn't fail if already set.