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..dae4eb8e9d5b 100644
--- a/drivers/media/v4l2-core/videobuf-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c
@@ -152,7 +152,8 @@ static void videobuf_dma_init(struct videobuf_dmabuf *dma)
}
static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,
- int direction, unsigned long data, unsigned long size)
+ int direction, unsigned long data, unsigned long size,
+ int *locked)
{
unsigned long first, last;
int err, rw = 0;
@@ -185,8 +186,17 @@ static int videobuf_dma_init_user_locked(struct
videobuf_dmabuf *dma,
dprintk(1, "init user [0x%lx+0x%lx => %d pages]\n",
data, size, dma->nr_pages);
- err = get_user_pages(data & PAGE_MASK, dma->nr_pages,
- flags, dma->pages, NULL);
+ if (locked && !*locked) {
+ down_read(¤t->mm->mmap_sem);
+ *locked = 1;
+ }
+
+ /*
+ * If the caller cannot have mmap_sem dropped, it passes locked == NULL
+ * so get_user_pages_locked will not release it.
+ */
+ err = get_user_pages_locked(data & PAGE_MASK, dma->nr_pages,
+ flags, dma->pages, locked);
if (err != dma->nr_pages) {
dma->nr_pages = (err >= 0) ? err : 0;
@@ -200,10 +210,11 @@ static int videobuf_dma_init_user(struct videobuf_dmabuf
*dma, int direction,
unsigned long data, unsigned long size)
{
int ret;
+ int locked = 0;
- down_read(¤t->mm->mmap_sem);
- ret = videobuf_dma_init_user_locked(dma, direction, data, size);
- up_read(¤t->mm->mmap_sem);
+ ret = videobuf_dma_init_user_locked(dma, direction, data, size,
&locked);
+ if (locked)
+ up_read(¤t->mm->mmap_sem);
return ret;
}
@@ -540,7 +551,7 @@ static int __videobuf_iolock(struct videobuf_queue *q,
err = videobuf_dma_init_user_locked(&mem->dma,
DMA_FROM_DEVICE,
- vb->baddr, vb->bsize);
+ vb->baddr, vb->bsize,
NULL);
if (0 != err)
return err;
}
