Patchwork [RFC,v2,1/4] net: port tap onto glib

login
register
mail settings
Submitter pingfan liu
Date March 28, 2013, 7:55 a.m.
Message ID <1364457355-4119-2-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/231923/
State New
Headers show

Comments

pingfan liu - March 28, 2013, 7:55 a.m.
From: Liu Ping Fan <pingfank@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 <pingfank@linux.vnet.ibm.com>
---
 include/net/net.h |   27 +++++++++++++++
 net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/tap.c         |   57 +++++++++++++++++++++++++------
 3 files changed, 169 insertions(+), 11 deletions(-)
Stefan Hajnoczi - March 28, 2013, 2:32 p.m.
On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@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 <pingfank@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.

> 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.
pingfan liu - April 3, 2013, 9:28 a.m.
On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@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 <pingfank@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
Stefan Hajnoczi - April 8, 2013, 11:44 a.m.
On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfank@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 <pingfank@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?

I guess that can be done.

What I was really thinking when I suggested getting rid of nsrc[] is
that it's a little ugly to have the array with 2 GSources.  Different
net clients have different numbers of GSources - 0 for NICs, 1 for most
backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.

So my thought was to leave the number of GSources in the layer that uses
them - each specific net client.

Stefan
pingfan liu - April 9, 2013, 5:12 a.m.
On Mon, Apr 8, 2013 at 7:44 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
>> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
>> >> From: Liu Ping Fan <pingfank@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 <pingfank@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?
>
> I guess that can be done.
>
> What I was really thinking when I suggested getting rid of nsrc[] is
> that it's a little ugly to have the array with 2 GSources.  Different
> net clients have different numbers of GSources - 0 for NICs, 1 for most
> backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.
>
> So my thought was to leave the number of GSources in the layer that uses
> them - each specific net client.
>
OK, see. What about nsrc[0] at the end of struct or **nsrc ?

> Stefan
Stefan Hajnoczi - April 11, 2013, 9:09 a.m.
On Tue, Apr 09, 2013 at 01:12:39PM +0800, liu ping fan wrote:
> On Mon, Apr 8, 2013 at 7:44 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
> >> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> >> >> From: Liu Ping Fan <pingfank@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 <pingfank@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?
> >
> > I guess that can be done.
> >
> > What I was really thinking when I suggested getting rid of nsrc[] is
> > that it's a little ugly to have the array with 2 GSources.  Different
> > net clients have different numbers of GSources - 0 for NICs, 1 for most
> > backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.
> >
> > So my thought was to leave the number of GSources in the layer that uses
> > them - each specific net client.
> >
> OK, see. What about nsrc[0] at the end of struct or **nsrc ?

The net core code doesn't need to know about nsrc at all.  I think
nsrc[] should be dropped completely.  The net backends should manage
their GSource (if they need one).

Stefan

Patch

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;
+
+    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
+     * can run on different threads.
+     */
+    NetClientSource * nsrc[2];
 };
 
 typedef struct NICState {
@@ -155,6 +177,9 @@  extern int default_net;
 extern const char *legacy_tftp_prefix;
 extern const char *legacy_bootp_filename;
 
+void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc,
+    int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void *opaque);
+
 int net_client_init(QemuOpts *opts, int is_netdev, Error **errp);
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(void);
@@ -190,4 +215,6 @@  unsigned compute_mcast_idx(const uint8_t *ep);
     .offset     = vmstate_offset_macaddr(_state, _field),            \
 }
 
+extern GSourceFuncs net_gsource_funcs;
+
 #endif
diff --git a/net/net.c b/net/net.c
index f3d67f8..abe8fef 100644
--- a/net/net.c
+++ b/net/net.c
@@ -299,6 +299,12 @@  static void qemu_free_net_client(NetClientState *nc)
     }
     g_free(nc->name);
     g_free(nc->model);
+    if (nc->nsrc[0]) {
+        g_source_remove(g_source_get_id(&nc->nsrc[0]->source));
+    }
+    if (nc->nsrc[1]) {
+        g_source_remove(g_source_get_id(&nc->nsrc[1]->source));
+    }
     if (nc->destructor) {
         nc->destructor(nc);
     }
