Messages in this thread | | | From | Linus Torvalds <> | Date | Thu, 12 Oct 2023 12:24:37 -0700 | Subject | Re: [PATCH v2 2/2] seqlock: change __seqprop() to return the function pointer |
| |
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
| |