Re: [PATCH] pipe_read: don't wake up the writer if the pipe is still full

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Linus,

On 3/4/2025 11:58 PM, Linus Torvalds wrote:
> On Tue, 4 Mar 2025 at 02:55, Oleg Nesterov <oleg@redhat.com> 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