[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile
From:       Michael Haggerty <mhagger () alum ! mit ! edu>
Date:       2014-04-16 15:36:14
Message-ID: 534EA36E.1030208 () alum ! mit ! edu
[Download RAW message or body]

On 04/15/2014 08:55 AM, Johannes Sixt wrote:
> Am 4/14/2014 15:54, schrieb Michael Haggerty:
>> The function remove_lock_file_on_signal() is used as a signal handler.
>> It is not realistic to make the signal handler conform strictly to the
>> C standard, which is very restrictive about what a signal handler is
>> allowed to do.  But let's increase the likelihood that it will work:
>>
>> The lock_file_list global variable and several fields from struct
>> lock_file are used by the signal handler.  Declare those values
>> "volatile" to increase the chance that the signal handler will see a
>> valid object state.
> 
> Yes, it's important that the signal handler sees a valid object state, and
> "volatile" can help here. But I think the reason why it helps is not
> obvious, and it should be mentioned in the commit message:
> 
> It is not so much that "volatile" forces the compiler to lay down each
> access of the variable coded in C in the assembly code, but more
> importantly, that "volatile" disallows any re-ordering of these accesses.
> Then:
> 
> - 'lock->active = 1' must be the last assignment during setup
> 
> - 'lock->active = 0' must be the first assignment during tear-down.
> 
> - Ideally, all members of struct lock_file should be "volatile".
> 
> The last point is important because the compiler is allowed to re-order
> accesses to non-"volatile" variables across "volatile" accesses. I say
> "ideally" because if filename were defined "volatile filename[PATH_MAX]",
> strcpy() could not be used anymore. OTOH, it is unlikely that a compiler
> re-orders a strcpy() call across other expressions, and we can get away
> without "volatile" in the "filename" case in practice.

Thanks for the clarification.  I will edit the commit message to better
explain the rationale.

>> Suggested-by: Johannes Sixt <j.sixt@viscovery.net>
> 
> Not a big deal, but just in case you re-roll again and you do not forget:
> 
>   Johannes Sixt <j6t@kdbg.org>
> 
> is preferred.

ACK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]