On Sat, Nov 7, 2020 at 4:42 PM Luc Van Oostenryck
<luc.vanoostenryck@xxxxxxxxx> wrote:
>
> But isn't a lot of code written to explicitly support this, with an
> argument or some other condition selecting between the locked mode
> or the unlocked mode?
A lot? No. I've actively discouraged it.
It does exist - don't get me wrong - but it shouldn't be the normal pattern.
> This one is fine for Sparse
> static inline int cond_lock1(void)
> {
> if (complex_cond()) {
> take_lock();
> minor();
> return 1;
> }
> return 0;
> }
> static void ok1(void)
> {
> int cond = cond_lock1();
> if (cond) {
> major();
> drop_lock();
> }
> }
That _is_ a somewhat common case, and it's basically the "first helper
tells the caller that it took the lock".
The _really_ basic version of that is the various "trylock()" things,
which has that "__cond_lock()" macro in the kernel for it, so that
sparse can catch that very special case.
That said, it's still not the _common_ case, which is just that the
code is straightforward
take_lock();
do something under the lock
drop_lock();
> The next one corresponds to the situation Song Liu reported
> static inline int cond_lock2(void)
> {
> if (!complex_cond())
> return -1;
>
> take_lock();
> minor();
> return 0;
> }
> static int okish(void)
> {
> int cond = cond_lock2();
> if (cond)
> return 0;
> major();
> drop_lock();
> return 1;
> }
Yeah, this is a more complex version of the same.
Clearly sparse doesn't grok it today, but the fact that your patches
make sparse able to track it is a good improvement.
> The one that really annoys me is this one. It's very simple and, as
> far as I know, quite common in the kernel but kinda hopeless.
> static void ko(int cond)
> {
> if (cond)
> take_lock();
> major();
> if (cond)
> drop_lock();
> }
This really is invalid code. "major()" is done in two different lock contexts.
Sparse _should_ complain.
Linus