@@ -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;
+}
+
+static gboolean check(GSource *src)
+{
+    NetClientSource *nsrc = (NetClientSource *)src;
+
+    if (nsrc->gfd.revents & nsrc->gfd.events) {
+        return true;
+    }
+    return false;
+}
+
+static gboolean dispatch(GSource *src, GSourceFunc cb, gpointer data)
+{
+    gboolean ret = false;
+
+    if (cb) {
+        ret = cb(data);
+    }
+    return ret;
+}
+
+GSourceFuncs net_gsource_funcs = {
+    prepare,
+    check,
+    dispatch,
+    NULL
+};
+
+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);
+}
+
 NetClientState *qemu_find_netdev(const char *id)
 {
     NetClientState *nc;
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;
+
+    if (s->enabled && s->read_poll &&
+        tap_can_send(s)) {
+        return true;
+    }
+    return false;
+}
+
+static bool writeable(void *opaque)
+{
+    TAPState *s = (TAPState *)opaque;
+
+    if (s->enabled && s->write_poll) {
+        return true;
+    }
+    return false;
+}
+
+static gboolean tap_handler(gpointer data)
+{
+    NetClientSource *nsrc = (NetClientSource *)data;
+
+    if (nsrc->gfd.revents & G_IO_IN) {
+        tap_send(nsrc->opaque);
+    }
+    if (nsrc->gfd.revents & G_IO_OUT) {
+        tap_writable(nsrc->opaque);
+    }
+    return true;
 }
 
 static void tap_read_poll(TAPState *s, bool enable)
 {
     s->read_poll = enable;
-    tap_update_fd_handler(s);
 }
 
 static void tap_write_poll(TAPState *s, bool enable)
 {
     s->write_poll = enable;
-    tap_update_fd_handler(s);
 }
 
 static void tap_writable(void *opaque)
@@ -298,6 +321,7 @@  static void tap_cleanup(NetClientState *nc)
 static void tap_poll(NetClientState *nc, bool enable)
 {
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
+    /* fixme, when tap backend on another thread, the disable should be sync */
     tap_read_poll(s, enable);
     tap_write_poll(s, enable);
 }
@@ -309,6 +333,11 @@  int tap_get_fd(NetClientState *nc)
     return s->fd;
 }
 
+static void tap_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+    g_source_attach(&nc->nsrc[0]->source, ctx);
+}
+
 /* fd support */
 
 static NetClientInfo net_tap_info = {
@@ -319,6 +348,7 @@  static NetClientInfo net_tap_info = {
     .receive_iov = tap_receive_iov,
     .poll = tap_poll,
     .cleanup = tap_cleanup,
+    .bind_ctx = tap_bind_ctx,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
@@ -596,13 +626,18 @@  static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                             int vnet_hdr, int fd)
 {
     TAPState *s;
+    NetClientSource *nsrc;
 
     s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     if (!s) {
         close(fd);
         return -1;
     }
-
+    nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
+            sizeof(NetClientSource));
+    net_init_gsource(&s->nc, 0, nsrc, s->fd, G_IO_IN|G_IO_OUT,
+            readable, writeable, tap_handler, s);
+    s->nc.info->bind_ctx(&s->nc, NULL);
     if (tap_set_sndbuf(s->fd, tap) < 0) {
         return -1;
     }
@@ -843,8 +878,8 @@  int tap_enable(NetClientState *nc)
     } else {
         ret = tap_fd_enable(s->fd);
         if (ret == 0) {
+            /*fixme, will be sync to ensure handler not be called */
             s->enabled = true;
-            tap_update_fd_handler(s);
         }
         return ret;
     }
@@ -861,8 +896,8 @@  int tap_disable(NetClientState *nc)
         ret = tap_fd_disable(s->fd);
         if (ret == 0) {
             qemu_purge_queued_packets(nc);
+            /*fixme, will be sync to ensure handler not be called */
             s->enabled = false;
-            tap_update_fd_handler(s);
         }
         return ret;
     }