On Tue, Dec 28, 2010 at 8:33 AM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote: > On Mon, Dec 27, 2010 at 3:55 PM, Felipe Contreras > <felipe.contreras@xxxxxxxxx> wrote: >> So, effectively, serializing the proc_begin_dma() and proc_end_dma() >> would not affect anyone negatively for the time being. > > You can never really tell who is using the kernel (or will be using > this kernel version), how and under which workload. No, but it's better to address real issues rather than hypothetical. However, as I sad, everybody is using proc_map() and proc_un_map() which take a lock, and there are no complaints. This patch would make proc_begin_dma() and proc_end_dma() as slow as the map operations, so even these hypothetical users would not get affected negatively. >> For the long-term goal I agree with that approach, however, first, I >> think my patch should be applied, since it's fixing a problem using an >> already existing and actively excersised mechanism. In fact, I think >> this should be pushed to 2.6.37 as: >> >> Â1) prevents a kernel panic >> Â2) the issue is reproducible and clearly identified >> Â3) the patch is small and obvious > > Both patches are (IMHO). But frankly I don't mind your patch will be > applied now as long as it doesn't stay. I can rebase my patch against > it after it is applied, and send separately. Ok, can I get your Ack? I guess Omar would be able to push it to Greg, and perhaps it would make .37. >> This approach looks cleaner, however, we need a flag in >> remove_mapping_information() in order to force the removal, otherwise >> there will be memory leaks. Imagine a process crashes, and >> remove_mapping_information() returns -EBUSY. > > Can't happen; both proc_*_dma() operations decrease the reference > count before exiting, so it's not up to the application. Then why did you add that check for is_map_obj_used(), and then return -EBUSY? If that can happen, then it can happen when the application is crashing; user-space crashes while kernel-space is in the middle of a proc_*_dma() operation. >> Sure, but I see this as a broader effort to have finer locking, part of >> this should be to remove the already existing proc_lock. > > Having bad locking is not an excuse for adding more. No, but not being a permanent solution is not an excuse for not fixing a kernel panic right away. > Anyway, as I said, I don't really mind your patch will be applied as > long as it is a temporary workaround. Can you Ack? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
