On 09/03/2014 06:58 AM, One Thousand Gnomes wrote:
> On Tue, 2 Sep 2014 17:39:30 -0400
> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
>
>> The stopped, hw_stopped, flow_stopped and packet bits are smp-unsafe
>> and interrupt-unsafe. For example,
>>
>> CPU 0 | CPU 1
>> |
>> tty->flow_stopped = 1 | tty->hw_stopped = 0
>>
>> One of these updates will be corrupted, as the bitwise operation
>> on the bitfield is non-atomic.
>>
>> Ensure each flag has a separate memory location, so concurrent
>> updates do not corrupt orthogonal states.
>
> Ouch....
>
> Alas the fix is not sufficient on some platforms. gcc will happily use
> 32bit operations on those fields if it thinks its a performance win.
>
> It needs to use set_bit and friends.
>
> x86 is generally ok, but ia64 gcc will do things like load all four
> bytes, mask and write the dword back. I believe ARM gcc may also
> sometimes generate similar results.
Ahh. Thanks for the insight, Alan.
But set_bit() et. al. will generate an incredible amount of churn;
what if I split the fields up to prevent false-sharing?
--- >% ---
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..36d3cf7 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -252,6 +252,11 @@ struct tty_struct {
struct rw_semaphore termios_rwsem;
struct mutex winsize_mutex;
spinlock_t ctrl_lock;
+ unsigned char ctrl_status; /* ctrl_lock fields */
+ bool packet;
spinlock_t flow_lock;
+ unsigned char stopped:1, /* flow_lock fields */
+ flow_stopped:1;
/* Termios values are protected by the termios rwsem */
struct ktermios termios, termios_locked;
struct termiox *termiox; /* May be NULL for unsupported */
@@ -261,8 +266,7 @@ struct tty_struct {
unsigned long flags;
int count;
struct winsize winsize; /* winsize_mutex */
- unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
- unsigned char ctrl_status; /* ctrl_lock */
+ bool hw_stopped;
unsigned int receive_room; /* Bytes free for queue */
int flow_change;
>>
>> Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>> ---
>> include/linux/tty.h | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 1c3316a..7cf61cb 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
>> @@ -261,7 +261,10 @@ struct tty_struct {
>> unsigned long flags;
>> int count;
>> struct winsize winsize; /* winsize_mutex */
>> - unsigned char stopped:1, hw_stopped:1, flow_stopped:1, packet:1;
>> + bool stopped;
>> + bool hw_stopped;
>> + bool flow_stopped;
>> + bool packet;
>> unsigned char ctrl_status; /* ctrl_lock */
>> unsigned int receive_room; /* Bytes free for queue */
>> int flow_change;
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
