As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
{down,up}_read_non_owner(). But these are not implemented by the -rt
patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
there a fundamental conflict here?
Ben.
--
Ben Hutchings
Experience is what causes a person to make new mistakes instead of old ones.
On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> {down,up}_read_non_owner(). But these are not implemented by the -rt
> patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> there a fundamental conflict here?
You should be able to cherry pick
84759c6d18c5144432781ddca037d929ee9db8a5 (Revert "rw_semaphore: remove
up/down_read_non_owner") - that went in when bcache was merged.
On Wed, 2013-08-07 at 13:53 -0700, Kent Overstreet wrote:
> On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> > As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> > {down,up}_read_non_owner(). But these are not implemented by the -rt
> > patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> > there a fundamental conflict here?
>
> You should be able to cherry pick
> 84759c6d18c5144432781ddca037d929ee9db8a5 (Revert "rw_semaphore: remove
> up/down_read_non_owner") - that went in when bcache was merged.
That's the commit I was referring to. But the -rt patchset has a
separate implementation of semaphores for PREEMPT_RT_FULL.
Ben.
--
Ben Hutchings
Experience is what causes a person to make new mistakes instead of old ones.
On Wed, Aug 07, 2013 at 11:08:42PM +0200, Ben Hutchings wrote:
> On Wed, 2013-08-07 at 13:53 -0700, Kent Overstreet wrote:
> > On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> > > As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> > > {down,up}_read_non_owner(). But these are not implemented by the -rt
> > > patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> > > there a fundamental conflict here?
> >
> > You should be able to cherry pick
> > 84759c6d18c5144432781ddca037d929ee9db8a5 (Revert "rw_semaphore: remove
> > up/down_read_non_owner") - that went in when bcache was merged.
>
> That's the commit I was referring to. But the -rt patchset has a
> separate implementation of semaphores for PREEMPT_RT_FULL.
Ahh - that makes more sense...
On Wed, Aug 07, 2013 at 01:53:57PM -0700, Kent Overstreet wrote:
> On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> > As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> > {down,up}_read_non_owner(). But these are not implemented by the -rt
> > patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> > there a fundamental conflict here?
>
> You should be able to cherry pick
> 84759c6d18c5144432781ddca037d929ee9db8a5 (Revert "rw_semaphore: remove
> up/down_read_non_owner") - that went in when bcache was merged.
That doesn't help with PREEMPT_RT_FULL because include/linux/rwsem.h
looks like:
[ ... some includes ... ]
#ifdef CONFIG_PREEMPT_RT_FULL
#include <linux/rwsem_rt.h>
#else /* PREEMPT_RT_FULL */
[ ... vanilla content including definitions of {down,up}_read_non_owner]
#endif
So Ben's question was how (if at all) we should implement
{down,up}_read_non_owner for PREEMPT_RT_FULL.
Is it save to just use the vanilla implementation on RT?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> {down,up}_read_non_owner(). But these are not implemented by the -rt
> patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> there a fundamental conflict here?
How did they get back in at all? I'm pretty sure I removed them for
good reason.
On Thu, Aug 08, 2013 at 12:26:23AM -0700, Christoph Hellwig wrote:
> On Wed, Aug 07, 2013 at 10:28:18PM +0200, Ben Hutchings wrote:
> > As Kent said back in 2011 (commit 84759c6d18c5), bcache needs
> > {down,up}_read_non_owner(). But these are not implemented by the -rt
> > patchset when PREEMPT_RT_FULL is enabled. Can they be added, or is
> > there a fundamental conflict here?
>
> How did they get back in at all? I'm pretty sure I removed them for
> good reason.
I seem to recall from looking at the logs that you just removed them
because all the old users could be and were converted to something
saner, for what they were doing (using them as completions, I want to
say?)
Bcache isn't using the rw sem as a completion though, it really is a
read/write lock that protects a specific data structure, and where
we're taking a read lock for the duration of write IOs - and since bios
are asynchronous, that's why we need the non_owner() bit.
On Thu, Aug 08, 2013 at 12:43:26AM -0700, Kent Overstreet wrote:
> I seem to recall from looking at the logs that you just removed them
> because all the old users could be and were converted to something
> saner, for what they were doing (using them as completions, I want to
> say?)
We explicitly converted them away so that we could kill it. This was
a joint project with Thomas.
> Bcache isn't using the rw sem as a completion though, it really is a
> read/write lock that protects a specific data structure, and where
> we're taking a read lock for the duration of write IOs - and since bios
> are asynchronous, that's why we need the non_owner() bit.
Part of this commit was to make the rw_semaphore behaviour similar to
plain mutex, that is making sure there is exactly one owner and not
different processes locking/unlocking it. This is useful for PI (that's
why the rt folks care), lock debugging and kinds of other use cases.
On Thu, 8 Aug 2013, Christoph Hellwig wrote:
> On Thu, Aug 08, 2013 at 12:43:26AM -0700, Kent Overstreet wrote:
> > I seem to recall from looking at the logs that you just removed them
> > because all the old users could be and were converted to something
> > saner, for what they were doing (using them as completions, I want to
> > say?)
>
> We explicitly converted them away so that we could kill it. This was
> a joint project with Thomas.
>
> > Bcache isn't using the rw sem as a completion though, it really is a
> > read/write lock that protects a specific data structure, and where
> > we're taking a read lock for the duration of write IOs - and since bios
> > are asynchronous, that's why we need the non_owner() bit.
>
> Part of this commit was to make the rw_semaphore behaviour similar to
> plain mutex, that is making sure there is exactly one owner and not
> different processes locking/unlocking it. This is useful for PI (that's
> why the rt folks care), lock debugging and kinds of other use cases.
Right. We had to implement an anon_rw_semaphore version, which caused
more headache than it was worth the trouble.
The solution for one of the non owner use cases was something like the
below:
read_lock(x->lock);
atomic_inc(x->io_active);
launch_io();
read_unlock(x->lock);
On the writer side:
write_lock(x->lock);
while (atomic_read(x->io_active) {
write_unlock(x->lock);
wait_event(x->wait_for_io, !atomic_read(x->io_active));
write_lock(x->io_active);
}
....
On the io side:
complete_io()
if (atomic_dec_and_test(x->io_active) &&
waitqueue_active(x->wait_for_io))
wake_up(x->wait_for_io);
That would fit into the bcache use case afacit.
Thanks,
tglx