Re: About a shortcoming of the verbs API

Bart Van Assche
2010-08-08T18:19:03Z

On Sat, Aug 7, 2010 at 6:32 PM, Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote:
> Not sure that I follow the problem you're worried about.  A given
> tasklet can only be running on one CPU at any one time -- if an
> interrupt occurs and reschedules the tasklet then it just runs again
> when it exits.
>
> Also I'm not sure I understand why this is special for IB hardware --
> standard practice is for interrupt handlers to clear the interrupt
> source and reenable interrupts, so I don't see why the same thing you
> describe can't happen with any interrupt-generating device that defers
> work to a tasklet.

One of the applications I have been looking at is adding blk-iopoll
support in ib_srp and to make it possible to enable and disable
blk-iopoll at runtime via sysfs. A naive implementation could look
e.g. as follows:

/* Poll the IB receive queue once. Returns zero if this function
should be called again. */
static int srp_recv_poll(struct ib_cq *cq, struct srp_target_port *target)
{
struct ib_wc wc;

do {
if (ib_poll_cq(cq, 1, &wc) > 0) {
if (wc.status == IB_WC_SUCCESS) {
srp_handle_recv(target, &wc);
return 0;
} else {
shost_printk(KERN_ERR, target->scsi_host,
PFX "failed receive status %d\n",
wc.status);
target->qp_in_error = 1;
}
}
} while (ib_req_notify_cq(target->recv_cq, IB_CQ_NEXT_COMP
| IB_CQ_REPORT_MISSED_EVENTS) > 0);
return -1;
}

/* blk-iopoll callback function, which is invoked on tasklet context. */
static int srp_iopoll(struct blk_iopoll *iop, int budget)
{
struct srp_target_port *target;
int processed;

target = container_of(iop, struct srp_target_port, iopoll);
for (processed = 0; processed < budget; processed++) {
if (srp_recv_poll_once(target->recv_cq, target) != 0) {
blk_iopoll_complete(iop);
break;
}
}
return processed;
}

/* receive completion queue notification callback function, which is
typically invoked on IRQ context. */
static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
{
struct srp_target_port *target = target_ptr;

if (target->iopoll_enabled
&& blk_iopoll_sched_prep(&target->iopoll) == 0)
blk_iopoll_sched(&target->iopoll);
else
while (srp_recv_poll_once(cq, target) == 0)
;
}

/* sysfs callback function that shows the current value of
target->blk_iopoll_enabled */

/* sysfs callback function that allows to set the value of
target->blk_iopoll_enabled */

As far as I can see with the above implementation enabling blk-iopoll
mode would be fine, but disabling not, because while disabling
blk-iopoll mode it could e.g. happen that srp_iopoll() is still
polling the receive completion queue on one CPU in tasklet context and
srp_recv_completion() starts polling the receive queue simultaneously
on another CPU in IRQ context. This can happen independently of
whether loop (1) or loop (2) is used to poll the IB receive completion
queue (see also http://www.spinics.net/lists/linux-rdma/msg05003.html
for the definitions of loop (1) and (2)). Although it is possible to
wait for completion of a tasklet by calling tasklet_disable(), I don't
think it is safe to call this function from IRQ context because that
might trigger a deadlock.

Bart.