| Message ID | 20170227215008.21457-1-lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 27, 2017 at 09:50:08PM +0000, Lorenzo Stoakes wrote: > Moving from get_user_pages() to get_user_pages_unlocked() simplifies the code > and takes advantage of VM_FAULT_RETRY functionality when faulting in pages. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> Queued for 4.12, thanks for the patch. -Daniel > --- > drivers/gpu/drm/via/via_dmablit.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c > index 1a3ad769f8c8..98aae9809249 100644 > --- a/drivers/gpu/drm/via/via_dmablit.c > +++ b/drivers/gpu/drm/via/via_dmablit.c > @@ -238,13 +238,9 @@ via_lock_all_dma_pages(drm_via_sg_info_t *vsg, drm_via_dmablit_t *xfer) > vsg->pages = vzalloc(sizeof(struct page *) * vsg->num_pages); > if (NULL == vsg->pages) > return -ENOMEM; > - down_read(¤t->mm->mmap_sem); > - ret = get_user_pages((unsigned long)xfer->mem_addr, > - vsg->num_pages, > - (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0, > - vsg->pages, NULL); > - > - up_read(¤t->mm->mmap_sem); > + ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr, > + vsg->num_pages, vsg->pages, > + (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0); > if (ret != vsg->num_pages) { > if (ret < 0) > return ret; > -- > 2.11.1 >
On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote: > > + ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr, > > + vsg->num_pages, vsg->pages, > > + (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0); Umm... Why not ret = get_user_pages_fast((unsigned long)xfer->mem_addr, vsg->num_pages, vsg->direction == DMA_FROM_DEVICE, vsg->pages); IOW, do you really need a warranty that ->mmap_sem will be grabbed and released?
On 28 February 2017 at 19:35, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote: > >> > + ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr, >> > + vsg->num_pages, vsg->pages, >> > + (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0); > > Umm... Why not > ret = get_user_pages_fast((unsigned long)xfer->mem_addr, > vsg->num_pages, > vsg->direction == DMA_FROM_DEVICE, > vsg->pages); > > IOW, do you really need a warranty that ->mmap_sem will be grabbed and > released? Daniel will be better placed to answer in this specific case, but more generally is there any reason why we can't just use get_user_pages_fast() in all such cases? These patches were simply a mechanical/cautious replacement for code that is more or less exactly equivalent but if this would make sense perhaps it'd be worth using gup_fast() where possible?
On Tue, Feb 28, 2017 at 08:28:08PM +0000, Lorenzo Stoakes wrote: > On 28 February 2017 at 19:35, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Feb 28, 2017 at 10:01:10AM +0100, Daniel Vetter wrote: > > > >> > + ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr, > >> > + vsg->num_pages, vsg->pages, > >> > + (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0); > > > > Umm... Why not > > ret = get_user_pages_fast((unsigned long)xfer->mem_addr, > > vsg->num_pages, > > vsg->direction == DMA_FROM_DEVICE, > > vsg->pages); > > > > IOW, do you really need a warranty that ->mmap_sem will be grabbed and > > released? > > Daniel will be better placed to answer in this specific case, but more > generally is there any reason why we can't just use > get_user_pages_fast() in all such cases? These patches were simply a > mechanical/cautious replacement for code that is more or less exactly > equivalent but if this would make sense perhaps it'd be worth using > gup_fast() where possible? I have no idea. drm/via is unmaintained, it's a dri1 racy driver with problems probably everywhere, and I'm not sure we even have someone left who cares (there's an out-of-tree kms conversion of via, but it's stuck since years). In short, it's the drm dungeons and the only reason I merge patches is to give people an easy target for test driving the patch submission process to dri-devel. And to avoid drm being a blocker for tree-wide refactorings. Otherwise 0 reasons to change anything here. -Daniel
diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c index 1a3ad769f8c8..98aae9809249 100644 --- a/drivers/gpu/drm/via/via_dmablit.c +++ b/drivers/gpu/drm/via/via_dmablit.c @@ -238,13 +238,9 @@ via_lock_all_dma_pages(drm_via_sg_info_t *vsg, drm_via_dmablit_t *xfer) vsg->pages = vzalloc(sizeof(struct page *) * vsg->num_pages); if (NULL == vsg->pages) return -ENOMEM; - down_read(¤t->mm->mmap_sem); - ret = get_user_pages((unsigned long)xfer->mem_addr, - vsg->num_pages, - (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0, - vsg->pages, NULL); - - up_read(¤t->mm->mmap_sem); + ret = get_user_pages_unlocked((unsigned long)xfer->mem_addr, + vsg->num_pages, vsg->pages, + (vsg->direction == DMA_FROM_DEVICE) ? FOLL_WRITE : 0); if (ret != vsg->num_pages) { if (ret < 0) return ret;
Moving from get_user_pages() to get_user_pages_unlocked() simplifies the code and takes advantage of VM_FAULT_RETRY functionality when faulting in pages. Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- drivers/gpu/drm/via/via_dmablit.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)