* [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() [not found] <1380724087-13927-1-git-send-email-jack@suse.cz> @ 2013-10-02 14:27 ` Jan Kara 2013-10-02 19:41 ` Laurent Pinchart 2013-10-02 14:28 ` [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() Jan Kara 1 sibling, 1 reply; 6+ messages in thread From: Jan Kara @ 2013-10-02 14:27 UTC (permalink / raw) To: LKML; +Cc: linux-mm, Jan Kara, Laurent Pinchart, linux-media CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com> CC: linux-media@vger.kernel.org Signed-off-by: Jan Kara <jack@suse.cz> --- drivers/media/platform/omap3isp/ispqueue.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/media/platform/omap3isp/ispqueue.c b/drivers/media/platform/omap3isp/ispqueue.c index e15f01342058..bed380395e6c 100644 --- a/drivers/media/platform/omap3isp/ispqueue.c +++ b/drivers/media/platform/omap3isp/ispqueue.c @@ -331,13 +331,9 @@ static int isp_video_buffer_prepare_user(struct isp_video_buffer *buf) if (buf->pages == NULL) return -ENOMEM; - down_read(¤t->mm->mmap_sem); - ret = get_user_pages(current, current->mm, data & PAGE_MASK, - buf->npages, - buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, 0, - buf->pages, NULL); - up_read(¤t->mm->mmap_sem); - + ret = get_user_pages_fast(data & PAGE_MASK, buf->npages, + buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, + buf->pages); if (ret != buf->npages) { buf->npages = ret < 0 ? 0 : ret; isp_video_buffer_cleanup(buf); -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() 2013-10-02 14:27 ` [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() Jan Kara @ 2013-10-02 19:41 ` Laurent Pinchart 2013-10-02 20:18 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2013-10-02 19:41 UTC (permalink / raw) To: Jan Kara; +Cc: LKML, linux-mm, linux-media Hi Jan, Thank you for the patch. On Wednesday 02 October 2013 16:27:46 Jan Kara wrote: > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > CC: linux-media@vger.kernel.org > Signed-off-by: Jan Kara <jack@suse.cz> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Could you briefly explain where you're headed with this ? The V4L2 subsystem has suffered for quite a long time from potentiel AB-BA deadlocks, due the drivers taking the mmap_sem semaphore while holding the V4L2 buffers queue lock in the code path below, and taking them in reverse order in the mmap() path (as the mm core takes the mmap_sem semaphore before calling the mmap() operation). We've solved that by releasing the V4L2 buffers queue lock before taking the mmap_sem lock below and taking it back after releasing the mmap_sem lock. Having an explicit indication that the mmap_sem lock was taken helped keeping the problem in mind. I'm not against hiding it in get_user_pages_fast(), but I'd like to know what the next step is. Maybe it would also be worth it adding a /* get_user_pages_fast() takes the mmap_sem */ comment before the call ? > --- > drivers/media/platform/omap3isp/ispqueue.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispqueue.c > b/drivers/media/platform/omap3isp/ispqueue.c index > e15f01342058..bed380395e6c 100644 > --- a/drivers/media/platform/omap3isp/ispqueue.c > +++ b/drivers/media/platform/omap3isp/ispqueue.c > @@ -331,13 +331,9 @@ static int isp_video_buffer_prepare_user(struct > isp_video_buffer *buf) if (buf->pages == NULL) > return -ENOMEM; > > - down_read(¤t->mm->mmap_sem); > - ret = get_user_pages(current, current->mm, data & PAGE_MASK, > - buf->npages, > - buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, 0, > - buf->pages, NULL); > - up_read(¤t->mm->mmap_sem); > - > + ret = get_user_pages_fast(data & PAGE_MASK, buf->npages, > + buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, > + buf->pages); > if (ret != buf->npages) { > buf->npages = ret < 0 ? 0 : ret; > isp_video_buffer_cleanup(buf); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() 2013-10-02 19:41 ` Laurent Pinchart @ 2013-10-02 20:18 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2013-10-02 20:18 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Jan Kara, LKML, linux-mm, linux-media On Wed 02-10-13 21:41:10, Laurent Pinchart wrote: > Hi Jan, > > Thank you for the patch. > > On Wednesday 02 October 2013 16:27:46 Jan Kara wrote: > > CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > CC: linux-media@vger.kernel.org > > Signed-off-by: Jan Kara <jack@suse.cz> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > > Could you briefly explain where you're headed with this ? My motivation is that currently filesystems have to workaround locking problems with ->page_mkwrite() (the write page fault handler) being called with mmap_sem held. The plan is to provide ability to ->page_mkwrite() handler to drop mmap_sem. And that needs an audit of all get_user_pages() callers to verify they won't be broken by this locking change. So I've started with making this audit simpler. > The V4L2 subsystem has suffered for quite a long time from potentiel > AB-BA deadlocks, due the drivers taking the mmap_sem semaphore while > holding the V4L2 buffers queue lock in the code path below, and taking > them in reverse order in the mmap() path (as the mm core takes the > mmap_sem semaphore before calling the mmap() operation). Yeah, I've read about this in some comment in V4L2. Also there are some places (drivers/media/platform/omap/omap_vout.c and drivers/media/v4l2-core/) which acquire mmap_sem pretty early to avoid lock inversion with the queue lock. These are actually causing me quite some headache :) > We've solved that by releasing the V4L2 buffers queue lock before > taking the mmap_sem lock below and taking it back after releasing the mmap_sem > lock. Having an explicit indication that the mmap_sem lock was taken helped > keeping the problem in mind. I'm not against hiding it in > get_user_pages_fast(), but I'd like to know what the next step is. Maybe it > would also be worth it adding a /* get_user_pages_fast() takes the mmap_sem */ > comment before the call ? OK, I can add the comment. Thanks for review. Honza > > --- > > drivers/media/platform/omap3isp/ispqueue.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/platform/omap3isp/ispqueue.c > > b/drivers/media/platform/omap3isp/ispqueue.c index > > e15f01342058..bed380395e6c 100644 > > --- a/drivers/media/platform/omap3isp/ispqueue.c > > +++ b/drivers/media/platform/omap3isp/ispqueue.c > > @@ -331,13 +331,9 @@ static int isp_video_buffer_prepare_user(struct > > isp_video_buffer *buf) if (buf->pages == NULL) > > return -ENOMEM; > > > > - down_read(¤t->mm->mmap_sem); > > - ret = get_user_pages(current, current->mm, data & PAGE_MASK, > > - buf->npages, > > - buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, 0, > > - buf->pages, NULL); > > - up_read(¤t->mm->mmap_sem); > > - > > + ret = get_user_pages_fast(data & PAGE_MASK, buf->npages, > > + buf->vbuf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE, > > + buf->pages); > > if (ret != buf->npages) { > > buf->npages = ret < 0 ? 0 : ret; > > isp_video_buffer_cleanup(buf); > -- > Regards, > > Laurent Pinchart > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() [not found] <1380724087-13927-1-git-send-email-jack@suse.cz> 2013-10-02 14:27 ` [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() Jan Kara @ 2013-10-02 14:28 ` Jan Kara 2013-10-05 12:02 ` Andy Walls 1 sibling, 1 reply; 6+ messages in thread From: Jan Kara @ 2013-10-02 14:28 UTC (permalink / raw) To: LKML; +Cc: linux-mm, Jan Kara, Andy Walls, linux-media CC: Andy Walls <awalls@md.metrocast.net> CC: linux-media@vger.kernel.org Signed-off-by: Jan Kara <jack@suse.cz> --- drivers/media/pci/ivtv/ivtv-udma.c | 6 ++---- drivers/media/pci/ivtv/ivtv-yuv.c | 12 ++++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/drivers/media/pci/ivtv/ivtv-udma.c b/drivers/media/pci/ivtv/ivtv-udma.c index 7338cb2d0a38..6012e5049076 100644 --- a/drivers/media/pci/ivtv/ivtv-udma.c +++ b/drivers/media/pci/ivtv/ivtv-udma.c @@ -124,10 +124,8 @@ int ivtv_udma_setup(struct ivtv *itv, unsigned long ivtv_dest_addr, } /* Get user pages for DMA Xfer */ - down_read(¤t->mm->mmap_sem); - err = get_user_pages(current, current->mm, - user_dma.uaddr, user_dma.page_count, 0, 1, dma->map, NULL); - up_read(¤t->mm->mmap_sem); + err = get_user_pages_unlocked(current, current->mm, user_dma.uaddr, + user_dma.page_count, 0, 1, dma->map); if (user_dma.page_count != err) { IVTV_DEBUG_WARN("failed to map user pages, returned %d instead of %d\n", diff --git a/drivers/media/pci/ivtv/ivtv-yuv.c b/drivers/media/pci/ivtv/ivtv-yuv.c index 2ad65eb29832..9365995917d8 100644 --- a/drivers/media/pci/ivtv/ivtv-yuv.c +++ b/drivers/media/pci/ivtv/ivtv-yuv.c @@ -75,15 +75,15 @@ static int ivtv_yuv_prep_user_dma(struct ivtv *itv, struct ivtv_user_dma *dma, ivtv_udma_get_page_info (&uv_dma, (unsigned long)args->uv_source, 360 * uv_decode_height); /* Get user pages for DMA Xfer */ - down_read(¤t->mm->mmap_sem); - y_pages = get_user_pages(current, current->mm, y_dma.uaddr, y_dma.page_count, 0, 1, &dma->map[0], NULL); + y_pages = get_user_pages_unlocked(current, current->mm, y_dma.uaddr, + y_dma.page_count, 0, 1, &dma->map[0]); uv_pages = 0; /* silence gcc. value is set and consumed only if: */ if (y_pages == y_dma.page_count) { - uv_pages = get_user_pages(current, current->mm, - uv_dma.uaddr, uv_dma.page_count, 0, 1, - &dma->map[y_pages], NULL); + uv_pages = get_user_pages_unlocked(current, current->mm, + uv_dma.uaddr, + uv_dma.page_count, 0, 1, + &dma->map[y_pages]); } - up_read(¤t->mm->mmap_sem); if (y_pages != y_dma.page_count || uv_pages != uv_dma.page_count) { int rc = -EFAULT; -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() 2013-10-02 14:28 ` [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() Jan Kara @ 2013-10-05 12:02 ` Andy Walls 2013-10-07 17:22 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Andy Walls @ 2013-10-05 12:02 UTC (permalink / raw) To: Jan Kara; +Cc: LKML, linux-mm, linux-media Hi Jan: <rant> This patch alone does not have suffcient information for me to evaluate it. get_user_pages_unlocked() is added in another patch which I did not receive, and which I cannot find in any list archives. I wasted quite a bit of time looking for this additional patch: https://git.kernel.org/cgit/linux/kernel/git/jack/linux-fs.git/commit/?h=get_user_pages&id=624fc1bfb70fb65d32d31fbd16427ad9c234653e </rant> If I found the correct patch for adding get_user_pages_unlocked(), then the patch below looks fine. Reviewed-by: Andy Walls <awalls@md.metrocast.net> Acked-by: Andy Walls <awalls@md.metrocast.net> Regards, Andy On Wed, 2013-10-02 at 16:28 +0200, Jan Kara wrote: > CC: Andy Walls <awalls@md.metrocast.net> > CC: linux-media@vger.kernel.org > Signed-off-by: Jan Kara <jack@suse.cz> > --- > drivers/media/pci/ivtv/ivtv-udma.c | 6 ++---- > drivers/media/pci/ivtv/ivtv-yuv.c | 12 ++++++------ > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/pci/ivtv/ivtv-udma.c b/drivers/media/pci/ivtv/ivtv-udma.c > index 7338cb2d0a38..6012e5049076 100644 > --- a/drivers/media/pci/ivtv/ivtv-udma.c > +++ b/drivers/media/pci/ivtv/ivtv-udma.c > @@ -124,10 +124,8 @@ int ivtv_udma_setup(struct ivtv *itv, unsigned long ivtv_dest_addr, > } > > /* Get user pages for DMA Xfer */ > - down_read(¤t->mm->mmap_sem); > - err = get_user_pages(current, current->mm, > - user_dma.uaddr, user_dma.page_count, 0, 1, dma->map, NULL); > - up_read(¤t->mm->mmap_sem); > + err = get_user_pages_unlocked(current, current->mm, user_dma.uaddr, > + user_dma.page_count, 0, 1, dma->map); > > if (user_dma.page_count != err) { > IVTV_DEBUG_WARN("failed to map user pages, returned %d instead of %d\n", > diff --git a/drivers/media/pci/ivtv/ivtv-yuv.c b/drivers/media/pci/ivtv/ivtv-yuv.c > index 2ad65eb29832..9365995917d8 100644 > --- a/drivers/media/pci/ivtv/ivtv-yuv.c > +++ b/drivers/media/pci/ivtv/ivtv-yuv.c > @@ -75,15 +75,15 @@ static int ivtv_yuv_prep_user_dma(struct ivtv *itv, struct ivtv_user_dma *dma, > ivtv_udma_get_page_info (&uv_dma, (unsigned long)args->uv_source, 360 * uv_decode_height); > > /* Get user pages for DMA Xfer */ > - down_read(¤t->mm->mmap_sem); > - y_pages = get_user_pages(current, current->mm, y_dma.uaddr, y_dma.page_count, 0, 1, &dma->map[0], NULL); > + y_pages = get_user_pages_unlocked(current, current->mm, y_dma.uaddr, > + y_dma.page_count, 0, 1, &dma->map[0]); > uv_pages = 0; /* silence gcc. value is set and consumed only if: */ > if (y_pages == y_dma.page_count) { > - uv_pages = get_user_pages(current, current->mm, > - uv_dma.uaddr, uv_dma.page_count, 0, 1, > - &dma->map[y_pages], NULL); > + uv_pages = get_user_pages_unlocked(current, current->mm, > + uv_dma.uaddr, > + uv_dma.page_count, 0, 1, > + &dma->map[y_pages]); > } > - up_read(¤t->mm->mmap_sem); > > if (y_pages != y_dma.page_count || uv_pages != uv_dma.page_count) { > int rc = -EFAULT; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() 2013-10-05 12:02 ` Andy Walls @ 2013-10-07 17:22 ` Jan Kara 0 siblings, 0 replies; 6+ messages in thread From: Jan Kara @ 2013-10-07 17:22 UTC (permalink / raw) To: Andy Walls; +Cc: Jan Kara, LKML, linux-mm, linux-media Hello, On Sat 05-10-13 08:02:21, Andy Walls wrote: > <rant> > This patch alone does not have suffcient information for me to evaluate > it. get_user_pages_unlocked() is added in another patch which I did not > receive, and which I cannot find in any list archives. > > I wasted quite a bit of time looking for this additional patch: > > https://git.kernel.org/cgit/linux/kernel/git/jack/linux-fs.git/commit/?h=get_user_pages&id=624fc1bfb70fb65d32d31fbd16427ad9c234653e > > </rant> Sorry, I should have CCed that patch to driver maintainers as well. > If I found the correct patch for adding get_user_pages_unlocked(), then > the patch below looks fine. > > Reviewed-by: Andy Walls <awalls@md.metrocast.net> > Acked-by: Andy Walls <awalls@md.metrocast.net> Thanks. Honza > On Wed, 2013-10-02 at 16:28 +0200, Jan Kara wrote: > > CC: Andy Walls <awalls@md.metrocast.net> > > CC: linux-media@vger.kernel.org > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > drivers/media/pci/ivtv/ivtv-udma.c | 6 ++---- > > drivers/media/pci/ivtv/ivtv-yuv.c | 12 ++++++------ > > 2 files changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/pci/ivtv/ivtv-udma.c b/drivers/media/pci/ivtv/ivtv-udma.c > > index 7338cb2d0a38..6012e5049076 100644 > > --- a/drivers/media/pci/ivtv/ivtv-udma.c > > +++ b/drivers/media/pci/ivtv/ivtv-udma.c > > @@ -124,10 +124,8 @@ int ivtv_udma_setup(struct ivtv *itv, unsigned long ivtv_dest_addr, > > } > > > > /* Get user pages for DMA Xfer */ > > - down_read(¤t->mm->mmap_sem); > > - err = get_user_pages(current, current->mm, > > - user_dma.uaddr, user_dma.page_count, 0, 1, dma->map, NULL); > > - up_read(¤t->mm->mmap_sem); > > + err = get_user_pages_unlocked(current, current->mm, user_dma.uaddr, > > + user_dma.page_count, 0, 1, dma->map); > > > > if (user_dma.page_count != err) { > > IVTV_DEBUG_WARN("failed to map user pages, returned %d instead of %d\n", > > diff --git a/drivers/media/pci/ivtv/ivtv-yuv.c b/drivers/media/pci/ivtv/ivtv-yuv.c > > index 2ad65eb29832..9365995917d8 100644 > > --- a/drivers/media/pci/ivtv/ivtv-yuv.c > > +++ b/drivers/media/pci/ivtv/ivtv-yuv.c > > @@ -75,15 +75,15 @@ static int ivtv_yuv_prep_user_dma(struct ivtv *itv, struct ivtv_user_dma *dma, > > ivtv_udma_get_page_info (&uv_dma, (unsigned long)args->uv_source, 360 * uv_decode_height); > > > > /* Get user pages for DMA Xfer */ > > - down_read(¤t->mm->mmap_sem); > > - y_pages = get_user_pages(current, current->mm, y_dma.uaddr, y_dma.page_count, 0, 1, &dma->map[0], NULL); > > + y_pages = get_user_pages_unlocked(current, current->mm, y_dma.uaddr, > > + y_dma.page_count, 0, 1, &dma->map[0]); > > uv_pages = 0; /* silence gcc. value is set and consumed only if: */ > > if (y_pages == y_dma.page_count) { > > - uv_pages = get_user_pages(current, current->mm, > > - uv_dma.uaddr, uv_dma.page_count, 0, 1, > > - &dma->map[y_pages], NULL); > > + uv_pages = get_user_pages_unlocked(current, current->mm, > > + uv_dma.uaddr, > > + uv_dma.page_count, 0, 1, > > + &dma->map[y_pages]); > > } > > - up_read(¤t->mm->mmap_sem); > > > > if (y_pages != y_dma.page_count || uv_pages != uv_dma.page_count) { > > int rc = -EFAULT; > > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-07 17:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1380724087-13927-1-git-send-email-jack@suse.cz>
2013-10-02 14:27 ` [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() Jan Kara
2013-10-02 19:41 ` Laurent Pinchart
2013-10-02 20:18 ` Jan Kara
2013-10-02 14:28 ` [PATCH 19/26] ivtv: Convert driver to use get_user_pages_unlocked() Jan Kara
2013-10-05 12:02 ` Andy Walls
2013-10-07 17:22 ` Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox