From: Stefan Hajnoczi <stefanha@gmail.com>
To: Liu Ping Fan <qemulist@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Anthony Liguori <aliguori@us.ibm.com>,
qemu-devel@nongnu.org, mdroth <mdroth@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib
Date: Thu, 28 Mar 2013 15:32:18 +0100 [thread overview]
Message-ID: <20130328143218.GH22865@stefanha-thinkpad.redhat.com> (raw)
In-Reply-To: <1364457355-4119-2-git-send-email-qemulist@gmail.com>
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.
next prev parent reply other threads:[~2013-03-28 14:32 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-28 7:55 [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib Liu Ping Fan
2013-03-28 7:55 ` [Qemu-devel] [RFC PATCH v2 1/4] net: port tap " Liu Ping Fan
2013-03-28 14:32 ` Stefan Hajnoczi [this message]
2013-04-03 9:28 ` liu ping fan
2013-04-08 11:44 ` Stefan Hajnoczi
2013-04-09 5:12 ` liu ping fan
2013-04-11 9:09 ` Stefan Hajnoczi
2013-03-28 7:55 ` [Qemu-devel] [RFC PATCH v2 2/4] net: resolve race of tap backend and its peer Liu Ping Fan
2013-03-28 14:34 ` Stefan Hajnoczi
2013-03-29 7:21 ` liu ping fan
2013-03-29 14:38 ` Stefan Hajnoczi
2013-03-28 7:55 ` [Qemu-devel] [RFC PATCH v2 3/4] net: port hub onto glib Liu Ping Fan
2013-03-28 14:47 ` Stefan Hajnoczi
2013-03-29 7:21 ` liu ping fan
2013-03-28 7:55 ` [Qemu-devel] [RFC PATCH v2 4/4] net: port virtio net " Liu Ping Fan
2013-03-28 8:42 ` [Qemu-devel] [RFC PATCH v2 0/4] port network layer " Paolo Bonzini
2013-03-28 13:40 ` Stefan Hajnoczi
2013-04-02 9:49 ` liu ping fan
2013-04-08 11:46 ` Stefan Hajnoczi
2013-04-09 5:10 ` liu ping fan
2013-04-11 9:19 ` Stefan Hajnoczi
2013-03-28 14:55 ` Stefan Hajnoczi
2013-03-28 17:41 ` mdroth
2013-04-01 8:15 ` liu ping fan
2013-04-08 11:49 ` Stefan Hajnoczi
2013-04-09 5:10 ` liu ping fan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130328143218.GH22865@stefanha-thinkpad.redhat.com \
--to=stefanha@gmail.com \
--cc=aliguori@us.ibm.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemulist@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).