On Fri, Sep 26, 2008 at 1:25 PM, FUJITA Tomonori
<fujita.tomonori@xxxxxxxxxxxxx> wrote:
> On Thu, 25 Sep 2008 19:09:09 +0300
> Alexander Nezhinsky <nezhinsky@xxxxxxxxx> wrote:
>
>> This patch introduces a few interconnected improvements, that
>> ...
>> Signed-off-by: Alexander Nezhinsky <nezhinsky@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
