lkml.org 
[lkml]   [2015]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 0/5] Optimize percpu-rwsem
On 05/26, Oleg Nesterov wrote:
>
> On 05/26, Linus Torvalds wrote:
> >
> >
> > We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.
> >
> > One.
>
> Well. IIRC Tejun is going to turn signal_struct->group_rwsem into
> percpu-rwsem.
>
> And it can have more users. Say, __sb_start_write/etc does something
> similar, and last time I checked this code it looked buggy to me.

I have found my old email, see below. Perhaps this code was changed
since 2013 when I sent this email, I didn't verify... but in any
case this logic doesn't look simple, imo it would be nice to rely
on the generic helpers from kernel/locking.

Oleg.

------------------------------------------------------------------------------
When I look at __sb_start_write/etc I am not sure this locking
is correct. OK, __sb_start_write() does:

percpu_counter_inc();

mb();

if (sb->s_writers.frozen)
abort_and_retry;

freeze_super() does STORE + mb + LOAD in reverse order so either
__sb_start_write() must see SB_FREEZE_WRITE or freeze_super() must
see the change in ->s_writers.counter. This is correct.

Still I am not sure sb_wait_write() can trust percpu_counter_sum(),
because it can also see _other_ changes.

To simplify the discussion, suppose that percpu_counter doesn't have
lock/count/batch/whatever and inc/dec/sum only uses "__percpu *counters".
Lets denote sb->s_writers.counter[level] as CTR[cpu].

Suppose that some thread did __sb_start_write() on CPU_1 and sleeps
"forever". CTR[0] == 0, CTR_[1] == 1, freezer_super() should block.

Now:

1. freeze_super() sets SB_FREEZE_WRITE, does mb(), and
starts sb_wait_write()->percpu_counter_sum().

2. __percpu_counter_sum() does for_each_online_cpu(),
reads CTR[0] == 0. ret = 0.

3. Another thread comes, calls __sb_start_write() on CPU_0,
increments CTR[0].

Then it notices sb->s_writers.frozen >= level and starts
__sb_end_write() before retry.

Then it migrates to CPU_1. And decrements CTR[1] before
__percpu_counter_sum() reads it.

So CTR[0] == 1, CTR[1] == 0. Everything is fine except
sb_wait_write() has already read CTR[0].

4. __percpu_counter_sum() continues, reads CTR[1] == 0
and returns ret == 0.

sb_wait_write() returns while it should not?



\
 
 \ /
  Last update: 2015-05-26 21:21    [W:0.154 / U:44.056 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site