On Tuesday 10 March 2009 00:50:41 Mauro Carvalho Chehab wrote: > On Mon, 9 Mar 2009 08:16:53 +0100 > > Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > > On Monday 09 March 2009 02:07:33 Trent Piepho wrote: > > > On Sun, 8 Mar 2009, Hans Verkuil wrote: > > > > The last one fixes an ivtv regression caused by this change: > > > > > > > > changeset: 10811:0a0eba8e64d5 > > > > user: Trent Piepho <xyzzy@xxxxxxxxxxxxx> > > > > date: Tue Mar 03 20:21:02 2009 -0800 > > > > summary: videodev: only copy needed part of RW ioctl's > > > > parameter > > > > > > > > The fix is simple: switch on the full ioctl command instead of just > > > > the NR field. > > > > > > > > Thanks to Martin Dauskardt for doing the bisect and tracing the > > > > breakage to this change. > > > > > > Switching on the whole ioctl makes the switch statement a lot less > > > efficient. I'd rather just put a if (_IOC_TYPE(cmd) != 'V') return > > > 0; in there. That should fix the non-v4l2 ioctls, right? > > > > That was my first thought as well, but I realized that this is one area > > where you really do not want to take any risk. > > IMO, it is better to use Trent's way, and reduce the number of places > that do memset(0). > > > Personally I think this code is overoptimization. In my view the > > performance advantage is minimal while reducing the readability of the > > code. > > In general, it is a good idea to avoid copying a data that won't be used > from userspace. I liked the optimizations done. Let's just fix what's > broken and test a lot to avoid causing a kernel regression. I strongly suspect that the extra function call and tests involved takes longer than the initial copy_from_user and memset. Perhaps with the exception of G_FMT, which is really the only one that has a non-trivial amount of data to copy. None of the commands involved are high-performance commands (well, we haven't any of those anyway), nor is a tight inner-loop involved. I'll see if I can run some benchmarks, but I remain convinced that this change has no benefit. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
