On Tue, 10 Mar 2009, Hans Verkuil wrote: > 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. If you bothered to read my commit messages and look at the diff stat, you'd see that this really wasn't about saving a few cycles. That's just a nice extra bonus. Code that was scattered thoughout the tree to clear struct fields was replaced with *less* code that is all in one place. It's not a more complicated solution, it's a simpler one. What's more, duplicating struct clearing over and over leads to bugs. For instance, there were two ioctls that were broken because important fields passed from userspace were mistakenly cleared before the driver got them. I mentioned that in my commit message. There are also many many places where drivers did not clear fields that they should have, leaving whatever garbage was in there. Now all those problems are fixed and driver authors don't have to worry about clearing fields and which fields they should clear. -- 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
