Re: I'm a total push-over..

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: I'm a total push-over..

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:08

Linus Torvalds [off-list ref] writes:
+static inline size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
+{
+	long per_entry;
+
+	per_entry = sizeof(struct cache_entry) - sizeof(struct ondisk_cache_entry);
+
+	/*
+	 * Alignment can cause differences. This should be "alignof", but 
+	 * since that's a gcc'ism, just use the size of a pointer.
+	 */
+	per_entry += sizeof(void *);
+	return ondisk_size + entries*per_entry;
+}
+
I wonder if the issue Dave Miller addressed with
69ae517541ed5ab7d4fdcd8f82a9b8bd949df347 (fast-import: fix
unalinged allocation and access) applies here.

commit 69ae517541ed5ab7d4fdcd8f82a9b8bd949df347
Author: David S. Miller [off-list ref]
Date:   Fri Dec 14 20:39:16 2007 -0800

    fast-import: fix unalinged allocation and access
    
    The specialized pool allocator fast-import uses aligned objects on the
    size of a pointer, which was not sufficient at least on Sparc.  Instead,
    make the alignment for objects of type unitmax_t.
    
    Signed-off-by: David S. Miller [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

Re: I'm a total push-over..

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:44:08


On Thu, 24 Jan 2008, Junio C Hamano wrote:
I wonder if the issue Dave Miller addressed with
69ae517541ed5ab7d4fdcd8f82a9b8bd949df347 (fast-import: fix
unalinged allocation and access) applies here.
Good point, although we actually do things wrong for *another* reason.

We currently force cache_entry to be 8-byte aligned regardless of what the 
actual "sizeof(ptr)" is, so we should assume that alignment:

	#define cache_entry_size(len) ((offsetof(struct cache_entry,name) + (len) + 8) & ~7)

and if that isn't correct, we'd need to change this #define.

So right now, the right thing to do is probably to make this alignment 
explicit:

	#define CE_ALIGN 8

and then use that both in the "cache_entry_size()" _and_ in the 
"estimate_cache_size()" calculations to make it obvious what the alignment 
is.

And then we could actually make the alignment less on architectures that 
don't need that much (there may be architectures that need more, but I 
doubt it: we don't have any large fields in that structure, so the 
structure alignment really probably does max out at 8 in practice even if 
the C language theory doesn't give you any such guarantees).

Side note: this is not likely to be a problem in _practice_. The on-disk 
representation is also aligned (also by 8), and while they can be 
*differently* aligned due to the relative alignment of the varying-length 
"name[]" field, and that can cause some padding to be needed, in practice 
it will never matter. The on-disk size also contains a header that we 
don't take into account, so it's already "over-estimated" to begin with 
for the in-memory representation.

So "estimate_cache_size()" really does over-estimate its needs by a 
biggish amount, which is why it all works regardless, but better safe than 
sorry. 

		Linus
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help