Subject: Re: [ofa-general] [POSSIBLE BUG] use of tasklet_unlock in
	ipath_no_bufs_available
From: Ralph Campbell <ralph.campbell@qlogic.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
       Andrew Morton <akpm@linux-foundation.org>,
       "David S. Miller" <davem@davemloft.net>,
       Roland Dreier <rdreier@cisco.com>, openib-general@openib.org,
       Oleg Nesterov <oleg@tv-sign.ru>, Christoph Hellwig <hch@infradead.org>,
       Ingo Molnar <mingo@elte.hu>,
       Linus Torvalds <torvalds@linux-foundation.org>,
       Thomas Gleixner <tglx@linutronix.de>
In-Reply-To: <1182799994.5493.201.camel@localhost.localdomain>
References: <1182799994.5493.201.camel@localhost.localdomain>
Content-Type: text/plain
Organization: QLogic
Date: Mon, 25 Jun 2007 13:37:01 -0700
Message-Id: <1182803821.18911.237.camel@brick.pathscale.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3972
Lines: 106

This was fixed by a patch that Arthur Jones sent out to
general@lists.openfabrics.org

Tue Jun 19 16:42:09 PDT 2007
[PATCH 17/28] IB/ipath - wait for PIO available interrupt

I imagine that it is working its way into Roland's git tree
for Linus.

On Mon, 2007-06-25 at 15:33 -0400, Steven Rostedt wrote:
> As some of you know, lately I've been trying to get rid of tasklets. In
> doing so, I've come across this usage of tasklet_unlock.
> 
> The only user of tasklet_unlock in the kernel outside of softirq.c is
> ipath_no_bufs_available in drivers/inifiniband/hw/ipath/ipath_ruc.c
> 
> Here's the offending code:
> 
> void ipath_no_bufs_available(struct ipath_qp *qp, struct ipath_ibdev *dev)
> {
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&dev->pending_lock, flags);
> 	if (list_empty(&qp->piowait))
> 		list_add_tail(&qp->piowait, &dev->piowait);
> 	spin_unlock_irqrestore(&dev->pending_lock, flags);
> 	/*
> 	 * Note that as soon as want_buffer() is called and
> 	 * possibly before it returns, ipath_ib_piobufavail()
> 	 * could be called.  If we are still in the tasklet function,
> 	 * tasklet_hi_schedule() will not call us until the next time
> 	 * tasklet_hi_schedule() is called.
> 	 * We clear the tasklet flag now since we are committing to return
> 	 * from the tasklet function.
> 	 */
> 	clear_bit(IPATH_S_BUSY, &qp->s_flags);
> 	tasklet_unlock(&qp->s_task);
> 	want_buffer(dev->dd);
> 	dev->n_piowait++;
> }
> 
> 
> As the comment states, it looks like it's trying to prevent a race where
> the want_buffer can allow for ipath_ib_piobufavail be called which would
> schedule this tasklet again. But since the tasklet is running, it would
> simply be skipped if it were to schedule on another CPU. And this would
> mean that the tasklet would need to wait for it to be scheduled again
> before doing the work.
> 
>   Is my above analysis correct?
> 
> Now for the BUG.
> 
> Lets say this situation does happen. Lets look at the code.
> 
> softirq.c: tasklet_hi_action
> 
> 		if (tasklet_trylock(t)) {
> 			if (!atomic_read(&t->count)) {
> 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
> 					BUG();
> 				t->func(t->data);
> 				tasklet_unlock(t);
> 				continue;
> 			}
> 			tasklet_unlock(t);
> 		}
> 
> The race being prevented is the failure of the tasklet_trylock running
> on another CPU. The call to tasklet_unlock in ipath_no_bufs_available is
> letting the other CPU succeed, and the comment suggests that this is OK
> because this function will be exiting shortly. But what it doesn't take
> into consideration is the above "tasklet_unlock" called again in
> tasklet_hi_action.
> 
> So while the tasklet function is allowed to run on another CPU, we are
> unlocking the tasklet on this CPU. So now this tasklet function is no
> longer protected from being reentrant. There is now no guarantee that
> the tasklet function would only be running on one CPU.
> 
> What's worse, we also add the chance of hitting the above BUG(). If the
> tasklet gets scheduled again, takes an interrupt before doing the
> tast_and_clear, another CPU runs the tasklet and clears the
> TASKLET_STATE_SCHED, when the first instance comes back from the
> interrupt, it will hit the BUG.
> 
> So, does all this make sense, or am I full of crap.  Still, I think
> tasklet_unlock and tasklet_trylock should not be exported for anyone
> else to use besides softirq.c and perhaps the ipath code needs to find a
> better way around this.
> 
> -- Steve
> 
> 
> _______________________________________________
> general mailing list
> general@lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/