Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAK-6q+hd-L54cqOiFNuufS2_VF5XS0R8cjQL7es8921+2u3uwQ@mail.gmail.com>
 <20220627184232.tjfuzeir57l3h5ll@mail> <CAK-6q+haOfQD8_N6pEm80BTrUXwaj07ZBcXP-EBHftpTVEc1XQ@mail.gmail.com>
 <CAK-6q+g795MwPvUGxWXTE+s-TfJnbh2wf=Udb1k7r6ru-4FTMQ@mail.gmail.com>
 <20220628085821.kn3jjrviyprgai4w@mail> <CAHk-=wie9HSY-MmFr9zHeizRTgSfdONop0-4ezBiq5hYNp4U6g@mail.gmail.com>
In-Reply-To: <CAHk-=wie9HSY-MmFr9zHeizRTgSfdONop0-4ezBiq5hYNp4U6g@mail.gmail.com>
From:   Alexander Aring <aahringo@redhat.com>
Date:   Wed, 29 Jun 2022 10:42:48 -0400
Message-ID: <CAK-6q+jngywC7UZ6XG=yww16nJQgz0Z=VYKH8Fhg0kbZCkaypA@mail.gmail.com>
Subject: Re: sparse warnings related to kref_put_lock() and refcount_dec_and_lock()
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
        jacob.e.keller@intel.com,
        Andrew Morton <akpm@linux-foundation.org>,
        thunder.leizhen@huawei.com,
        Sparse Mailing-list <linux-sparse@vger.kernel.org>,
        cluster-devel <cluster-devel@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Tue, Jun 28, 2022 at 1:27 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jun 28, 2022 at 1:58 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > I would certainly not recommend this but ...
> > if it's OK to cheat and lie then you can do:
> > +       bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock) __acquires(lock);
>
> Actually, we have "__cond_lock()" in the kernel to actually document
> that something takes a lock only in certain conditions.
>
> It needs to be declared as a macro in the header file, with this as an example:
>
>    #define raw_spin_trylock(lock)  __cond_lock(lock, _raw_spin_trylock(lock))
>

I added a prefix of "raw_" to refcount_dec_and_lock() and similar
functions and replaced the original functions with the __cond_lock()
macro to redirect to their raw_ functions. Similar to how the
raw_spinlock_trylock() naming scheme is doing it. The "raw_"
functionality should never be called by the user then.

> ie that says that "raw_spin_trylock() takes 'lock' when
> _raw_spin_trylock() returned true".
>
> That then makes it possible to write code like
>
>     if (raw_spin_trylock(lock)) {..
>                  raw_spin_unlock(lock));
>     }
>
> and sparse will get the nesting right.
>
> But that does *not* solve the issue of people then writing this as
>
>     locked = raw_spin_trylock(lock);
>     .. do_something ..
>     if (locked)
>                  raw_spin_unlock(lock));
>
> and you end up with the same thing again.
>

Yes it mostly removed all sparse warnings I see. I needed to move at
one call of the refcount_dec_and_lock() function inside the if
condition and the sparse warning was gone. It should not be a problem
to change it in this way.

If there are no other complaints I will send a patch for the raw_
prefix to all those conditional refcount lock functions and the add a
__cond_lock() macro for the original function calls.

Thanks!

- Alex