Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common/buffer: replace RWLock with spinlocks #7294

Merged
merged 1 commit into from Jan 26, 2016

Conversation

branch-predictor
Copy link
Contributor

This decreases buffer::raw size by about 100 bytes (since RWLock occupies
104 bytes and simple_spinlock_t just 4) and also decreases time wasted
by locking and unlocking, possibly reducing context switches too.
Particularly visible with small I/O block size.
For example, putting a 4GB file in 4KB blocks is faster by around 4%:

Before:

# time rados -p storage put rnd randbytes -b 4096

real    37m23.600s
user    2m2.716s
sys     1m48.143s

After:

# time rados -p storage put rnd randbytes -b 4096

real    36m5.605s
user    1m54.048s
sys     1m42.558s

Also, memory usage decreases. Following code:

bufferlist test;                
char data[256];                 
for (int i=0; i<20000000; i++) {
  bufferptr pt(&data[0], 64);   
  test.append(pt);              
}

causes to consume 6.3GB of RAM before applying this patch and 3.6GB after.

Signed-off-by: Piotr Dałek piotr.dalek@ts.fujitsu.com

This decreases buffer::raw size by about 100 bytes (since RWLock occupies
104 bytes and simple_spinlock_t just 4) and also significantly decreases
time wasted by locking and unlocking, possibly reducing context switches too.
Particularly visible when using small blocksizes.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
@branch-predictor
Copy link
Contributor Author

One caveat here is that spinlocks consume CPU time when spinning, but that shouldn't be an issue as reading and storing CRC map entries is a quite fast operation.
Also, actual memory usage depends on workload and typically there's no big difference. From my understanding, the greatest difference will be seen once pglog_entry_t's snaps bufferlists will contain actual data.

@songbaisen
Copy link

@branch-predictor Hi,It is really a good job!But i have a question,the simple_spinlock_t lock is used with multi core machine.and the work is long,whether the simple_spinlock_t lock waster more time than RWLock lock? But if the work is short,the simple_spinlock_t lock use less time than RWLock lock.Because the RWLock lock use more time on context switch.So in this function the work should be short and quickly,is that right?

@branch-predictor
Copy link
Contributor Author

That's right, spinlocks are used when context switches are more expensive than active waiting (spinning), and for that reason they're used to protect short/fast code paths, like in bufferlist CRC code.

liewegas added a commit that referenced this pull request Jan 26, 2016
common/buffer: replace RWLock with spinlocks

Reviewed-by: Sage Weil <sage@redhat.com>
Reviewed-by: song baisen <song.baisen@zte.com.cn>
@liewegas liewegas merged commit 8097adb into ceph:master Jan 26, 2016
@branch-predictor branch-predictor deleted the bp-buffer-spinlock branch May 22, 2016 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants