Hi, On Mon, Jun 27, 2022 at 8:56 PM Alexander Aring <aahringo@xxxxxxxxxx> wrote: > > Hi Luc and others, > > On Mon, Jun 27, 2022 at 2:42 PM Luc Van Oostenryck > <luc.vanoostenryck@xxxxxxxxx> wrote: > > > > On Mon, Jun 27, 2022 at 11:15:17AM -0400, Alexander Aring wrote: > > > Hi, > > > > > > I recently converted to use kref_put_lock() in fs/dlm subsystem and > > > now I get the following warning in sparse: > > > > > > warning: context imbalance in 'put_rsb' - unexpected unlock > > > > > > It seems sparse is not able to detect that there is a conditional > > > requirement that the lock passed to kref_put_lock() (or also > > > refcount_dec_and_lock()) is locked or not. I evaluate the return value > > > to check if kref_put_lock() helds the lock and unlock it then. The > > > idea is that the lock needs only to be held when the refcount is going > > > to be zero. > > > > > > It seems other users unlock the lock at the release callback of > > > kref_put_lock() and annotate the callback with "__releases()" which > > > seems to work to avoid the sparse warning. However this works if you > > > don't have additional stack pointers which you need to pass to the > > > release callback. > > > > > > My question would be is this a known problem and a recommended way to > > > avoid this sparse warning (maybe just for now)? > > > > Hi, > > > > I suppose that your case here can be simplified into something like: > > > > if (some_condition) > > take(some_lock); > > > > do_stuff(); > > > > if (some_condition) > > release(some_lock); > > > > Sparse issues the 'context imbalance' warning because, a priori, > > it can't exclude that some execution will takes the lock and not > > releases it (or the opposite). In some case, when do_stuff() is > > very simple, sparse can see that everything is OK, but generally > > it won't (some whole kernel analysis but the general case is > > undecidable anyway). > > > > The recommended way would be to write things rather like this: > > > > if (some_condition) { > > take(some_lock); > > do_stuff(); > > release(some_lock); > > } else { > > do_stuff(); > > } > > > > This is not an alternative for me because the lock needs to hold > during the "some_condition". (More background is that we dealing with > data structures here and cannot allow a get() from this data > structures during "some_condition", the lock is preventing this) > It is the refcount code which causes trouble here [0] and I think the > refcount code should never call the unlock() procedure in any > condition and leave it to the caller to call unlock() in any case. > > I to'ed (hope to get more attention to this) more people related to > lib/refcount.c implementation (provided by get_maintainers.pl -f). > > > > > The __acquires() and __releases() annotations are needed for sparse > > to know that the annotated function always take or always release > > some lock but won't handle conditional locks. > > > > If we change the refcount code to _never_ calling unlock() for the > specific lock, then all those foo_and_lock_bar() functions can be > annotated with "__acquires()". This should also end in the same code? sorry, this will not work because of the first condition of "if (refcount_dec_not_one(r))" which will never hold the lock if true... that's what the optimization is all about. However, maybe somebody has another idea... - Alex