Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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.

> 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.

Are you sure you need to remove and re-add the GPollFD when .events is
changed?

> +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().

> 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.

> +
> +    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.