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

[Stefan Roesch <shr@devkernel.io>]

Matthew Wilcox <willy@infradead.org> writes:

> On Tue, Apr 11, 2023 at 08:16:46PM -0700, Stefan Roesch wrote:
>>  	case PR_SET_VMA:
>>  		error = prctl_set_vma(arg2, arg3, arg4, arg5);
>>  		break;
>> +#ifdef CONFIG_KSM
>> +	case PR_SET_MEMORY_MERGE:
>> +		if (mmap_write_lock_killable(me->mm))
>> +			return -EINTR;
>> +
>> +		if (arg2) {
>> +			int err = ksm_add_mm(me->mm);
>> +
>> +			if (!err)
>> +				ksm_add_vmas(me->mm);
>
> in the last version of this patch, you reported the error.  Now you
> swallow the error.  I have no idea which is correct, but you've
> changed the behaviour without explaining it, so I assume it's wrong.
>

I don't see how the error is swallowed in the arg2 case. If there is
an error ksm_add_vmas is not executedd and at the end of the function
the error is returned. Am I missing something?

>> +		} else {
>> +			clear_bit(MMF_VM_MERGE_ANY, &me->mm->flags);
>> +		}
>> +		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;
>
> Why do we need a GET?  Just for symmetry, or is there an actual need for
> it?

There are three reasons:
- For symmetry
- The ksm sharing is inherited by child processes. This allows the test
  programs to verify that this is working.
- For child processes it might be useful to have the ability to check if
  ksm sharing has been enabled