PREEMPT_RT vs bcache

PREEMPT_RT vs bcache

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: PREEMPT_RT vs bcache

[Kent Overstreet]

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.

Re: PREEMPT_RT vs bcache

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: PREEMPT_RT vs bcache

[Kent Overstreet]

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...

Re: PREEMPT_RT vs bcache

[Uwe Kleine-König]

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/  |

Re: PREEMPT_RT vs bcache

[Christoph Hellwig]

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.

Re: PREEMPT_RT vs bcache

[Kent Overstreet]

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.

Re: PREEMPT_RT vs bcache

[Christoph Hellwig]

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.

Re: PREEMPT_RT vs bcache

[Thomas Gleixner]

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