On Tue, Sep 23, 2008 at 11:06 AM, FUJITA Tomonori
<fujita.tomonori@xxxxxxxxxxxxx> wrote:
>> > Pete Wyckoff wrote:
>> >>> +static void iser_poll_cq_normal(struct iser_device *dev) +{
>> >>> + int ret;
>> >>> +
>> >>> + ret = iser_poll_cq(dev,8);
>> >>> + if (ret < 0)
>> >>> + exit(1);
>> >>
>> >> Please eprintf() then exit.
This exit (and a couple of other calls to iser_poll_cq) does not
eprintf because
it is already issued in iser_poll_cq, in case ret is negative.
>> Unfortunately most of the exit()s reveal "bugs" in the code, in
>> particular, unhandled issues due to resource exhaustion.
>
> I think that iser code should do better in resource exhaustion
> situations instead of just calling exit().
This is one case, the other is when something went wrong in the underlying
IB machinery. In most such cases the connection is being shut down
asynchronously and among the next events we'll see flushed rx buffers and
connection shutdown notification. But i guess in most such cases we can't tell
so we have to initiate the shutdown by ourselves. Am i right? Are there any
pitfalls in doing this?
Anyway, i'm going to leave the exit() calls, because they are orthogonal (as Or
pointed out) to the submitted patch. Let's discuss those issues (resource
allocations, conn. shutdowns, BUG_ON etc.), i'm ready to work on it and
prepare an additional patch later.
> It sure took me a long time to (maybe?) understand this logic.
> You're trying to switch between active polling and blocking on the
> IB CQ. First fixup the naming. normal == poll_cq, but drain ==
> drain_cq. A problem with "drain" is that word is used when shutting
> down a connection. How about "iser_check_cq_poll" and
> "iser_check_cq_block" and do as appropriate with the other functions
> starting with those names? Comments would help too.
I reworked this portion a bit, and added some comments, hope this will be
clearer this time.
The difference is not in polling and blocking (at least there is no
immediate blocking). The basic function is iser_poll_cq().
As the name suggests, it just polls cq, up to a given number of times
and returns zero if cq is found empty, positive if more comletions may
remain. Another one, which wraps it, renamed to iser_poll_cq_armable(),
because it reacts to the ret. value of iser_poll_cq() and when cq is still
non-empty, schedules iser_sched_poll_cq() to continue normal
piece-wise polling. If cq is empty it re-arms the interrupts and schedules
iser_sched_consume_cq(), just to consume the completions which
could sneak in between the moment the cq was seen empty and
just before the interrupts were re-armed.
I'll send the corrected patch shortly.
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
