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

[Eric Dumazet <dada1@cosmosbay.com>]

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.

> 
>> +   * 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.

Yes, this part is really important because some multi-threaded benchmarks get 
a nice speedup with this separation in two parts.

Threads that are doing read()/write() (reading the first part of files_struct) 
are not slowed by others that do open()/close() syscalls (writing the second 
part), no false sharing (and no locking thanks to RCU of course).

Big Apache 2.0 servers, database servers directly benefit from this.

Note that process that tend to create/destroy a lot of threads per second are 
writing into 'count' field and might dirty the 'read mostly' part, but we can 
expect that well writen high performance programs wont do this.

Eric