[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib
From: |
liu ping fan |
Subject: |
Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib |
Date: |
Wed, 3 Apr 2013 17:28:39 +0800 |
On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.
>
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
- Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib,
liu ping fan <=