Re: [PATCH] Shrinks sizeof(files_struct) and better layout

[Andi Kleen <ak@suse.de>]

On Wednesday 04 January 2006 12:13, Eric Dumazet wrote:
> Andi Kleen a écrit :
> > Eric Dumazet <dada1@cosmosbay.com> writes:
> >> 1) Reduces the size of (struct fdtable) to exactly 64 bytes on 32bits
> >> platforms, lowering kmalloc() allocated space by 50%.
> > 
> > It should be probably a kmem_cache_alloc() instead of a kmalloc
> > in the first place anyways. This would reduce fragmentation.
> 
> Well in theory yes, if you really expect thousand of tasks running...
> But for most machines, number of concurrent tasks is < 200, and using a 
> special cache for this is not a win.

It is because it avoids fragmentation because objects with similar livetimes
are clustered together. In general caches are a win
if the data is nearly a page or more.

> 
> > 
> >> +   * read mostly part
> >> +   */
> >>  	atomic_t count;
> >>  	struct fdtable *fdt;
> >>  	struct fdtable fdtab;
> >> -	fd_set close_on_exec_init;
> >> -	fd_set open_fds_init;
> >> +  /*
> >> +   * written part on a separate cache line in SMP
> >> +   */
> >> +	spinlock_t file_lock ____cacheline_aligned_in_smp;
> >> +	int next_fd;
> >> +	embedded_fd_set close_on_exec_init;
> >> +	embedded_fd_set open_fds_init;
> > 
> > You didn't describe that change, but unless it's clear the separate cache lines
> > are a win I would not do it and save memory again. Was this split based on
> > actual measurements or more theoretical considerations? 
> 
> As it is a refinement on a previous patch (that was integrated in 2.6.15) that 
> put spin_lock after the array[] (so cleary using a separate cache line), I 
> omited to describe it.

Ok, perhaps you should describe that too then

-Andi