| Message ID | 1380724087-13927-6-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Laurent Pinchart |
| Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1VRNVI-0007u0-5g; Wed, 02 Oct 2013 16:34:16 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-5) with esmtp id 1VRNVG-0005zg-7K; Wed, 02 Oct 2013 16:34:16 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753256Ab3JBO27 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 2 Oct 2013 10:28:59 -0400 Received: from cantor2.suse.de ([195.135.220.15]:48981 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752928Ab3JBO25 (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 2 Oct 2013 10:28:57 -0400 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D07C2A52D7; Wed, 2 Oct 2013 16:28:56 +0200 (CEST) Received: by quack.suse.cz (Postfix, from userid 1000) id E117C80EC0; Wed, 2 Oct 2013 16:28:54 +0200 (CEST) From: Jan Kara <jack@suse.cz> To: LKML <linux-kernel@vger.kernel.org> Cc: linux-mm@kvack.org, Jan Kara <jack@suse.cz>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, linux-media@vger.kernel.org Subject: [PATCH 05/26] omap3isp: Make isp_video_buffer_prepare_user() use get_user_pages_fast() Date: Wed, 2 Oct 2013 16:27:46 +0200 Message-Id: <1380724087-13927-6-git-send-email-jack@suse.cz> X-Mailer: git-send-email 1.8.1.4 In-Reply-To: <1380724087-13927-1-git-send-email-jack@suse.cz> References: <1380724087-13927-1-git-send-email-jack@suse.cz> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2013.10.2.142415 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1300_1399 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Jan Kara
Oct. 2, 2013, 2:27 p.m. UTC
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(-)
Comments
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);
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 >
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);