From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-media@vger.kernel.org, hverkuil@xs4all.nl, mchehab@osg.samsung.com
Subject: Re: [PATCH v2.1 3/5] media: Refactor copying IOCTL arguments from and to user space
Date: Sat, 09 Jul 2016 22:29:03 +0300 [thread overview]
Message-ID: <3214203.0Eb5nWCm1s@avalon> (raw)
In-Reply-To: <57308DAA.1000404@linux.intel.com>
Hi Sakari,
On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Wednesday 04 May 2016 16:09:51 Sakari Ailus wrote:
> >> Refactor copying the IOCTL argument structs from the user space and back,
> >> in order to reduce code copied around and make the implementation more
> >> robust.
> >>
> >> As a result, the copying is done while not holding the graph mutex.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> since v2:
> >>
> >> - Remove function to calculate maximum argument size, replace by a char
> >> array of 256 or kmalloc() if that's too small.
> >>
> >> drivers/media/media-device.c | 194 ++++++++++++++++---------------------
> >> 1 file changed, 94 insertions(+), 100 deletions(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 9b5a88d..0797e4b 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
[snip]
> >> @@ -453,10 +432,24 @@ static long __media_device_ioctl(
> >>
> >> info = &info_array[_IOC_NR(cmd)];
> >>
> >> + if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> >> + karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> >> + if (!karg)
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + info->arg_from_user(karg, arg, cmd);
> >> +
> >> mutex_lock(&dev->graph_mutex);
> >> - ret = info->fn(dev, arg);
> >> + ret = info->fn(dev, karg);
> >> mutex_unlock(&dev->graph_mutex);
> >>
> >> + if (!ret)
> >
> > How about if (!ret && info->arg_to_user) instead, and getting rid of
> > copy_arg_to_user_nop() ?
>
> I thought of that, but I decided to optimise the common case --- which
> is that the argument is copied back and forth. Not copying the argument
> back is a very special case, we use it for a single compat IOCTL.
>
> That said, we could use it for the proper ENUM_LINKS as well. Still that
> does not change what's normal.
We're talking about one comparison and one branching instruction (that will
not be taken in the common case). Is that micro-optimization really worth it
in an ioctl path that is not that performance-critical ? If you think it is,
could you analyse what the impact of the copy_arg_to_user_nop() function on
cache locality is for the common case ? ;-)
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2016-07-09 19:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-04 11:20 [PATCH v2 0/5] Refactor media IOCTL handling, add variable length arguments Sakari Ailus
2016-05-04 11:20 ` [PATCH v2 1/5] media: Determine early whether an IOCTL is supported Sakari Ailus
2016-05-09 12:43 ` Laurent Pinchart
2016-05-04 11:20 ` [PATCH v2 2/5] media: Unify IOCTL handler calling Sakari Ailus
2016-05-09 12:43 ` Laurent Pinchart
2016-05-04 11:20 ` [PATCH v2 3/5] media: Refactor copying IOCTL arguments from and to user space Sakari Ailus
2016-05-04 12:24 ` Hans Verkuil
2016-05-04 12:31 ` Sakari Ailus
2016-05-04 13:09 ` [PATCH v2.1 " Sakari Ailus
2016-05-09 12:43 ` Laurent Pinchart
2016-05-09 13:16 ` Sakari Ailus
2016-07-09 19:29 ` Laurent Pinchart [this message]
2016-07-09 22:03 ` Sakari Ailus
2016-07-09 23:12 ` Laurent Pinchart
2016-07-09 23:23 ` Sakari Ailus
2016-07-21 10:56 ` [PATCH v2.3 " Sakari Ailus
2016-05-17 14:49 ` [PATCH v2.2 " Sakari Ailus
2016-05-04 11:20 ` [PATCH v2 4/5] media: Add flags to tell whether to take graph mutex for an IOCTL Sakari Ailus
2016-05-04 14:50 ` Shuah Khan
2016-05-04 16:26 ` Sakari Ailus
2016-05-09 12:44 ` Laurent Pinchart
2016-07-09 19:47 ` Laurent Pinchart
2016-07-09 22:07 ` Sakari Ailus
2016-07-21 11:04 ` [PATCH v2.1 " Sakari Ailus
2016-05-04 11:20 ` [PATCH v2 5/5] media: Support variable size IOCTL arguments Sakari Ailus
2016-05-04 12:37 ` Hans Verkuil
2016-05-04 23:06 ` [PATCH v2.1 " Sakari Ailus
2016-06-17 16:21 ` Hans Verkuil
2016-06-20 17:03 ` Sakari Ailus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3214203.0Eb5nWCm1s@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@osg.samsung.com \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.