Messages in this thread | | | Date | Tue, 14 Jul 2015 15:37:31 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH RFC 0/4] change sb_writers to use percpu_rw_semaphore |
| |
On 07/14, Jan Kara wrote: > > Hello, > > On Mon 13-07-15 23:25:36, Oleg Nesterov wrote: > > Al, Jan, could you comment? I mean the intent, the patches are > > obviously not for inclusion yet. > > Thanks for the patches! Hum, what do people have with freeze protection > these days? Noone cared about it for years and sudddently two patch sets > within a month :) Anyway, have you seen the patch set from Dave Hansen? > > It is here: https://lkml.org/lkml/2015/6/24/682 > He modifies the freezing primitives in a different way. AFAICS the > resulting performance of the fast path should be about the same.
At first glance, 2-3 do something similar, yes...
> So unless > I'm missing something and there is a significant performance advantage to > Dave's patches I'm all for using a generic primitive you suggest.
I think percpu_rw_semaphore looks a bit better. And even a bit faster. And it will not block __sb_start_write() entirely while freeze_super() sleeps in synchronize_rcu().
freeze_super() should be faster too after rcu_sync changes, but this is not that important.
But again, to me the main advantage is that we can use the generic primitives and remove this nontrivial code in fs/super.c.
> Can you perhaps work with Dave on some common resolution?
Dave, what do you think? Will you agree with percpu_rw_semaphore ?
> > - __sb_start_write() will be a little bit faster, but this > > is minor. > > Actually Dave could measure the gain achieved by removing the barrier. It > would be good to verify that your patches achieve a similar gain.
The fast path in percpu_down_read() is really fast, it does a plain LOAD plus __this_cpu_add() under preempt_disable(). I doubt this can be improved. The actual code is:
static bool update_fast_ctr(struct percpu_rw_semaphore *brw, unsigned int val) { bool success = false;
preempt_disable(); if (likely(!atomic_read(&brw->write_ctr))) { __this_cpu_add(*brw->fast_read_ctr, val); success = true; } preempt_enable();
return success; }
void percpu_down_read(struct percpu_rw_semaphore *brw) { might_sleep(); if (likely(update_fast_ctr(brw, +1))) { rwsem_acquire_read(&brw->rw_sem.dep_map, 0, 0, _RET_IP_); return; }
down_read(&brw->rw_sem); atomic_inc(&brw->slow_read_ctr); __up_read(&brw->rw_sem); }
> > - Fix get_super_thawed(), it will spin if MS_RDONLY... > > > > It is not clear to me what exactly should we do, but this > > doesn't look hard. Perhaps it can just return if MS_RDONLY. > > What's the exact problem here?
Note that freeze_super() does not do sb_wait_write() (which blocks __sb_start_write) if MS_RDONLY. This means that after 1/4 get_super_thawed() will spin until SB_UNFROZEN in this case, sb_start_write() won't block. But please forget, this is not the problem. I mean, afaics this is easy to fix, but the initial version should just keep ->wait_unfrozen specially for get_super_thawed(), then we can remove it in a separate patch.
Thanks!
Oleg.
| |