Thanks for your comments. I'll include them and try to send out another patch.. As regards your comment regarding "works only with shmget" - you're right..I sent out the patch to mainly get your comments regarding the memory mgmt that can be done. I'll certainly look into the possibilities of using mmap, as soon as the proposed logic is accepted.
-Madhu -----Original Message----- From: Justin Erenkrantz [mailto:[EMAIL PROTECTED] Sent: Friday, September 14, 2001 10:00 AM To: MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1) Cc: [email protected]; '[email protected]' Subject: Re: [PATCH] shmem.c - again Comments inline. > Index: shmem.c > =================================================================== > RCS file: /home/cvspublic/apr/shmem/unix/shmem.c,v > retrieving revision 1.33 > diff -u -r1.33 shmem.c > --- shmem.c 2001/08/30 17:11:04 1.33 > +++ shmem.c 2001/09/12 21:07:29 > @@ -89,182 +89,593 @@ > #include <kernel/OS.h> > #endif > > +#define MIN_BLK_SIZE 200 Where did this number come from? It should at least be a power of two. > + > +/************************************************************************** * > +The Shared Memory - Management Architecture : > + > +List Memory (Shared Memory - 1) > + ____________________________________________ > + | Memory Offsets for the List and | > + | for the Shared Memory | > + |------------------------------------------| > +L1 | MemChunk(MC1) |Next| MemChunk(MC2) |Next | > + |------------------------------------------| > + |------------------------------------------| > +L2 | MemChunk(MC3) |Next| MemChunk(MC4) |Next | > + |------------------------------------------| > + ______| . . . . . . . > + | . . . . . . . . > + | . . . . . . . . > + | |------------------------------------------| > +Ln | | MemChunk(MCm) |Next| ...... | > + | |------------------------------------------| > + | | > + | |________________________________________ > + V | (offset from shm_base) > +Chunk Memory (Shared Memory - 2) V > +------------------------------------------------------------------------- > +| | | | | | | > +| C1 | C2 | C3 | C4 | C5 |.... | > +| | | | | | | > +------------------------------------------------------------------------- > + > +*************************************************************************** / > + > + > +typedef struct memoffsets_t { > + apr_off_t l_used; /* Starting of the Used list elements */ > + apr_off_t l_free; /* Starting of the Freed list elements */ > + apr_off_t l_usedlast; /* Last element of the Used list */ > + apr_off_t l_freelast; /* Last element of the Freed list */ > + apr_off_t c_used; /* Begining of the used chunk list */ > + apr_off_t c_free; /* Begining of the freed chunk list */ > + apr_off_t shm_offset; /* The current offset of the shared memory */ > + apr_off_t shm_length; /* Total length of shared memory available */ > +} memoffsets_t; > + > +typedef struct memchunk_t { > + apr_off_t offset; /* Offset of the chunk - from m->shm_base */ > + apr_size_t size; /* Size of the chunk */ > + apr_off_t next; /* Next chunk - from m->list_base) */ > + apr_off_t prev; /* Previous chunk - from m->list_base) */ > +} memchunk_t; > + > +typedef struct memlist_t { > + struct memchunk_t chunk; > + apr_off_t next; > +} memlist_t; > + > struct shmem_t { > - void *mem; > - void *curmem; > - apr_size_t length; > - apr_lock_t *lock; > - char *filename; > + apr_pool_t *p; > + void *shm_base; /* Starting address of the shared memory */ > + memoffsets_t *offsets; /* Begining of the set of offsets */ > + memlist_t *list_base; /* Begining of the list elements */ > + > + apr_lock_t *lock; > + char *filename; > #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM || APR_USE_SHMEM_MMAP_ZERO > - apr_file_t *file; > -#elif APR_USE_SHMEM_MMAP_ANON > - /* Nothing else. */ > + apr_file_t *file; > #elif APR_USE_SHMEM_SHMGET > - apr_os_file_t file; > + apr_os_file_t file; > + apr_os_file_t listfd; > #elif APR_USE_SHMEM_BEOS > - area_id areaid; > + area_id areaid; > #endif As a matter of style, I don't like the extra spaces in between the type and the variable name. One space please. The formatting changes make it hard to see what it is you have changed. Please lose them. I think you have added some extra variables that are no longer needed. (I only have email access while I am stranded, so I can't apply your patch.) > +#define SHM_ADDR(offset) ((offset < 0) ? (void *)NULL : \ > + (void *)((unsigned char *)m->shm_base + offset)) Uh, where is m coming from? You need to pass that in as well. > + > +#define CHUNK_OFFSET(ptr) ((ptr == (memchunk_t *)NULL) ? 0 : \ > + ((unsigned char *)ptr - (unsigned char *)m->list_base)) NULL shouldn't need to be casted to memchunk_t here. And the fact that you are doing casts at all worries me greatly. I think that means that your chosen types are incorrect. If you are planning on casting the result of the macro to another type, I think that cast should be explicit on the part of the code that calls the macro. > +static memchunk_t *list_malloc (apr_shmem_t *m) No spaces between malloc and (. > +{ > + apr_off_t index; > + memlist_t *elem, *last; > + > + /* Find the first free list element */ > + if ((elem = LIST_ADDR(m->offsets->l_free)) == (memlist_t *)NULL) > + return (memchunk_t *)NULL; > + else { > + index = m->offsets->l_free; > + m->offsets->l_free = elem->next; > + } > + > + elem->next = -1; > + > + if (m->offsets->l_used == -1) > + m->offsets->l_used = index; > + else { > + last = LIST_ADDR(m->offsets->l_usedlast); > + last->next = index; > + } > + > + m->offsets->l_usedlast = index; > + > + return (&(elem->chunk)); > +} > + > +static apr_status_t list_free (apr_shmem_t *m, memchunk_t *chunk) > +{ > + apr_off_t index; > + memlist_t *elem, *iter; > + > + /* Find the required element in the allocated list */ > + iter = LIST_ADDR(m->offsets->l_used); > + while (iter && (&(iter->chunk) != chunk)) { > + elem = iter; > + index = iter->next; > + iter = LIST_ADDR(iter->next); > + } > + > + if ((!iter) || (&(iter->chunk) != chunk)) > + return -1; > + else > + elem->next = iter->next; > + > + iter->next = -1; > + > + if (m->offsets->l_free == -1) > + m->offsets->l_free = index; > + else { > + elem = LIST_ADDR(m->offsets->l_freelast); > + elem->next = index; > + } > + > + m->offsets->l_freelast = index; > + > + return APR_SUCCESS; > +} > + > +static void add_chunk (apr_shmem_t *m, apr_off_t *list, memchunk_t *blk) > +{ > + memchunk_t *elem; > + if (*list == -1) > + *list = CHUNK_OFFSET(blk); > + > + elem = CHUNK_ADDR(*list); > + > + if (blk == elem){ > + blk->prev = CHUNK_OFFSET(blk); > + blk->next = CHUNK_OFFSET(blk); > + } else { > + (CHUNK_ADDR(elem->prev))->next = CHUNK_OFFSET(blk); > + blk->prev = elem->prev; > + blk->next = CHUNK_OFFSET(elem); > + elem->prev = CHUNK_OFFSET(blk); > + } > +} > + > +static void remove_chunk(apr_shmem_t *m, apr_off_t *list, memchunk_t *blk) > +{ > + memchunk_t *elem; > + > + elem = CHUNK_ADDR(*list); > + > + if ((elem == blk) > + && (CHUNK_ADDR(blk->next) == blk) && (blk == CHUNK_ADDR(blk->prev))) { > + *list = -1; > + } else { > + (CHUNK_ADDR(blk->next))->prev = blk->prev; > + (CHUNK_ADDR(blk->prev))->next = blk->next; > + if (elem == blk) > + *list = blk->next; > + } > + blk->next = -1; > + blk->prev = -1; > +} > + > +static void split_chunk( > + apr_shmem_t *m, apr_off_t *list, memchunk_t *blk, apr_size_t size) The correct style would be: static void split_chunk(apr_shmem_t *m, apr_off_t *list, memchunk_t *blk, apr_size_t size) > +{ > + memchunk_t *b; > + > + b = list_malloc (m); > + if (b != (memchunk_t *)NULL) { Why the casts to memchunk_t* for NULL? Just do: if (b) > + b->size = blk->size - size; > + b->offset = blk->offset + size; > + blk->size = size; > + add_chunk(m, list, b); > + } > +} > + > +static memchunk_t *find_chunk_by_addr( > + apr_shmem_t *m, apr_off_t *list, void *addr) > +{ > + memchunk_t *iter = CHUNK_ADDR(*list); > + > + if (!iter) return NULL; > + > + do { > + if (SHM_ADDR(iter->offset) == addr) > + return iter; > + } while (iter && ((iter = CHUNK_ADDR(iter->next)) != CHUNK_ADDR(*list))); > + > + return NULL; > +} > + > +static memchunk_t *find_chunk_by_size( > + apr_shmem_t *m, apr_off_t *list, apr_size_t size) > +{ > + apr_size_t diff = -1; > + memchunk_t *iter = CHUNK_ADDR(*list), *found = NULL; > + > + if (!iter) return NULL; > + > + do { > + if (iter->size == size) > + return iter; > + if (iter->size > size) { > + if (diff == -1) > + diff = iter->size; > + if ((iter->size - size) < diff) { > + diff = iter->size - size; > + found = iter; > + } > + } > + } while ((iter = CHUNK_ADDR(iter->next)) != CHUNK_ADDR(*list)); This search goes against how the pool code does memory management. In case you haven't read the pool code, what it does is keeps the last chunk free and the rest are not-free. In high alloc/free usage with lots of fragmentation, this code is going to be horrendously expensive (think what happens when you have lots of 200-byte chunks in a 1MB shared memory segment). Yuck. > + > + if (diff > MIN_BLK_SIZE) > + split_chunk(m, list, found, size); > + > + return found; > +} > + > +static void display_shm_snapshot(apr_shmem_t *m) > +{ > +#if 0 > + memchunk_t *iter; > + > + extern ap_log_error (); > + if (!m) return; > + > + if ((iter = CHUNK_ADDR(m->offsets->c_used)) != NULL) { > + ap_log_error(__FILE__, __LINE__, 3, 0, NULL, "USELIST :"); > + do { > + ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"[%ld] [%ld] [%ld]", > + iter->offset, iter->next, iter->size); > + } while (iter && > + ((iter = CHUNK_ADDR(iter->next)) != CHUNK_ADDR(m->offsets->c_used))); > + } > + > + if ((iter = CHUNK_ADDR(m->offsets->c_free)) != NULL) { > + ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"FREELIST :"); > + do { > + ap_log_error(__FILE__, __LINE__, 3, 0, NULL,"[%ld] [%ld] [%ld]", > + iter->offset, iter->next, iter->size); > + } while (iter && > + ((iter = CHUNK_ADDR(iter->next)) != CHUNK_ADDR(m->offsets->c_free))); > + } > + ap_log_error(__FILE__, __LINE__, 3, 0, NULL,""); > +#endif > + return; > +} Please no debugging code. Either the code works, or it doesn't. If it doesn't, then you fix it (and probably before it even gets committed). But, the debugging code does not belong in CVS, IMHO. > + > +static memchunk_t *alloc_chunk(apr_shmem_t *m, apr_size_t size) > +{ > + memchunk_t *b = NULL; > + > + /* Align size to a word boundary */ > + if (size < MIN_BLK_SIZE) > + size = MIN_BLK_SIZE; > + size = ((1 + ((size - 1) / sizeof (void *))) * sizeof (void *)); > + > + if (m->offsets->shm_length < size) > + return NULL; > + > + b = find_chunk_by_size (m, &(m->offsets->c_free), size); > + > + if (b != (memchunk_t *)NULL) > + remove_chunk (m, &(m->offsets->c_free), b); > + else { > + b = list_malloc(m); > + if (b == (memchunk_t *)NULL) > + return (memchunk_t *)NULL; > + b->offset = m->offsets->shm_offset; > + b->size = size; > + m->offsets->shm_offset += b->size; > + } > + > + m->offsets->shm_length -= b->size; > + add_chunk(m, &(m->offsets->c_used), b); > + > + display_shm_snapshot (m); > + > + return b; > +} > + > +static void free_chunk(apr_shmem_t *m, void *entity) > +{ > + memchunk_t *b; > + > + if (entity == NULL) > + return; > + > + b = find_chunk_by_addr (m, &(m->offsets->c_used), entity); > + if (b != (memchunk_t *)NULL) { > + remove_chunk(m, &(m->offsets->c_used), b); > + add_chunk(m, &(m->offsets->c_free), b); > + m->offsets->shm_length += b->size; > + } > + > + display_shm_snapshot (m); > +} > + > +static memchunk_t *realloc_chunk(apr_shmem_t *m, void *entity, apr_size_t size) > +{ > + memchunk_t *b, *new_b; > + > + /* Align size to a word boundary */ > + size = ((1 + ((size - 1) / sizeof (void *))) * sizeof (void *)); > + > + b = find_chunk_by_addr(m, &(m->offsets->c_used), entity); > + > + if (b != (memchunk_t *)NULL) { > + if (b->size > size) > + split_chunk (m, &(m->offsets->c_used), b, size); > + else > + if ((b->size < size) && (size < m->offsets->shm_length)) { > + new_b = alloc_chunk (m, size); > + memcpy (SHM_ADDR(new_b->offset), SHM_ADDR(b->offset), b->size); > + free_chunk (m, entity); > + b = new_b; > + } > + } > + return b; > +} > + > +/* > + * FIXME: > + * 1. Is APR_OS_DEFAULT sufficient? > + * 2. status = apr_file_remove(filename, p); > + * 3. Handle errors from return values of system calls. > + */ You moved the FIXMEs from the code itself (where the context was clearer) to the top of the function where the context has been lost. Please restore them to their proper place - this is how we typically record FIXMEs - as close to where the questionable code actually is. > APR_DECLARE(apr_status_t) apr_shm_init(apr_shmem_t **m, apr_size_t reqsize, > - const char *filename, apr_pool_t *pool) > + const char *filename, apr_pool_t *p) Why the change from pool to p? I don't see a reason for that. > { > - apr_shmem_t *new_m; > - void *mem; > #if APR_USE_SHMEM_SHMGET > struct shmid_ds shmbuf; > apr_uid_t uid; > apr_gid_t gid; > + int i, listsize; > + memlist_t *iter, *piter; > + void *addr; > #endif > + > #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM || APR_USE_SHMEM_MMAP_ZERO > apr_status_t status; > + status = APR_SUCCESS; > #endif > + > #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM || \ > APR_USE_SHMEM_MMAP_ZERO || APR_USE_SHMEM_SHMGET > int tmpfd; > #endif > > - new_m = apr_palloc(pool, sizeof(apr_shmem_t)); > - if (!new_m) > + (*m) = (apr_shmem_t *)apr_palloc(p, sizeof(apr_shmem_t)); You shouldn't modify m until you know you have succeeded (i.e. when you are about ready to return success). Also, the (*m) serves to make the code much less readable and actually slower since you might have to do derefencing a lot more. Please restore the new_m. It also makes the diff considerably less. > + if (!(*m)) > return APR_ENOMEM; > > -/* These implementations are very similar except for opening the file. */ > #if APR_USE_SHMEM_MMAP_TMP || APR_USE_SHMEM_MMAP_SHM || APR_USE_SHMEM_MMAP_ZERO > - /* FIXME: Ignore error for now. * > - * status = apr_file_remove(filename, pool);*/ > - status = APR_SUCCESS; > > #if APR_USE_SHMEM_MMAP_TMP > - /* FIXME: Is APR_OS_DEFAULT sufficient? */ > - status = apr_file_open(&new_m->file, filename, > - APR_READ | APR_WRITE | APR_CREATE, APR_OS_DEFAULT, > - pool); > + status = apr_file_open(&(*m)->file, filename, > + APR_READ | APR_WRITE | APR_CREATE, APR_OS_DEFAULT, p); > if (status != APR_SUCCESS) > return APR_EGENERAL; > > - status = apr_os_file_get(&tmpfd, new_m->file); > - status = apr_file_trunc(new_m->file, reqsize); > + status = apr_os_file_get(&tmpfd, (*m)->file); > + status = apr_file_trunc((*m)->file, reqsize); > if (status != APR_SUCCESS) > return APR_EGENERAL; > > #elif APR_USE_SHMEM_MMAP_SHM > - /* FIXME: Is APR_OS_DEFAULT sufficient? */ > tmpfd = shm_open(filename, O_RDWR | O_CREAT, APR_OS_DEFAULT); > if (tmpfd == -1) > return errno; > > - apr_os_file_put(&new_m->file, &tmpfd, pool); > - status = apr_file_trunc(new_m->file, reqsize); > + apr_os_file_put(&(*m)->file, &tmpfd, p); > + status = apr_file_trunc((*m)->file, reqsize); > if (status != APR_SUCCESS) > { > shm_unlink(filename); > return APR_EGENERAL; > } > #elif APR_USE_SHMEM_MMAP_ZERO > - status = apr_file_open(&new_m->file, "/dev/zero", APR_READ | APR_WRITE, > - APR_OS_DEFAULT, pool); > + status = apr_file_open(&(*m)->file, "/dev/zero", APR_READ | APR_WRITE, > + APR_OS_DEFAULT, p); > if (status != APR_SUCCESS) > return APR_EGENERAL; > - status = apr_os_file_get(&tmpfd, new_m->file); > + status = apr_os_file_get(&tmpfd, (*m)->file); > #endif > > - mem = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, MAP_SHARED, tmpfd, 0); > + (*m)->shm_base = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, > + MAP_SHARED, tmpfd, 0); > > #elif APR_USE_SHMEM_MMAP_ANON > - mem = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0); > + (*m)->shm_base = mmap(NULL, reqsize, PROT_READ|PROT_WRITE, > + MAP_ANON|MAP_SHARED,-1, 0); > #elif APR_USE_SHMEM_SHMGET > - tmpfd = shmget(IPC_PRIVATE, reqsize, (SHM_R|SHM_W|IPC_CREAT)); > - if (tmpfd == -1) > - return errno; > > - new_m->file = tmpfd; > + listsize = (reqsize/MIN_BLK_SIZE + 1) * sizeof(memlist_t) > + + sizeof (memoffsets_t); > + (*m)->listfd = shmget(IPC_PRIVATE, listsize, (SHM_R|SHM_W|IPC_CREAT)); > + if ((*m)->listfd == -1) > + return errno + APR_OS_START_SYSERR; Eh, what? It looks like this whole logic is only available if you are using shmget. No current OS actually uses shmget anymore - MAP_ANON should be our best choice. > + > + addr = (void *)shmat((*m)->listfd, NULL, 0); > + if (addr == (void *)NULL) > + return errno + APR_OS_START_SYSERR; > + > + (*m)->offsets = (memoffsets_t *)addr; > + (*m)->list_base = (memlist_t *)(addr + (sizeof(memoffsets_t))); > + > + (*m)->file = shmget(IPC_PRIVATE, reqsize, (SHM_R|SHM_W|IPC_CREAT)); > + if ((*m)->file == -1) > + return errno + APR_OS_START_SYSERR; > + > + (*m)->shm_base = (void *)shmat((*m)->file, NULL, 0); > + if ((*m)->shm_base == (void *)NULL) > + return errno + APR_OS_START_SYSERR; > > - mem = shmat(new_m->file, NULL, 0); > - > - /* FIXME: Handle errors. */ > - if (shmctl(new_m->file, IPC_STAT, &shmbuf) == -1) > + if (shmctl((*m)->file, IPC_STAT, &shmbuf) == -1) > return errno; > > - apr_current_userid(&uid, &gid, pool); > + apr_current_userid(&uid, &gid, p); > shmbuf.shm_perm.uid = uid; > shmbuf.shm_perm.gid = gid; > > - if (shmctl(new_m->file, IPC_SET, &shmbuf) == -1) > - return errno; > + if (shmctl((*m)->file, IPC_SET, &shmbuf) == -1) > + return errno + APR_OS_START_SYSERR; > > - /* remove in future (once use count hits zero) */ > - if (shmctl(new_m->file, IPC_RMID, NULL) == -1) > - return errno; > + if (shmctl((*m)->listfd, IPC_SET, &shmbuf) == -1) > + return errno + APR_OS_START_SYSERR; > > + /* Initially all elements after the first element is a free block */ > + piter = (*m)->list_base; > + for (i = 0; i < reqsize/MIN_BLK_SIZE; i++) { > + piter->next = i + 1; > + piter++; > + } > + piter->next = -1; > + > + (*m)->offsets->l_used = -1; > + (*m)->offsets->l_usedlast = -1; > + (*m)->offsets->l_free = 0; > + (*m)->offsets->l_freelast = i; > + (*m)->offsets->c_used = -1; > + (*m)->offsets->c_free = -1; > + > #elif APR_USE_SHMEM_BEOS > - new_m->area_id = create_area("mm", (void*)&mem, B_ANY_ADDRESS, reqsize, > - B_LAZY_LOCK, B_READ_AREA|B_WRITE_AREA); > - /* FIXME: error code? */ > - if (new_m->area_id < 0) > - return APR_EGENERAL; > > + (*m)->area_id = create_area("apr_shm", (void*)&((*m)->shm_base), B_ANY_ADDRESS, > + reqsize, B_LAZY_LOCK, B_READ_AREA|B_WRITE_AREA); > + if ((*m)->area_id < 0) > + return APR_EGENERAL; /* FIXME: error code? */ > #endif > > - new_m->mem = mem; > - new_m->curmem = mem; > - new_m->length = reqsize; > + (*m)->p = p; > + (*m)->offsets->shm_offset = 0; > + (*m)->offsets->shm_length = reqsize; > > - apr_lock_create(&new_m->lock, APR_MUTEX, APR_CROSS_PROCESS, NULL, pool); > - if (!new_m->lock) > + apr_lock_create(&(*m)->lock, APR_MUTEX, APR_CROSS_PROCESS, NULL, p); > + if (!(*m)->lock) > return APR_EGENERAL; > > - *m = new_m; > return APR_SUCCESS; > } At this point, it is obvious to me that this code only works with shmget (there is no extra allocation of the list structures for mmap()-based shared memory systems). I could be wrong, but I'm seeing no way that the list structures are going to be allocated in shared memory for anything other than shmget. (Or, you are using part of the memory that was originally allocated to store the list structures). If your intention is to support all shared memory, please submit a patch that does so. If you do not intend to support the other systems (and it'll only work with shmget), then this does not warrant inclusion into APR. -- justin
