CONFIG_PREEMPT_RT and ib_umad_close() issue

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

this doesn't seem relevant here...

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.

Hi Roland,

right. just some background info.

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.