Hello Linus,
On 3/4/2025 11:58 PM, Linus Torvalds wrote:
On Tue, 4 Mar 2025 at 02:55, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
I thought this is wrong, but then I noticed that in your version
->head_tail is the 1st member in this union.
Yes. That was intentional, to make the code have much less extraneous noise.
The only reason to ever use that "union pipe_index" is for this whole
"one word for everything", so I feel that making it compact is
actually more legible than adding extra markers.
+ * Really only alpha needs 32-bit fields, but
+ * might as well do it for 64-bit architectures
+ * since that's what we've historically done,
+ * and it makes 'head_tail' always be a simple
+ * 'unsigned long'.
+ */
+#ifdef CONFIG_64BIT
+ typedef unsigned int pipe_index_t;
+#else
+ typedef unsigned short pipe_index_t;
+#endif
I am just curious, why we can't use "unsigned short" unconditionally
and avoid #ifdef ?
Is "unsigned int" more efficient on 64-bit?
The main reason is that a "unsigned short" write on alpha isn't atomic
- it's a read-modify-write operation, and so it isn't safe to mix
spin_lock_irq(&pipe->rd_wait.lock);
...
pipe->tail = ++tail;
...
spin_unlock_irq(&pipe->rd_wait.lock);
From my understanding, this is still done with "pipe->mutex" held. Both
anon_pipe_read() and pipe_resize_ring() will lock "pipe->mutex" first
and then take the "pipe->rd_wait.lock" when updating "pipe->tail".
"pipe->head" is always updated with "pipe->mutex" held.
Could that be enough to guaranteed that RMW on a 16-bit data on Alpha
is safe since the updates to the two 16-bit fields are protected by the
"pipe->mutex" or am I missing something?
with
mutex_lock(&pipe->mutex);
...
pipe->head = head + 1;
...
mutex_unlock(&pipe->mutex);
because while they are two different fields using two different
locks, on alpha the above only works if they are in separate words
(because updating one will do a read-and-write-back of the other).
This is a fundamental alpha architecture bug. I was actually quite
ready to just kill off alpha support entirely, because it's a dead
architecture that is unfixably broken. But there's some crazy patches
to make gcc generate horrific atomic code to make this all work on
alpha by Maciej Rozycki, so one day we'll be in the situation that
alpha can be considered "fixed", but we're not there yet.
Do we really care? Maybe not. The race is probably very hard to hit,
so with the two remaining alpha users, we could just say "let's just
make it use 16-bit ops".
But even on x86, 32-bit ops potentially generate just slightly better
code due to lack of some prefix bytes.
And those fields *used* to be 32-bit, so my patch basically kept the
status quo on 64-bit machines (and just turned it into 16-bit fields
on 32-bit architectures).
Anyway, I wouldn't object to just unconditionally making it "two
16-bit indexes make a 32-bit head_tail" if it actually makes the
structure smaller. It might not even matter on 64-bit because of
alignment of fields around it - I didn't check. As mentioned, it was
more of a combination of "alpha" plus "no change to relevant other
architectures".
Linus
--
Thanks and Regards,
Prateek
