[stgt] [PATCH] nonblocking epoll_wait loop, sched events, ISER/IB polling

[Alexander Nezhinsky]

On Fri, Sep 26, 2008 at 1:25 PM, FUJITA Tomonori
<fujita.tomonori at lab.ntt.co.jp> wrote:
> On Thu, 25 Sep 2008 19:09:09 +0300
> Alexander Nezhinsky <nezhinsky at gmail.com> wrote:
>
>> This patch introduces a few interconnected improvements, that
>> ...
>> Signed-off-by: Alexander Nezhinsky <nezhinsky at gmail.com>
>
> Thanks,
> Seems that you fixed most of the style issues but there are still some:
> ERROR: trailing whitespace

Sorry. Gonna fix it too.

>> -struct tgt_event {
>> -     union {
>> -             event_handler_t *handler;
>> -             counter_event_handler_t *counter_handler;
>> -     };
>> -     union {
>> -             int fd;
>> -             int *counter;
>> -     };
>> -     void *data;
>> -     struct list_head e_list;
>> -};
>
> exporting struct tgt_event fine for now but I don't fancy it much. If
> we have proper APIs for it, we could avoid it, I guess. But I've not
> see rdma code so I'm not sure.

It serves as a scheduled event descriptor, to request scheduling, to pass
the user parameters upon handler's call, to cancel, check etc.

Alternatively, we can define an API which returns a sort of handle
and passes a pre-defined format user data, like void *.

Also, if you feel that it was a bad idea to use the same struct both for
fd-based events and this new scheduler mechanism, they can be
separated, and only the new struct exported.

>> +static void event_loop(void)
>> +{
>> +     int timeout, wait_timeout = TGTD_TICK_PERIOD * 1000;
>> +
>> +     while (system_active) {
>> +             tgt_exec_scheduled();
>> +             /* wait if no scheduled work, poll if there is */
>> +             timeout = list_empty(&tgt_sched_events_list) ?
>> +                             wait_timeout : 0;
>> +             tgt_poll_events(timeout);
>> +     }
>
> Can you avoid rewriting (or cleaning up) even_loop()?
>
> That's not related with this patch. You want to replace the first half
> (counter_events code) in the original even_loop(). I don't think that
> you need to rewrite event_loop in the above way, right?
>
> If you do something like the following, your patch is more smaller, I
> think.
>
>
> static void event_loop(void)
> {
>        int nevent, i, done, timeout = TGTD_TICK_PERIOD * 1000;
>        struct epoll_event events[1024];
>        struct tgt_event *tev, *tevn;
>
> retry:
>        done = schedule_event(); /* choose whatever name you like */
>        if (!done)
>                timeout = 0;
>
>        nevent = epoll_wait(ep_fd, events, ARRAY_SIZE(events), timeout);
>        if (nevent < 0) {
>                if (errno != EINTR) {
>                        eprintf("%m\n");
>                        exit(1);
>                }
>        } else if (nevent) {
>                for (i = 0; i < nevent; i++) {
>                        tev = (struct tgt_event *) events[i].data.ptr;
>                        tev->handler(tev->fd, events[i].events, tev->data);
>                }
>        } else
>                schedule();
>
>        if (system_active)
>                goto retry;
> }
>

I thought that the new form is more expressive, but i can do it as you
suggested.
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html