lkml.org 
[lkml]   [2015]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2015-07-14 16:01    [W:0.339 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site