I don't think this is really a merge-blocker, but this code seems to
have faith in the talismanic power of atomic_t to avoid races in nearby
code. The following anti-pattern appears in a few other variants in the
driver as well (just search for atomic_inc_return).
> + if (atomic_inc_return(&irp->relock_timer_active) == 1) {
> + init_timer(&irp->relock_timer);
> + irp->relock_timer.function = qib_run_relock;
> + irp->relock_timer.data = (unsigned long) dd;
> + irp->relock_interval = timeout;
> + irp->relock_timer.expires = jiffies + timeout;
> + add_timer(&irp->relock_timer);
> + } else {
> + irp->relock_interval = timeout;
> + mod_timer(&irp->relock_timer, jiffies + timeout);
> + atomic_dec(&irp->relock_timer_active);
> + }
Think about if two threads hit this code at the same time:
if (atomic_inc_return(&active) == 1) {
/* yep, first time through */
if (atomic_inc_return(&active) == 1) {
/* nope, go to else */
} else {
irp->relock_interval = timeout;
/* err, who set up this timer?
*/
mod_timer(&irp->relock_timer,
jiffies + timeout);
atomic_dec(&irp->relock_timer_active);
}
/* a bit too late, the timer might already have run */
init_timer(&irp->relock_timer);
irp->relock_timer.function = qib_run_relock;
irp->relock_timer.data = (unsigned long) dd;
irp->relock_interval = timeout;
irp->relock_timer.expires = jiffies + timeout;
add_timer(&irp->relock_timer);
Now, maybe this is safe because the code is actually never running
concurrently, but using this atomic_t relock_timer_active makes it
really hard to see that this code is safe. Why not just use a proper
lock for this, since it doesn't look particularly performance critical
(and an atomic_t is really the same cost as a spinlock anyway)?
As I said, not a merge-blocker but it would be nice to get this fixed
someday before everyone loses interest in this driver.
- R.
--
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
