* Re: Unnecessary BKL contention in video1394 [not found] <1161203487.28713.8.camel@systems03.lan.brontes3d.com> @ 2006-10-18 21:36 ` Stefan Richter 2006-10-19 13:19 ` Daniel Drake 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2006-10-18 21:36 UTC (permalink / raw) To: Daniel Drake; +Cc: linux1394-devel, linux-kernel (added Cc: lkml to pull in some clues) Daniel Drake wrote at linux1394-devel: > Hi, > > I noticed that video1394 calls lock_kernel in it's file_operations. I > thought about converting these into a per-instance mutex or something, > but in the end I couldn't find a reason why this locking is needed. (The > more important I/O system is protected by separate spinlocks) > > The BKL is only contended when operations such as ioctl() or release() > are invoked. My knowledge lacks at this point, but I'm reasonably sure > that some upper layer must ensure that these invokations are serialized, > as opposed to leaving it up to the driver to handle nasty potential > situations e.g. release() being called halfway through a read(). Can > anyone clarify? > > Thanks, > Daniel I think you are right. Same with dv1394. Although we need to double-check whether something needs replacement protection then. > ------------------------------------------------------------------------ > > Index: linux/drivers/ieee1394/video1394.c > =================================================================== > --- linux.orig/drivers/ieee1394/video1394.c > +++ linux/drivers/ieee1394/video1394.c > @@ -1161,9 +1161,7 @@ static int __video1394_ioctl(struct file > static long video1394_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > int err; > - lock_kernel(); > err = __video1394_ioctl(file, cmd, arg); > - unlock_kernel(); > return err; > } > > @@ -1181,13 +1179,11 @@ static int video1394_mmap(struct file *f > struct file_ctx *ctx = (struct file_ctx *)file->private_data; > int res = -EINVAL; > > - lock_kernel(); > if (ctx->current_ctx == NULL) { > PRINT(KERN_ERR, ctx->ohci->host->id, > "Current iso context not set"); > } else > res = dma_region_mmap(&ctx->current_ctx->dma, file, vma); > - unlock_kernel(); > > return res; > } > @@ -1200,7 +1196,6 @@ static unsigned int video1394_poll(struc > struct dma_iso_ctx *d; > int i; > > - lock_kernel(); > ctx = file->private_data; > d = ctx->current_ctx; > if (d == NULL) { > @@ -1221,7 +1216,6 @@ static unsigned int video1394_poll(struc > } > spin_unlock_irqrestore(&d->lock, flags); > done: > - unlock_kernel(); > > return mask; > } > @@ -1257,7 +1251,6 @@ static int video1394_release(struct inod > struct list_head *lh, *next; > u64 mask; > > - lock_kernel(); > list_for_each_safe(lh, next, &ctx->context_list) { > struct dma_iso_ctx *d; > d = list_entry(lh, struct dma_iso_ctx, link); > @@ -1278,7 +1271,6 @@ static int video1394_release(struct inod > kfree(ctx); > file->private_data = NULL; > > - unlock_kernel(); > return 0; > } > > -- Stefan Richter -=====-=-==- =-=- =--=- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Unnecessary BKL contention in video1394 2006-10-18 21:36 ` Unnecessary BKL contention in video1394 Stefan Richter @ 2006-10-19 13:19 ` Daniel Drake 2006-10-19 13:27 ` Andi Kleen 0 siblings, 1 reply; 5+ messages in thread From: Daniel Drake @ 2006-10-19 13:19 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel, ak On Wed, 2006-10-18 at 23:36 +0200, Stefan Richter wrote: > (added Cc: lkml to pull in some clues) > > Daniel Drake wrote at linux1394-devel: > > Hi, > > > > I noticed that video1394 calls lock_kernel in it's file_operations. I > > thought about converting these into a per-instance mutex or something, > > but in the end I couldn't find a reason why this locking is needed. (The > > more important I/O system is protected by separate spinlocks) > > > > The BKL is only contended when operations such as ioctl() or release() > > are invoked. My knowledge lacks at this point, but I'm reasonably sure > > that some upper layer must ensure that these invokations are serialized, > > as opposed to leaving it up to the driver to handle nasty potential > > situations e.g. release() being called halfway through a read(). Can > > anyone clarify? > I think you are right. Same with dv1394. Although we need to > double-check whether something needs replacement protection then. Adding Andi Kleen to CC, who added the BKL around __video1394_ioctl a long while back (when converting video1394 to compat_ioctl). I don't feel that any replacement protection is needed, since the critical sections (where structures are used both in interrupts and in file_operations) are already protected by spinlocks. > > ------------------------------------------------------------------------ > > > > Index: linux/drivers/ieee1394/video1394.c > > =================================================================== > > --- linux.orig/drivers/ieee1394/video1394.c > > +++ linux/drivers/ieee1394/video1394.c > > @@ -1161,9 +1161,7 @@ static int __video1394_ioctl(struct file > > static long video1394_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > { > > int err; > > - lock_kernel(); > > err = __video1394_ioctl(file, cmd, arg); > > - unlock_kernel(); > > return err; > > } > > > > @@ -1181,13 +1179,11 @@ static int video1394_mmap(struct file *f > > struct file_ctx *ctx = (struct file_ctx *)file->private_data; > > int res = -EINVAL; > > > > - lock_kernel(); > > if (ctx->current_ctx == NULL) { > > PRINT(KERN_ERR, ctx->ohci->host->id, > > "Current iso context not set"); > > } else > > res = dma_region_mmap(&ctx->current_ctx->dma, file, vma); > > - unlock_kernel(); > > > > return res; > > } > > @@ -1200,7 +1196,6 @@ static unsigned int video1394_poll(struc > > struct dma_iso_ctx *d; > > int i; > > > > - lock_kernel(); > > ctx = file->private_data; > > d = ctx->current_ctx; > > if (d == NULL) { > > @@ -1221,7 +1216,6 @@ static unsigned int video1394_poll(struc > > } > > spin_unlock_irqrestore(&d->lock, flags); > > done: > > - unlock_kernel(); > > > > return mask; > > } > > @@ -1257,7 +1251,6 @@ static int video1394_release(struct inod > > struct list_head *lh, *next; > > u64 mask; > > > > - lock_kernel(); > > list_for_each_safe(lh, next, &ctx->context_list) { > > struct dma_iso_ctx *d; > > d = list_entry(lh, struct dma_iso_ctx, link); > > @@ -1278,7 +1271,6 @@ static int video1394_release(struct inod > > kfree(ctx); > > file->private_data = NULL; > > > > - unlock_kernel(); > > return 0; > > } > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Unnecessary BKL contention in video1394 2006-10-19 13:19 ` Daniel Drake @ 2006-10-19 13:27 ` Andi Kleen 2006-10-19 14:36 ` Stefan Richter 0 siblings, 1 reply; 5+ messages in thread From: Andi Kleen @ 2006-10-19 13:27 UTC (permalink / raw) To: Daniel Drake; +Cc: Stefan Richter, linux1394-devel, linux-kernel > Adding Andi Kleen to CC, who added the BKL around __video1394_ioctl a > long while back (when converting video1394 to compat_ioctl). > > I don't feel that any replacement protection is needed, since the > critical sections (where structures are used both in interrupts and in > file_operations) are already protected by spinlocks. Fine by me. I just did it to preserve old semantics because I didn't want to audit the 1394 locking. But if you think it's not needed feel free to remove them. -andi ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Unnecessary BKL contention in video1394 2006-10-19 13:27 ` Andi Kleen @ 2006-10-19 14:36 ` Stefan Richter 2006-10-19 15:31 ` Daniel Drake 0 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2006-10-19 14:36 UTC (permalink / raw) To: Andi Kleen; +Cc: Daniel Drake, linux1394-devel, linux-kernel Andi Kleen wrote: [Daniel Drake wrote:] >> Adding Andi Kleen to CC, who added the BKL around __video1394_ioctl a >> long while back (when converting video1394 to compat_ioctl). >> >> I don't feel that any replacement protection is needed, since the >> critical sections (where structures are used both in interrupts and in >> file_operations) are already protected by spinlocks. > > Fine by me. I just did it to preserve old semantics because I didn't want > to audit the 1394 locking. But if you think it's not needed feel free to remove > them. Thanks for the info. Daniel, do you want to resend a signed-off patch? And __video1394_ioctl and its wrapper video1394_ioctl can certainly be merged then. -- Stefan Richter -=====-=-==- =-=- =--== http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Unnecessary BKL contention in video1394 2006-10-19 14:36 ` Stefan Richter @ 2006-10-19 15:31 ` Daniel Drake 0 siblings, 0 replies; 5+ messages in thread From: Daniel Drake @ 2006-10-19 15:31 UTC (permalink / raw) To: Stefan Richter; +Cc: Andi Kleen, linux1394-devel, linux-kernel On Thu, 2006-10-19 at 16:36 +0200, Stefan Richter wrote: > Thanks for the info. Daniel, do you want to resend a signed-off patch? > And __video1394_ioctl and its wrapper video1394_ioctl can certainly be > merged then. Yep, I had already made that change locally. I will run it overnight on several cameras just to be sure, and will send a patch tomorrow. I also did some more investigation and straightened out my knowledge of file_operations: release() can never be called while inside a read() or ioctl(): This would imply that separate threads are in use, and the driver's release() function is not called until *all* threads have closed the fd. In other words I'm now much more confident that this patch is not removing any necessary locking. Daniel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-10-19 15:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1161203487.28713.8.camel@systems03.lan.brontes3d.com>
2006-10-18 21:36 ` Unnecessary BKL contention in video1394 Stefan Richter
2006-10-19 13:19 ` Daniel Drake
2006-10-19 13:27 ` Andi Kleen
2006-10-19 14:36 ` Stefan Richter
2006-10-19 15:31 ` Daniel Drake
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.