On Thu, Mar 26, 2020 at 9:48 PM Davidlohr Bueso <dave@xxxxxxxxxxxx> wrote: > > On Thu, 26 Mar 2020, Michel Lespinasse wrote: > > >Add a couple APIs to allow splitting mmap_read_unlock() into two calls: > >- mmap_read_release(), called by the task that had taken the mmap lock; > >- mmap_read_unlock_non_owner(), called from a work queue. > > > >These apis are used by kernel/bpf/stackmap.c only. > > I'm not crazy about the idea generalizing such calls into an mm api. > We try to stay away from non-owner semantics in locking - granted > the IS_ENABLED(CONFIG_PREEMPT_RT) warning, but still. > > Could this give future users the wrong impression? What about just > using rwsem calls directly in bpf? I see what you mean and I certainly don't want to encourage any new non-owner call sites to appear.... This bpf stackmap site is a small pain point in my larger range locking patchset too. I am not sure what is the proper response to it; the opposite side of your argument could be that using a direct rwsem call there hides the issue and makes it less likely for someone to fix it ? I don't have a very strong opinion on this, as I think it can be argued either way... But at a minimum, I think it'd be worth adding a comment asking people not to add new call sites to the mmap_read_release() and mmap_read_unlock_non_owner() APIs ? -- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies.
