Skip to content
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.

Sign up for GitHub

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

handler() variable redundancy/naming cleanup #35

Merged
merged 1 commit into from
Feb 27, 2020
Merged

handler() variable redundancy/naming cleanup #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 29 additions & 29 deletions src/backend/mpi/swdsm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* @brief This file implements the MPI-backend of ArgoDSM
* @copyright Eta Scale AB. Licensed under the Eta Scale Open Source License. See the LICENSE file for details.
*/
#include<cstddef>

#include "signal/signal.hpp"
#include "virtual_memory/virtual_memory.hpp"
#include "swdsm.h"
Expand Down Expand Up @@ -295,14 +297,14 @@ void init_mpi_cacheblock(void){
MPI_Type_commit(&cacheblock);
}

unsigned long alignAddr(unsigned long addr){
unsigned long mod = addr % pagesize;
if(addr % pagesize != 0){
addr = addr - mod;
}
addr /= pagesize;
addr *= pagesize;
return addr;
/**
* @brief align an offset into a memory region to the beginning of its size block
* @param offset the unaligned offset
* @param size the size of each block
* @return the beginning of the block of size size where offset is located
*/
inline std::size_t align_backwards(std::size_t offset, std::size_t size) {
SakalisC marked this conversation as resolved.
Show resolved Hide resolved
return (offset / size) * size;
}

void handler(int sig, siginfo_t *si, void *unused){
Expand All @@ -312,19 +314,18 @@ void handler(int sig, siginfo_t *si, void *unused){

unsigned long tag;
argo_byte owner,state;
unsigned long distrAddr = (unsigned long)((unsigned long)(si->si_addr) - (unsigned long)(startAddr));

unsigned long alignedDistrAddr = alignAddr(distrAddr);
unsigned long remCACHELINE = alignedDistrAddr % (CACHELINE*pagesize);
unsigned long lineAddr = alignedDistrAddr - remCACHELINE;
unsigned long classidx = get_classification_index(lineAddr);

unsigned long * localAlignedAddr = (unsigned long *)((char*)startAddr + lineAddr);
unsigned long startIndex = getCacheIndex(lineAddr);
alignedDistrAddr = lineAddr;

unsigned long homenode = getHomenode(lineAddr);
unsigned long offset = getOffset(lineAddr);
/* compute offset in distributed memory in bytes, always positive */
const std::size_t access_offset = static_cast<char*>(si->si_addr) - static_cast<char*>(startAddr);

/* align access offset to cacheline */
const std::size_t aligned_access_offset = align_backwards(access_offset, CACHELINE*pagesize);
unsigned long classidx = get_classification_index(aligned_access_offset);

/* compute start pointer of cacheline. char* has byte-wise arithmetics */
char* const aligned_access_ptr = static_cast<char*>(startAddr) + aligned_access_offset;
unsigned long startIndex = getCacheIndex(aligned_access_offset);
unsigned long homenode = getHomenode(aligned_access_offset);
unsigned long offset = getOffset(aligned_access_offset);
unsigned long id = 1 << getID();
unsigned long invid = ~id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we should ensure that all variables have better descriptive and more consistent names, in addition to using snake_case in accordance with the code standard. Not important for this specific PR, but the simple fact that id equals 1 << getID() rather than simply the return value of getID() is not straightforward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, eventually I would like to get rid of everything in this file and replace it with more modular, easier to read, better documented, and more consistent code. Should we create another ticket for this particular complaint, in the hope someone addresses it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is out of the scope of this PR, as changing either of the variable names above would lead to changes on multiple locations in the backend. Sounds like a good idea for another ticket to be dealt with in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now issue #36, please feel free to contribute your find there.


Expand Down Expand Up @@ -364,7 +365,7 @@ void handler(int sig, siginfo_t *si, void *unused){
}
/* set page to permit reads and map it to the page cache */
/** @todo Set cache offset to a variable instead of calculating it here */
vm::map_memory(localAlignedAddr, pagesize*CACHELINE, cacheoffset+offset, PROT_READ);
vm::map_memory(aligned_access_ptr, pagesize*CACHELINE, cacheoffset+offset, PROT_READ);

}
else{
Expand Down Expand Up @@ -400,7 +401,7 @@ void handler(int sig, siginfo_t *si, void *unused){
}
}
/* set page to permit read/write and map it to the page cache */
vm::map_memory(localAlignedAddr, pagesize*CACHELINE, cacheoffset+offset, PROT_READ|PROT_WRITE);
vm::map_memory(aligned_access_ptr, pagesize*CACHELINE, cacheoffset+offset, PROT_READ|PROT_WRITE);

}
sem_post(&ibsem);
Expand All @@ -411,10 +412,10 @@ void handler(int sig, siginfo_t *si, void *unused){
state = cacheControl[startIndex].state;
tag = cacheControl[startIndex].tag;

if(state == INVALID || (tag != lineAddr && tag != GLOBAL_NULL)){
load_cache_entry(alignedDistrAddr, (startIndex%cachesize));
if(state == INVALID || (tag != aligned_access_offset && tag != GLOBAL_NULL)) {
load_cache_entry(aligned_access_offset, (startIndex%cachesize));
#ifdef DUAL_LOAD
prefetch_cache_entry((alignedDistrAddr+CACHELINE*pagesize), ((startIndex+CACHELINE)%cachesize));
prefetch_cache_entry((aligned_access_offset+CACHELINE*pagesize), ((startIndex+CACHELINE)%cachesize));
SakalisC marked this conversation as resolved.
Show resolved Hide resolved
#endif
pthread_mutex_unlock(&cachemutex);
double t2 = MPI_Wtime();
Expand Down Expand Up @@ -483,11 +484,10 @@ void handler(int sig, siginfo_t *si, void *unused){
}
}
sem_post(&ibsem);
unsigned char * real = (unsigned char *)(localAlignedAddr);
unsigned char * copy = (unsigned char *)(pagecopy + line*pagesize);
memcpy(copy,real,CACHELINE*pagesize);
memcpy(copy,aligned_access_ptr,CACHELINE*pagesize);
addToWriteBuffer(startIndex);
mprotect(localAlignedAddr, pagesize*CACHELINE,PROT_WRITE|PROT_READ);
mprotect(aligned_access_ptr, pagesize*CACHELINE,PROT_WRITE|PROT_READ);
pthread_mutex_unlock(&cachemutex);
double t2 = MPI_Wtime();
stats.storetime += t2-t1;
Expand Down
6 changes: 0 additions & 6 deletions src/backend/mpi/swdsm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,12 +314,6 @@ unsigned long isPowerOf2(unsigned long x);
* @return cacheindex where addr should map to in the ArgoDSM page cache
*/
unsigned long getCacheIndex(unsigned long addr);
/**
* @brief Returns an address correspongint to the page addr is addressing
* @param addr Address in the global address space
* @return addr rounded down to nearest multiple of pagesize
*/
unsigned long alignAddr(unsigned long addr);
/**
* @brief Gives homenode for a given address
* @param addr Address in the global address space
Expand Down