New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PMEMcond alignment confusion #358

Closed
GBuella opened this Issue Nov 25, 2016 · 3 comments

Comments

Projects
None yet
5 participants
@GBuella
Copy link
Member

GBuella commented Nov 25, 2016

In src/include/libpmemobj/thread.h three structs are defined, all of them with a 1 char alignment requirement, and 64 char size :

/*                                                                      
 * Locking.                                                             
 */                                                                     
#define _POBJ_CL_ALIGNMENT 64 /* cache line alignment for performance */
                                                                        
typedef struct {                                                        
        char padding[_POBJ_CL_ALIGNMENT];                               
} PMEMmutex;                                                            
                                                                        
typedef struct {                                                        
        char padding[_POBJ_CL_ALIGNMENT];                               
} PMEMrwlock;                                                           
                                                                        
typedef struct {                                                        
        char padding[_POBJ_CL_ALIGNMENT];                               
} PMEMcond;                                                             

The original intention was likely to force 64 char alignment, but it was futile.
The data in these structs are internally cast to types which require a more strict alignment, as was discovered with sanitizer run:

sync.c:112:6: runtime error: load of misaligned address 0x7f2d1abbe584 for type 'volatile uint64_t', which requires 8 byte alignment

This should not be a problem, as long as simple integer accesses are used on x86_64 ( such unaligned accesses work with any instruction on current x86_64 as far as I know ).

@plebioda plebioda added the Type: Bug label Nov 25, 2016

@vitalyvch

This comment has been minimized.

Copy link

vitalyvch commented Nov 25, 2016

Most likely we should play with gcc options.

@krzycz

This comment has been minimized.

Copy link

krzycz commented Dec 2, 2016

  1. Same issue applies to PMEMmutex and PMEMrwlock.

  2. The problem is much more serious than it appears. :-[
    According to "Intel® 64 and IA-32 Architectures Software Developer Manual, Volume3, Section 8.1.1" - unaligned access to memory is not guaranteed to be atomic, except for "unaligned 16-, 32-, and 64-bit accesses to cached memory that fit within a cache line".
    Since the current definition of PMEM locks does not guarantee any alignment, it may actually happen that - depending on the location/offset of the lock within the user-defined structures - some members of the PMEM locks structure are split across cache line boundary, so again - it cannot be guaranteed that the operation on such a member will always be carried out atomically.

The potential solution could be to always use atomic (LOCK-prefixed) instructions when accessing PMEM lock members, as for locked instructions, the atomicity of the operation is guaranteed even if the memory access is unaligned or if it crosses the cache line boundary (at least on x86_64).
However, besides the performance penalty introduced by locked instructions (slowing down the lock/unlock operations by more than 5 times), the key issue is that we could apply it only to access runid member of the PMEM locks. All the operations on the actual pthread locks that are embeded in PMEM locks are done using the pthread API, and the question is if we can assume that it is safe to pass an unaligned mutex pointer to the functions like pthread_mutex_lock(), pthread_mutex_unlock(), etc.
IOW, the question is if libpthread implementation makes any assumptions on the lock alignment, or if it always uses locked instructions.
Based on the glibc source code analysis and some empirical tests, we believe that it is safe, but building the design/fix on that is not a good idea, as the implementation may change in the future, and also because it may not be true on other architectures having some more restrictive requirements on data alignment.

@GBuella

This comment has been minimized.

Copy link
Member Author

GBuella commented Dec 2, 2016

For reference:
pmem/pmdk#1489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment