> Subject: Re: [ofa-general] [PATCH] [WORKAROUND] CONFIG_PREEMPT_RT and
ib_umad_close() issue
> From: Roland Dreier <[email protected]>
> Date: Mon, 17 Sep 2007 08:56:01 -0700
> To: John Blackwood <[email protected]>
> CC: [email protected], [email protected],
[email protected], Sven-Thorsten Dietrich <[email protected]>
>
> > When using OFED-1.2.5 based infiniband kernel modules on 2.6.22 based
> > kernels with the Ingo Molnar CONFIG_PREEMPT_RT applied, then commands
> > such as ibnetdiscvoer, smpquery, sminfo, etc. will hang. The problem
> > is with the downgrade_write() rw semaphore usage in the
> > ib_umad_close() routine.
>
> Can you give a few more details on how PREEMPT_RT changes locking
> rules (or just exposes existing bugs maybe?) so that the
> downgrade_write() causes the issue? I would like to fix this cleanly
> but I don't really understand what the problem is.
>
> - R.
Hi Roland,
Thanks for your interest in this matter.
I'm not one of the preempt rt experts, so others may want to speak up ...
(thanks Daniel...)
But basically, with CONFIG_PREEMPT_RT enabled, the lock points, such as
aqcuiring a spinlock, potentially become places where the current task
may be context switched out / preempted.
Therefore, when a call is made to lock a spinlock for example, the
caller should not currently have irqs disabled, or preemption disabled,
since a context switch may occur.
I believe that in the case of rw_semaphores, the comments
in include/linux/rt_lock.h with the rt preempt patch applied say:
/*
* RW-semaphores are a spinlock plus a reader-depth count.
*
* Note that the semantics are different from the usual
* Linux rw-sems, in PREEMPT_RT mode we do not allow
* multiple readers to hold the lock at once, we only allow
* a read-lock owner to read-lock recursively. This is
* better for latency, makes the implementation inherently
* fair and makes it simpler as well:
*/
So I believe that a read lock on a rw_semaphore is just as
exclusive as the old write lock, except that the read locks
may nest.
And with the preempt patch enabled, the downgrade_write() becomes:
void fastcall rt_downgrade_write(struct rw_semaphore *rwsem)
{
BUG();
}
EXPORT_SYMBOL(rt_downgrade_write);
So I think code such as:
ib_umad_close()
{
...
down_write(&file->port->mutex);
... do exclusive stuff
downgrade_write(&file->port->mutex);
... do potentially recursive stuff
up_read(&file->port->mutex);
...
}
Could probably become (only when CONFIG_PREEMPT_RT is enabled):
ib_umad_close()
{
...
down_read(&file->port->mutex);
... do exclusive stuff
... do potentially recursive stuff
up_read(&file->port->mutex);
...
}
since the down_read will not allow other readers at the same time,
but will allow nesting.
I'm not aware of any tools that find these issues, other than
just running through the code.
I do know that Ingo's preempt rt patch can be found at
http://www.kernel.org/pub/linux/kernel/projects/rt
and applied to an infiniband kernel.
If you enabled CONFIG_PREEMPT_RT, and maybe also enable
parameters such as
CONFIG_DEBUG_PREEMPT, CONFIG_DEBUG_SPINLOCK, etc. you should
see the issue with something like a ibnetdiscover invocation.
Thanks.
Thanks for the explanation...
> But basically, with CONFIG_PREEMPT_RT enabled, the lock points, such as
> aqcuiring a spinlock, potentially become places where the current task
> may be context switched out / preempted.
>
> Therefore, when a call is made to lock a spinlock for example, the
> caller should not currently have irqs disabled, or preemption disabled,
> since a context switch may occur.
this doesn't seem relevant here...
> void fastcall rt_downgrade_write(struct rw_semaphore *rwsem)
> {
> BUG();
> }
this seems to be the problem... the -rt patch turns downgrade_write()
into a BUG().
I need to look at the locking in user_mad.c again, but I think it may
be possible to replace both places that do downgrade_write() with
up_write() followed by down_read().
- R.
Roland Dreier wrote:
> Thanks for the explanation...
>
> > But basically, with CONFIG_PREEMPT_RT enabled, the lock points, such as
> > aqcuiring a spinlock, potentially become places where the current task
> > may be context switched out / preempted.
> >
> > Therefore, when a call is made to lock a spinlock for example, the
> > caller should not currently have irqs disabled, or preemption disabled,
> > since a context switch may occur.
>
> this doesn't seem relevant here...
Hi Roland,
right. just some background info.
> > void fastcall rt_downgrade_write(struct rw_semaphore *rwsem)
> > {
> > BUG();
> > }
>
> this seems to be the problem... the -rt patch turns downgrade_write()
> into a BUG().
>
> I need to look at the locking in user_mad.c again, but I think it may
> be possible to replace both places that do downgrade_write() with
> up_write() followed by down_read().
>
> - R.
that sounds like it would be a good solution for both preempt rt and
non-preempt rt kernels.
thanks again for looking at this for us.