On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote: >> From: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >> >> Bind each NetClientState with a GSource(ie,NetClientSource). Currently, >> these GSource attached with default context, but in future, after >> resolving the race between handlers and the interface exposed by >> NetClientInfo >> and other re-entrant issue, we can run NetClientState on different threads >> >> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >> --- >> include/net/net.h | 27 +++++++++++++++ >> net/net.c | 96 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> net/tap.c | 57 +++++++++++++++++++++++++------ >> 3 files changed, 169 insertions(+), 11 deletions(-) > > Please split this into two patches: > > 1. NetClientSource > 2. Convert tap to NetClientSource > > Once you do that it turns out that NetClientSource has nothing to do > with the net subsystem, it's a generic file descriptor GSource (weird > that glib doesn't already provide this abstraction). > > Each net client needs to reimplement .bind_ctx() anyway, so I don't see > much point in having NetClientSource.nsrc[]. We might as well let net > clients have that field themselves and destroy the GSource in their > destructor function. > The only way to detach the GSource from GMainContext is g_source_destroy, so if we want to re-bind nc from threadA to threadB, we should destroy the old one and create a new. Is that meaningful?
>> diff --git a/include/net/net.h b/include/net/net.h >> index cb049a1..8fdb7eb 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -44,6 +44,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const >> struct iovec *, int); >> typedef void (NetCleanup) (NetClientState *); >> typedef void (LinkStatusChanged)(NetClientState *); >> typedef void (NetClientDestructor)(NetClientState *); >> +typedef void (NetClientBindCtx)(NetClientState *, GMainContext *); >> >> typedef struct NetClientInfo { >> NetClientOptionsKind type; >> @@ -55,8 +56,25 @@ typedef struct NetClientInfo { >> NetCleanup *cleanup; >> LinkStatusChanged *link_status_changed; >> NetPoll *poll; >> + NetClientBindCtx *bind_ctx; >> } NetClientInfo; >> >> +typedef bool (*Pollable)(void *opaque); >> + >> +typedef struct NetClientSource { >> + GSource source; >> + GPollFD gfd; >> + GMainContext *ctx; >> + >> + Pollable readable; >> + Pollable writeable; >> + /* status returned by pollable last time */ >> + bool last_rd_sts; >> + bool last_wr_sts; > > Please use full names, then you can also drop the comment: > > bool last_readable_status; > bool last_writeable_status; > >> + >> + void *opaque; >> +} NetClientSource; >> + >> struct NetClientState { >> NetClientInfo *info; >> int link_down; >> @@ -69,6 +87,10 @@ struct NetClientState { >> unsigned receive_disabled : 1; >> NetClientDestructor *destructor; >> unsigned int queue_index; >> + /* For virtio net, rx on [0], tx on [1], so the virtque > > s/virtque/virtqueue/ > >> @@ -558,6 +564,96 @@ qemu_sendv_packet(NetClientState *nc, const struct >> iovec *iov, int iovcnt) >> return qemu_sendv_packet_async(nc, iov, iovcnt, NULL); >> } >> >> +static gboolean prepare(GSource *src, gint *time) >> +{ >> + NetClientSource *nsrc = (NetClientSource *)src; >> + bool rd, wr, change = false; >> + >> + if (!nsrc->readable && !nsrc->writeable) { >> + return false; >> + } >> + if (nsrc->readable) { >> + rd = nsrc->readable(nsrc->opaque); >> + if (nsrc->last_rd_sts != rd) { >> + nsrc->last_rd_sts = rd; >> + change = true; >> + } >> + } >> + if (nsrc->writeable) { >> + wr = nsrc->writeable(nsrc->opaque); >> + if (nsrc->last_wr_sts != wr) { >> + nsrc->last_wr_sts = wr; >> + change = true; >> + } >> + } >> + if (!change) { >> + return false; >> + } >> + >> + g_source_remove_poll(src, &nsrc->gfd); >> + if (rd) { >> + nsrc->gfd.events |= G_IO_IN; >> + } else { >> + nsrc->gfd.events &= ~G_IO_IN; >> + } >> + if (wr) { >> + nsrc->gfd.events |= G_IO_OUT; >> + } else { >> + nsrc->gfd.events &= ~G_IO_OUT; >> + } >> + g_source_add_poll(src, &nsrc->gfd); >> + return false; > > This seems equivalent to: > > int events = 0; > > if (nsrc->readable && nsrc->readable(nsrc->opaque)) { > events |= G_IO_IN; > } > if (nsrc->writeable && nsrc->writeable(nsrc->opaque)) { > events |= G_IO_OUT; > } > if (events != nsrc->gfd.events) { > g_source_remove_poll(src, &nsrc->gfd); > nsrc->gfd.events = events; > g_source_add_poll(src, &nsrc->gfd); > } > return FALSE; > > This way you can drop last_wr_sts/last_rd_sts. > Thanks for the simpler. > Are you sure you need to remove and re-add the GPollFD when .events is > changed? > Re-see the glib source code, and find it is not necessary. >> +void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc, >> + int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void >> *opaque) >> +{ >> + nsrc->gfd.fd = fd; >> + nsrc->gfd.events = events; >> + nsrc->opaque = opaque; >> + nsrc->readable = rd; >> + nsrc->writeable = wr; >> + nsrc->last_rd_sts = false; >> + nsrc->last_wr_sts = false; >> + if (idx == 0) { >> + nc->nsrc[0] = nsrc; >> + } else { >> + nc->nsrc[1] = nsrc; >> + } >> + /* add PollFD if it wont change, otherwise left for GSource prepare */ >> + if (!rd && !wr) { >> + g_source_add_poll(&nsrc->source, &nsrc->gfd); >> + } >> + g_source_set_callback(&nsrc->source, f, nsrc, NULL); >> +} > > Simpler version: > > NetClientSource *net_source_new(size_t size, int fd, > GSourceFunc f, void *opaque) > { > NetClientSource *nsrc = (NetClientSource > *)g_source_new(&net_gsource_funcs, > size); > memset(nsrc, 0, sizeof(*nsrc)); /* docs don't say whether memory is 0 */ > nsrc->gfd.fd = fd; > nsrc->opaque = opaque; > g_source_set_callback(&nsrc->source, f, nsrc, NULL); > return nsrc; > } > > net_gsource_funcs can be static, callers don't need to know about it. > > We don't need to mess with readable/writeable or g_source_add_poll(). > Let the caller do nsrc->readable = foo and let prepare() take care of > g_source_add_poll(). > I will follow this. >> diff --git a/net/tap.c b/net/tap.c >> index daab350..0b663d1 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -70,25 +70,48 @@ static int tap_can_send(void *opaque); >> static void tap_send(void *opaque); >> static void tap_writable(void *opaque); >> >> -static void tap_update_fd_handler(TAPState *s) >> +static bool readable(void *opaque) >> { >> - qemu_set_fd_handler2(s->fd, >> - s->read_poll && s->enabled ? tap_can_send : NULL, >> - s->read_poll && s->enabled ? tap_send : NULL, >> - s->write_poll && s->enabled ? tap_writable : NULL, >> - s); >> + TAPState *s = (TAPState *)opaque; > > No cast necessary from void* pointer. > Oh, will fix all this issue >> + >> + if (s->enabled && s->read_poll && >> + tap_can_send(s)) { >> + return true; >> + } >> + return false; >> +} >> + >> +static bool writeable(void *opaque) >> +{ >> + TAPState *s = (TAPState *)opaque; > > No cast necessary from void* pointer. > >> + >> + if (s->enabled && s->write_poll) { >> + return true; >> + } >> + return false; >> +} >> + >> +static gboolean tap_handler(gpointer data) >> +{ >> + NetClientSource *nsrc = (NetClientSource *)data; > > No cast necessary from gpointer. > >> + >> + if (nsrc->gfd.revents & G_IO_IN) { >> + tap_send(nsrc->opaque); >> + } >> + if (nsrc->gfd.revents & G_IO_OUT) { >> + tap_writable(nsrc->opaque); > > Since tap already uses "writable" please use it instead of "writeable" > in your patch. I grepped to check that "writeable" is not used in the > net subsystem. Ok, Thanks for your detail review Pingfan