Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue

[Kirti Wankhede]

On 4/18/2017 8:24 AM, Peter Xu wrote:
> On Mon, Apr 17, 2017 at 03:32:20PM -0600, Alex Williamson wrote:
>> On Tue, 18 Apr 2017 01:02:12 +0530
>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>
>>> On 4/18/2017 12:49 AM, Alex Williamson wrote:
>>>> On Tue, 18 Apr 2017 00:35:06 +0530
>>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>>>   
>>>>> On 4/17/2017 8:02 PM, Alex Williamson wrote:  
>>>>>> On Mon, 17 Apr 2017 14:47:54 +0800
>>>>>> Peter Xu <peterx@xxxxxxxxxx> wrote:
>>>>>>     
>>>>>>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>    
>>>>>>>> -static void vfio_lock_acct(struct task_struct *task, long npage)
>>>>>>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap)
>>>>>>>>  {
>>>>>>>> -	struct vwork *vwork;
>>>>>>>>  	struct mm_struct *mm;
>>>>>>>>  	bool is_current;
>>>>>>>> +	int ret;
>>>>>>>>  
>>>>>>>>  	if (!npage)
>>>>>>>> -		return;
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>>  	is_current = (task->mm == current->mm);
>>>>>>>>  
>>>>>>>>  	mm = is_current ? task->mm : get_task_mm(task);
>>>>>>>>  	if (!mm)
>>>>>>>> -		return; /* process exited */
>>>>>>>> +		return -ESRCH; /* process exited */
>>>>>>>>  
>>>>>>>> -	if (down_write_trylock(&mm->mmap_sem)) {
>>>>>>>> -		mm->locked_vm += npage;
>>>>>>>> -		up_write(&mm->mmap_sem);
>>>>>>>> -		if (!is_current)
>>>>>>>> -			mmput(mm);
>>>>>>>> -		return;
>>>>>>>> -	}
>>>>>>>> +	ret = down_write_killable(&mm->mmap_sem);
>>>>>>>> +	if (!ret) {
>>>>>>>> +		if (npage < 0 || lock_cap) {      
>>>>>>>
>>>>>>> Nit: maybe we can avoid passing in lock_cap in all the callers of
>>>>>>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0?
>>>>>>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't
>>>>>>> need to pass in "false" any time when doing unpins.    
>>>>>>
>>>>>> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap
>>>>>> since it tests whether the user is exceeding their locked memory
>>>>>> limit.  The other callers could certainly get away with
>>>>>> vfio_lock_acct() testing the capability itself but that would add a
>>>>>> redundant call for the most common user.  I'm not a big fan of passing
>>>>>> a lock_cap bool either, but it seemed the best fix for now.  The
>>>>>> cleanest alternative I can up with is this (untested):
>>>>>>     
>>>>>
>>>>> In my opinion, passing 'bool lock_cap' looks much clean and simple.
>>>>>
>>>>> Reviewed-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>  
>>>>
>>>> Well shoot, I was just starting to warm up to the bool*.  I like that
>>>> we're not presuming the polarity for the callers we expect to be
>>>> removing pages and I generally just dislike passing fixed bool
>>>> parameters to change the function behavior.  I've cleaned it up a bit
>>>> further and was starting to do some testing on this which I'd propose
>>>> for v5.  Does it change your opinion?  
>>>
>>> If passing fixed bool parameter is the concern then I would lean towards
>>> Peter's suggestion. vfio_pin_pages_remote() will check lock capability
>>> outside vfio_lock_acct() and again in vfio_lock_acct(). At other places,
>>> it will be takes care within vfio_lock_acct()
>>
>> Sorry, I don't see that as a viable option.  Testing for CAP_IPC_LOCK in
>> both vfio_pin_pages_remote() and vfio_lock_acct() results in over a
>> 10% performance hit on the mapping path with a custom micro-benchmark.
>> In fact, it suggests we should probably pass that from even higher in
>> the call stack.  Thanks,
> 
> Sorry I wasn't aware of such a performance degradation with such a
> change. Then I would be perfectly fine with either current patch, or
> the new one you proposed (with bool *). Thanks,
> 

Sorry, even I wasn't aware of.
Looking at v5 version now.

Thanks,
Kirti