RFC patchcommit-slab.h: Fix memory allocation and addressing

The slab initialization code includes the calculation of the slab 'elem_size', which is in turn used to determine the size (capacity) of the slab. Each element of the slab represents an array, of length 'stride', of 'elemtype'. (Note that it may be clearer if the define_commit_slab macro parameter was called 'basetype' rather than 'elemtype'). However, the 'elem_size' calculation incorrectly uses 'sizeof(struct slabname)' in the expression, rather than 'sizeof(elemtype)'.

Within the slab access routine, <slabname>_at(), the given commit 'index' is transformed into an (slab#, slot#) pair used to address the required element (a pointer to the first element of the array of 'elemtype' associated with that commit). The current code to calculate these address coordinates multiplies the commit index by the 'stride' which, at least for the slab#, produces the wrong result. Using the commit index directly, without scaling by the 'stride', produces the correct 'logical' address.

Also, when allocating a new slab, the size of the allocation only allows for a slab containing elements of single element arrays of 'elemtype'. This should allow for elements of an array of length 'stride' of 'elemtype'. In order to fix this, we need to change the element size parameter to xcalloc() by multiplying the current element size (sizeof(**s->slab)) by the s->stride.

Having changed the calculation of the slot#, we now need to convert the logical 'nth_slot', by scaling with s->stride, into the correct physical address.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Junio,

While looking into a sparse warning, which involved the use of the "commit-slab.h" header file, I noticed some problems with that code. (at least I _think_ I did! ;-)

I was convinced, just by reading the code in the header, that when
used with stride > 1, the memory allocated to a slab would not be
sufficient. (ie it would be too small by:
    s->slab_size * (sizeof(**s->slab) * (stride - 1))
). So, I had expected t3202-show-branch-octopus.sh to provoke memory
error reports when run under valgrind.

Hmm, it didn't ... so much for that theory! :-D

So, I'm a little puzzled; I must be missing something obvious, which is why this is marked RFC.

What am I missing?

ATB, Ramsay Jones

 commit-slab.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

Because sizeof(struct reachabe_cslab) is much larger than sizeof(unsigned char), and the test does not use that many commits to cause us to spill to more than one slab?

Your fix of elem_size in your first hunk is correct; for stride N, we want to allocate N*sizeof(elemtype) consecutive bytes in a single slab (we do not want to split the array of N elements across two slabs). So "elem_size" is the size of the array for a single commit (when stride==1, we are storing an array with a single element).

And we can fit arrays for s->slab_size = (COMMIT_SLAB_SIZE / elem_size) commits in a single slab. c->index / s->slab_size gives us which slab the array for the commit goes, and modulus gives us which slot in that slab the array sits. So the second hunk looks correct, too.

And of course, the slab must be allocated as an array with s->slab_size elements, each of which holds s->stride elements of elem_type that is sizeof(**s->slab) bytes long.

Thanks; the patch looks correct to me.

It is somewhat embarrassing that nobody caught it X-<.