apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1)" <madhusudan_mathiha...@hp.com>
Subject RE: [PATCH] shmem.c - 3rd try
Date Tue, 18 Sep 2001 18:40:11 GMT
Hi Justin,
It should be really trivial to implement using mmap - I just want to
finalize on the logic before I attempt other forms of shared memory.
As regards your comments , I had some answers : (inline ). [I'll include the
style comments in my next patch].. 


>> +
>> +typedef apr_int32_t index_t;
>Does this really need to have a typedef?

It need not be one. But I have it so that it's easier to go thru' the code
and realize that elem, first etc are indexes and nothing more.. A
apr_int32_t type for those is not intutive.. 

>> +
>> +static index_t posn_in_sorted_list(apr_shmem_t *m, index_t *first,
index_t elem)
>> +{
>> +    index_t idx = *first;
>> +
>> +    if (idx < 0) return -1;
>> +
>> +    do {
>> +        if (m->list[idx].offset > m->list[elem].offset)
>> +            break;
>> +        idx = m->list[idx].next;
>> +    } while ((idx >= 0) && (idx < MAX_LIST_ELEM(m)) && (idx
!= *first));
>> +    return idx;
>> +}
>
>Hmm.  You have an array and rather than reshuffling the array elements
>to keep the list sorted, you treat it internally as a linked list but 
>retrieve the values as array indices?  Something strikes me the wrong 
>way about this approach.  Wouldn't it be better to just use it as
>a true linked list?

It's very much a doubly linked list.. It's just the addressing that's
different. Instead of maintaining the address of the next element, I
maintain the index of the next element - so as to eliminate the problem of
the shared memory being mapped to different address locations in different
processes. I can afford to do this because the maximum number of elements in
the list is fixed.. 


>> +
>> +static void add_list (apr_shmem_t *m, index_t *first, index_t elem)
>
>Still have the space between add_list and the (.

All styling guidelines will be implemented :-|.. 

>> +    if (idx == m->offsets->c_free)
>> +        *first = elem;
>
>How would this conditional ever be executed?  Could both elem and
>*first be m->offsets->c_free?

the c_free list is always supposed to be a sorted list, with the lowest
offset being the first element. Suppose block-40 was free, and now I have
block-5 freed, then c_free should point to block-5.. In this case, c_free is
initially pointing to 40, idx is also 40 - so, c_free should now start
pointing to 5.. 


>> +static void free_list(apr_shmem_t *m, index_t elem)
>> +{
>> +    index_t idx = elem;
>
>The assignment to elem is not needed.

Will do.. 

>> +        m->list[m->list[idx].prev].next = idx; 
>> +        m->list[m->list[idx].next].prev = idx;
>> +        m->offsets->l_total--;
>> +    }
>> +}
>
>I'm not really sure what this function is trying to achieve
>(one line comments about each function would do worlds of wonder
>here).  You seem to be copying in the value of the the list at
>m->offsets->l_total (what if there are no free elements?).
>You then seem to be setting up the DLL for idx - which doesn't
>seem to make any sense.  What are you trying to do?


It's for freeing a list element during the garbage collection. Suppose I
decide that 2 blocks can be merged to one single bigger block, then I update
the size item in the first list element and free the second list element.
The freed list element is at the end of the list.. This avoids the need for
another linked list for maintaining the set of available list elements.

>> +            return idx;
>> +    } while ((idx >= 0) && ((idx = m->list[idx].next) != first));
>> +
>> +    return NULL;
>> +}
>
>Again you are doing a linear search which bugs me (hashing?).  Also, 
>wouldn't it be faster to just subtract offset from addr and then
>you don't have to do the adjustment inside the search.  Would be
>faster...)

I'll look into this ...


>> +    do {
>> +        if (m->list[idx].size == size)
>> +            return idx;
>> +        if (m->list[idx].size > size) {
>> +            if (diff == -1)
>> +                diff = m->list[idx].size;
>> +            if ((m->list[idx].size - size) < diff) {
>> +                diff = m->list[idx].size - size;
>> +                found = idx;
>> +            }
>
>Be better off if you did:
>if (diff == -1 || (m->list[idx].size - size) < diff) {
>  diff = m->list[idx].size - size;
>  found = idx;
>}


Sure, will do..


>> +
>> +static memchunk_t *alloc_chunk(apr_shmem_t *m, apr_size_t size)
>> +{
>> +    index_t idx;
>> +
>> +    size = ROUND_UP(size);
>
>Since we are dealing with already allocated space, I'm not sure
>if we need to align the chunks.  It'd seem to be a waste of memory.
>I dunno.

It's probably true.. I'm doing it to ensure safeness.. Also, it would help
guarding memory boundaries (ofcourse not much - but to a certain extent
atleast)..


>> +
>> +    if (m->offsets->shm_length < size)
>> +        return NULL;
>
>This condition isn't always true to test based on what I can tell.
>shm_length seems to be the total space, but it isn't necessarily
>the *contiguous* space available.  Consider a 516KB segment:

The Garbage collection logic is yet to be implemented. It would certainly
make sense at that time.

>> +
>> +    return ((idx >= 0) ? &(m->list[idx]) : NULL);
>
>How would the false branch of this conditional ever get hit?


Probably not.. 'shall remove it.


>> +static memchunk_t *realloc_chunk(apr_shmem_t *m, void *entity,
apr_size_t size)
>> +{
>> +    index_t idx;
>> +    memchunk_t *new_b;
>> +
>> +    size = ROUND_UP(size);
>
>Not sure if we need to be page-aligned.  Dunno.

'shall check it out..


>> -    new_m = apr_palloc(pool, sizeof(apr_shmem_t));
>> +    new_m = (apr_shmem_t *)apr_palloc(pool, sizeof(apr_shmem_t));
>
>Don't need to cast here.  =-)  void* goes to anything.

It's to ensure that some compilers do not complain when I use new_m as
apr_shmem_t.. Anyways, does it really matter if I have it ?..


>> -        return errno;
>> +        return errno + APR_OS_START_SYSERR;
>
>I believe this is incorrect.  I had this with + SYSERR at one point 
>and we took it out because errno doesn't get adjusted.  I'm confused 
>as to what it should be.  But, I think what it is in there now is
>correct (i.e. don't do adjustments).  Ask on the list...

Okay.. will check it out.. 


>I'll look over the apr_shm_* functions again once the other issues
>have been addressed.  -- justin

I shall send out the updated patch by today noon.. 

Thanks,
-Madhu

Mime
View raw message