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 |