Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile

On 04/15/2014 08:55 AM, Johannes Sixt wrote:

Am 4/14/2014 15:54, schrieb Michael Haggerty:

quoted

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.

quoted

Suggested-by: Johannes Sixt <redacted>

Not a big deal, but just in case you re-roll again and you do not forget:

  Johannes Sixt [off-list ref]

is preferred.

ACK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/