On 08/18/2017 04:18 PM, Bart Van Assche wrote: > On Fri, 2017-08-18 at 16:01 -0400, Waiman Long wrote: >> [ ... ] >> Instead of using bd_mutex, a new global blktrace mutex is now used >> to protect against concurrent access, creation and destruction of the >> blk_trace structure that is used only in the blktrace.c file. As >> blktrace files will not be frequently accessed, using a global mutex >> should not cause any performance problem. >> [ ... ] >> +/* >> + * The bd_mutex was used previously for protecting blk_trace structure. >> + * That could lead to deadlock with concurrent block device deletion and >> + * sysfs access. So a global blktrace_mutex is now used instead for >> + * protecting the blk_trace structure. >> + * >> + * The references to the opened sysfs or device files should prevent the >> + * underlying block device from being removed. >> + */ >> +static DEFINE_MUTEX(blktrace_mutex); > Hello Waiman, > > Thanks for having addressed my previous comment. Regarding this patch: sorry > but I don't think it's a good idea to use a global mutex for serializing > accesses to tracing data of a single block device. Global mutexes create > unwanted lock dependencies between different block devices. Additionally, on > multiprocessor systems global mutexes can cause cache line ping-pong between > processors and hence can cause a severe slowdown. Please make blktrace_mutex > per block device instead of global. I fully understand the problem of a global lock. The main reason of using a global lock here is that the blktrace APIs are not in a performance critical path. In fact, I think it is used primarily for debugging purpose. Activating it will certainly slow thing down no matter what kind of lock is used. I also don't believe that the blktrace APIs will be used in a high enough frequency that it will cause a performance issue. Please let me know if I am wrong in my assumptions. Cheers, Longman
