Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <20231012143158.GA16133@redhat.com> <20231012143227.GA16143@redhat.com>
 <ZSg5KAFxVzKoFlhZ@gmail.com>
In-Reply-To: <ZSg5KAFxVzKoFlhZ@gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 12 Oct 2023 12:24:37 -0700
Message-ID: <CAHk-=wgsybshMs3KLsyheP8hHhndrRhjo70L1qi+GdBZND8M+A@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] seqlock: change __seqprop() to return the function pointer
To:     Ingo Molnar <mingo@kernel.org>
Cc:     Oleg Nesterov <oleg@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Alexey Gladkov <legion@kernel.org>,
        Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Waiman Long <longman@redhat.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 12 Oct 2023 at 11:21, Ingo Molnar <mingo@kernel.org> wrote:
>
> Okay, so dropping 'const' makes sense in terms of staying bug-compatible
> with the  previous API and not build-breaking the world - but could we
> perhaps follow this up with fixups of the type misuse and then a removal
> of the forced type casts from these APIs?

No. The use of 'const' here is *not* a bug.

The thing is, 'const' doesn't mean what you seem to think it means. A
'const' pointer in C in no way means that the target is constant - it
means that *THIS* use of the pointer will not write to the target!

Those may sound similar, but they are very very very different.

In particular, for the sequence

        seq = raw_seqcount_begin(seq_ptr);
        ...
        if (read_seqcount_retry(seq_ptr, seq))
                goto retry;

then 'seq_ptr' really *is* a 'const' pointer. The reader very much
does not write to it, and this very much is part of the fundamental
design.

The above is *literally* what sequence locking is all about: readers
are pure readers.

So no, making it a 'const seqptr_t' is absolutely not a bug. It's very
much a FUNDAMENTAL FEATURE of sequence locks.

Now, I'm not sure how much we actually take advantage of this and
there may not be very many cases of this all, but I really think this
is fundamental to the whole data structure, and there are most
definitely cases where we probably *should* take more advantage of the
fact that a read_seqcount is a read-only op.

For example, I think a function like 'dget_parent()' should actually
take a 'const struct dentry *dentry' as its argument, and the seqcount
is embedded inside that dentry and as such would also be const.

Right now the dentry code doesn't actually do that, because this isn't
one of the areas we have constified, but it's conceptually the right
thing to do. We use the dentry argument in a read-only manner
(although the *parent* that we look up then is written to, and in the
case of a root dentry, the parent may end up being the same as the
original).

Note that the 'const' should only be an issue for the begin/retry
cases, and obviously not for the write ones, but those readers do use
the seqprop_ptr() helper. So those absolutely need to handle the const
case.

So no, the cast wasn't "masking" anything at all. The 'const' is real.

            Linus