On 09/24/2015 05:12 PM, Markus Armbruster wrote:
Yang Hongyang <yan...@cn.fujitsu.com> writes:
This filter is to buffer/release packets, this feature can be used
when using MicroCheckpointing, or other Remus like VM FT solutions, you
What's "Remus"?
Remus is an opensource VM FT solution:
paper: http://http//www.cs.ubc.ca/~andy/papers/remus-nsdi-final.pdf
First implemented on Xen:
http://wiki.xen.org/wiki/Remus
MicroCheckpointing in QEMU is another Remus implementation.
can also use it to simulate the network delay.
It has an interval option, if supplied, this filter will release
packets by interval.
Suggest "will delay packets by that time interval."
Is interval really optional?
It supposed to be optional. When the buffer filter has a user:
http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg00363.html
In this patchset, the zero interval check is removed. packets are
released on demand through filter_buffer_release_all() api call.
Usage:
-netdev tap,id=bn0
-object filter-buffer,id=f0,netdev=bn0,chain=in,interval=1000
NOTE:
the scale of interval is microsecond.
Perhaps "interval is in microseconds".
Better, thanks.
Signed-off-by: Yang Hongyang <yan...@cn.fujitsu.com>
---
v11: add a fixme comment from Jason
v10: use NetQueue flush api to flush packets
sent_cb can not be called when we already return size
v9: adjustment due to the qapi change
v7: use QTAILQ_FOREACH_SAFE() when flush packets
v6: move the interval check earlier and some comment adjust
v5: remove dummy sent_cb
change interval type from int64 to uint32
check interval!=0 when initialise
rename FILTERBUFFERState to FilterBufferState
v4: remove bh
pass the packet to next filter instead of receiver
v3: check packet's sender and sender->peer when flush it
---
net/Makefile.objs | 1 +
net/filter-buffer.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-options.hx | 18 ++++++
vl.c | 7 ++-
4 files changed, 195 insertions(+), 1 deletion(-)
create mode 100644 net/filter-buffer.c
diff --git a/net/Makefile.objs b/net/Makefile.objs
index 914aec0..5fa2f97 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -14,3 +14,4 @@ common-obj-$(CONFIG_SLIRP) += slirp.o
common-obj-$(CONFIG_VDE) += vde.o
common-obj-$(CONFIG_NETMAP) += netmap.o
common-obj-y += filter.o
+common-obj-y += filter-buffer.o
diff --git a/net/filter-buffer.c b/net/filter-buffer.c
new file mode 100644
index 0000000..ef94e91
--- /dev/null
+++ b/net/filter-buffer.c
@@ -0,0 +1,170 @@
+/*
+ * Copyright (c) 2015 FUJITSU LIMITED
+ * Author: Yang Hongyang <yan...@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "net/filter.h"
+#include "net/queue.h"
+#include "qemu-common.h"
+#include "qemu/timer.h"
+#include "qemu/iov.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+
+#define TYPE_FILTER_BUFFER "filter-buffer"
+
+#define FILTER_BUFFER(obj) \
+ OBJECT_CHECK(FilterBufferState, (obj), TYPE_FILTER_BUFFER)
+
+struct FilterBufferState {
+ NetFilterState parent_obj;
+
+ NetQueue *incoming_queue;
+ uint32_t interval;
+ QEMUTimer release_timer;
+};
+typedef struct FilterBufferState FilterBufferState;
Again, not splitting the declaration is more concise.
+
+static void filter_buffer_flush(NetFilterState *nf)
+{
+ FilterBufferState *s = FILTER_BUFFER(nf);
+
+ if (!qemu_net_queue_flush(s->incoming_queue)) {
+ /* Unable to empty the queue, purge remaining packets */
+ qemu_net_queue_purge(s->incoming_queue, nf->netdev);
+ }
+}
This either flushes or purges incoming_queue, where "purge" means
dropping packets. Correct?
I think it is correct. Dropping packets is allowed, even on real
hardware, packets may lose.
+
+static void filter_buffer_release_timer(void *opaque)
+{
+ NetFilterState *nf = opaque;
+ FilterBufferState *s = FILTER_BUFFER(nf);
Style nit: blank line between declarations and statements, please.
+ filter_buffer_flush(nf);
Is purging correct here?
+ timer_mod(&s->release_timer,
+ qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
Timer rearmed to fire again in s->interval microseconds.
Yes.
+}
+
+/* filter APIs */
+static ssize_t filter_buffer_receive_iov(NetFilterState *nf,
+ NetClientState *sender,
+ unsigned flags,
+ const struct iovec *iov,
+ int iovcnt,
+ NetPacketSent *sent_cb)
+{
+ FilterBufferState *s = FILTER_BUFFER(nf);
+
+ /*
+ * we return size when buffer a packet, the sender will take it as
+ * a already sent packet, so sent_cb should not be called later
Humor me: when a comment has multiple sentences, start each one with a
capital letter, and end it with punctuation.
Ok.
+ * FIXME: even if guest can't receive packet for some reasons. Filter
+ * can still accept packet until its internal queue is full.
+ */
I'm not sure I understand the comment.
This is taken from Jason's comments, may be he can have a better explain?
+ qemu_net_queue_append_iov(s->incoming_queue, sender, flags,
+ iov, iovcnt, NULL);
+ return iov_size(iov, iovcnt);
+}
+
+static void filter_buffer_cleanup(NetFilterState *nf)
+{
+ FilterBufferState *s = FILTER_BUFFER(nf);
+
+ if (s->interval) {
+ timer_del(&s->release_timer);
+ }
+
+ /* flush packets */
+ if (s->incoming_queue) {
+ filter_buffer_flush(nf);
I guess purging is correct here.
+ g_free(s->incoming_queue);
+ }
+}
+
+static void filter_buffer_setup(NetFilterState *nf, Error **errp)
+{
+ FilterBufferState *s = FILTER_BUFFER(nf);
+
+ /*
+ * this check should be dropped when there're VM FT solutions like MC
+ * or COLO use this filter to release packets on demand.
+ */
If you end a sentence with a period, you get to start it with a capital
letter :)
Ok, thanks.
"there're" is odd. Perhaps something like "We may want to accept zero
interval when VM FT solutions like MC or COLO use this filter to release
packets on demand."
+ if (!s->interval) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "interval",
+ "a non-zero interval");
How can this happen? Doesn't filter_buffer_set_interval() catch zero
intervals already?
When user do not supply an interval parameter, filter_buffer_set_interval()
won't be called, and the interval is 0.
When we have actual user like I mentioned above, we should allow user to omit
the interval option.
+ return;
+ }
+
+ s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
+ if (s->interval) {
Condition cannot be false. Same in filter_buffer_cleanup().
+ timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
+ filter_buffer_release_timer, nf);
+ timer_mod(&s->release_timer,
+ qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
Timer armed to fire in s->interval microseconds.
Unless I misunderstand something, this doesn't actually delay each
packet by s->interval microseconds, it batches packet delivery: all
packets arriving in a given interval are delayed until the end of the
interval. Correct?
Correct.
+ }
+}
+
+static void filter_buffer_class_init(ObjectClass *oc, void *data)
+{
+ NetFilterClass *nfc = NETFILTER_CLASS(oc);
+
+ nfc->setup = filter_buffer_setup;
+ nfc->cleanup = filter_buffer_cleanup;
+ nfc->receive_iov = filter_buffer_receive_iov;
+}
+
+static void filter_buffer_get_interval(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ FilterBufferState *s = FILTER_BUFFER(obj);
+ uint32_t value = s->interval;
+
+ visit_type_uint32(v, &value, name, errp);
+}
+
+static void filter_buffer_set_interval(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ FilterBufferState *s = FILTER_BUFFER(obj);
+ Error *local_err = NULL;
+ uint32_t value;
+
+ visit_type_uint32(v, &value, name, &local_err);
+ if (local_err) {
+ goto out;
+ }
+ if (!value) {
+ error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
+ PRIu32 "'", object_get_typename(obj), name, value);
What does it take? That's what the user wants to know... Perhaps
"Property '%s.%s' requires a positive value".
Maybe better, thanks.
+ goto out;
+ }
+ s->interval = value;
+
+out:
+ error_propagate(errp, local_err);
+}
+
+static void filter_buffer_init(Object *obj)
+{
+ object_property_add(obj, "interval", "int",
+ filter_buffer_get_interval,
+ filter_buffer_set_interval, NULL, NULL, NULL);
+}
+
+static const TypeInfo filter_buffer_info = {
+ .name = TYPE_FILTER_BUFFER,
+ .parent = TYPE_NETFILTER,
+ .class_init = filter_buffer_class_init,
+ .instance_init = filter_buffer_init,
+ .instance_size = sizeof(FilterBufferState),
+};
+
+static void register_types(void)
+{
+ type_register_static(&filter_buffer_info);
+}
+
+type_init(register_types);
diff --git a/qemu-options.hx b/qemu-options.hx
index 7e147b8..b09f97f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3642,6 +3642,24 @@ in PEM format, in filenames @var{ca-cert.pem},
@var{ca-crl.pem} (optional),
@var{server-cert.pem} (only servers), @var{server-key.pem} (only servers),
@var{client-cert.pem} (only clients), and @var{client-key.pem} (only clients).
+@item -object
filter-buffer,id=@var{id},netdev=@var{netdevid}[,chain=@var{all|in|out}][,interval=@var{t}]
+
+Buffer network packets on netdev @var{netdevid}.
+If interval @var{t} provided, will release packets by interval.
+Interval scale: microsecond.
+
+If interval @var{t} not provided, you have to make sure the packets can be
Are you sure omitting t works? filter_buffer_set_interval() rejects
zero...
When we omit t, filter_buffer_set_interval() won't be called.
+released, either by manually remove this filter or call the release buffer API,
+otherwise, the packets will be buffered forever. Use with caution.
Please wrap your lines a bit earlier.
+
+chain @var{all|in|out} is an option that can be applied to any netfilter,
default is @option{all}.
+
+@option{all} means this filter will receive packets both sent to/from the
netdev
+
+@option{in} means this filter will receive packets sent to the netdev
+
+@option{out} means this filter will receive packets sent from the netdev
+
I'd describe this like "filter is inserted in the receive / transmit
queue".
thanks.
@end table
ETEXI
diff --git a/vl.c b/vl.c
index ec589e2..3cf89d5 100644
--- a/vl.c
+++ b/vl.c
@@ -2794,7 +2794,12 @@ static bool object_create_initial(const char *type)
if (g_str_equal(type, "rng-egd")) {
return false;
}
- /* TODO: return false for concrete netfilters */
+
+ /* return false for concrete netfilters */
I find this comment useless, please drop it :)
Ok, thanks.
+ if (g_str_equal(type, "filter-buffer")) {
+ return false;
+ }
+
return true;
}
.
--
Thanks,
Yang.
