Re: [PATCH 2/2] mm: remove get_user_pages_locked()
On 31/10/2016 14:48, Lorenzo Stoakes wrote:
> On Mon, Oct 31, 2016 at 12:45:36PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 31/10/2016 11:02, Lorenzo Stoakes wrote:
>>> - *
>>> - * get_user_pages should be phased out in favor of
>>> - * get_user_pages_locked|unlocked or get_user_pages_fast. Nothing
>>> - * should use get_user_pages because it cannot pass
>>> - * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault.
>>
>> This comment should be preserved in some way. In addition, removing
>
> Could you clarify what you think should be retained?
>
> The comment seems to me to be largely rendered redundant by this change -
> get_user_pages() now offers identical behaviour, and of course the latter part
> of the comment ('because it cannot pass FAULT_FLAG_ALLOW_RETRY') is rendered
> incorrect by this change.
>
> It could be replaced with a recommendation to make use of VM_FAULT_RETRY logic
> if possible.
Yes, exactly. locked == NULL should be avoided whenever mmap_sem can
be dropped, and the comment indirectly said so. Now most of those cases
actually are those where you can just call get_user_pages_unlocked.
>> get_user_pages_locked() makes it harder (compared to a simple "git grep
>> -w") to identify callers that lack allow-retry functionality). So I'm
>> not sure about the benefits of these patches.
>
> That's a fair point, and certainly this cleanup series is less obviously
> helpful
> than previous ones.
>
> However, there are a few points in its favour:
>
> 1. get_user_pages_remote() was the mirror of get_user_pages() prior to adding
> an
>int *locked parameter to the former (necessary to allow for the unexport of
>__get_user_pages_unlocked()), differing only in task/mm being specified and
>FOLL_REMOTE being set. This patch series keeps these functions
> 'synchronised'
>in this respect.
>
> 2. There is currently only one caller of get_user_pages_locked() in
>mm/frame_vector.c which seems to suggest this function isn't widely
>used/known.
Or not widely necessary. :)
> 3. This change results in all slow-path get_user_pages*() functions having the
>ability to use VM_FAULT_RETRY logic rather than 'defaulting' to using
>get_user_pages() that doesn't let you do this even if you wanted to.
This is only true if someone does the work though. From a quick look
at your series, the following files can be trivially changed to use
get_user_pages_unlocked:
- drivers/gpu/drm/via/via_dmablit.c
- drivers/platform/goldfish/goldfish_pipe.c
- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
- drivers/rapidio/devices/rio_mport_cdev.c
- drivers/virt/fsl_hypervisor.c
Also, videobuf_dma_init_user could be changed to use retry by adding a
*locked argument to videobuf_dma_init_user_locked, prototype patch
after my signature.
Everything else is probably best kept using get_user_pages.
> 4. It's somewhat confusing/redundant having both get_user_pages_locked() and
>get_user_pages() functions which both require mmap_sem to be held (i.e.
> both
>are 'locked' versions.)
>
>> If all callers were changed, then sure removing the _locked suffix would
>> be a good idea.
>
> It seems many callers cannot release mmap_sem so VM_FAULT_RETRY logic couldn't
> happen anyway in this cases and in these cases we couldn't change the caller.
But then get_user_pages_locked remains a less-common case, so perhaps
it's a good thing to give it a longer name!
> Overall, an alternative here might be to remove get_user_pages() and update
> get_user_pages_locked() to add a 'vmas' parameter, however doing this renders
> get_user_pages_unlocked() asymmetric as it would lack an vmas parameter
> (adding
> one there would make no sense as VM_FAULT_RETRY logic invalidates VMAs) though
> perhaps not such a big issue as it makes sense as to why this is the case.
Adding the 'vmas' parameter to get_user_pages_locked would make little
sense. Since VM_FAULT_RETRY invalidates it and g_u_p_locked can and
does retry, it would generally not be useful.
So I think we have the right API now:
- do not have lock? Use get_user_pages_unlocked, get retry for free,
no need to handle mmap_sem and the locked argument; cannot get back vmas.
- have and cannot drop lock? User get_user_pages, no need to pass NULL
for the locked argument; can get back vmas.
- have but can drop lock (rare case)? Use get_user_pages_locked,
cannot get back vams.
Paolo
> get_user_pages_unlocked() definitely seems to be a useful helper and therefore
> makes sense to retain.
> Of course another alternative is to leave things be :)
>
diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c
b/drivers/media/v4l2-core/videobuf-dma-sg.c
index 1db0af6c7f94..dae4
Re: [PATCH 2/2] mm: remove get_user_pages_locked()
On 31/10/2016 11:02, Lorenzo Stoakes wrote: > - * > - * get_user_pages should be phased out in favor of > - * get_user_pages_locked|unlocked or get_user_pages_fast. Nothing > - * should use get_user_pages because it cannot pass > - * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault. This comment should be preserved in some way. In addition, removing get_user_pages_locked() makes it harder (compared to a simple "git grep -w") to identify callers that lack allow-retry functionality). So I'm not sure about the benefits of these patches. If all callers were changed, then sure removing the _locked suffix would be a good idea. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()
= 0;
> unsigned long max_pages_per_loop = PVM_MAX_KMALLOC_PAGES
> / sizeof(struct pages *);
> + unsigned int flags = FOLL_REMOTE;
>
> /* Work out address and page range required */
> if (len == 0)
> return 0;
> nr_pages = (addr + len - 1) / PAGE_SIZE - addr / PAGE_SIZE + 1;
>
> + if (vm_write)
> + flags |= FOLL_WRITE;
> +
> while (!rc && nr_pages && iov_iter_count(iter)) {
> int pages = min(nr_pages, max_pages_per_loop);
> size_t bytes;
> @@ -104,8 +108,7 @@ static int process_vm_rw_single_vec(unsigned long addr,
>* current/current->mm
>*/
> pages = __get_user_pages_unlocked(task, mm, pa, pages,
> - vm_write, 0, process_pages,
> - FOLL_REMOTE);
> + process_pages, flags);
> if (pages <= 0)
> return -EFAULT;
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index db96688..8035cc1 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -84,7 +84,8 @@ static void async_pf_execute(struct work_struct *work)
>* mm and might be done in another context, so we must
>* use FOLL_REMOTE.
>*/
> - __get_user_pages_unlocked(NULL, mm, addr, 1, 1, 0, NULL, FOLL_REMOTE);
> + __get_user_pages_unlocked(NULL, mm, addr, 1, NULL,
> + FOLL_WRITE | FOLL_REMOTE);
>
> kvm_async_page_present_sync(vcpu, apf);
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 81dfc73..28510e7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1416,10 +1416,15 @@ static int hva_to_pfn_slow(unsigned long addr, bool
> *async, bool write_fault,
> down_read(>mm->mmap_sem);
> npages = get_user_page_nowait(addr, write_fault, page);
> up_read(>mm->mmap_sem);
> - } else
> + } else {
> + unsigned int flags = FOLL_TOUCH | FOLL_HWPOISON;
> +
> + if (write_fault)
> + flags |= FOLL_WRITE;
> +
> npages = __get_user_pages_unlocked(current, current->mm, addr,
> 1,
> -write_fault, 0, page,
> -FOLL_TOUCH|FOLL_HWPOISON);
> +page, flags);
> + }
> if (npages != 1)
> return npages;
>
>
Acked-by: Paolo Bonzini <pbonz...@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken ioctl error returns (was Re: [PATCH 2/3] block: fail SCSI passthrough ioctls on partition devices)
On 01/05/2012 06:02 PM, Linus Torvalds wrote: + return ret == -EINVAL || + ret == -ENOTTY || + ret == ENOIOCTLCMD; Missing minus before ENOIOCTLCMD. Paolo -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
