From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56620) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLMps-0007pQ-3o for qemu-devel@nongnu.org; Mon, 18 Jan 2016 22:20:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLMpm-000478-JQ for qemu-devel@nongnu.org; Mon, 18 Jan 2016 22:20:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLMpm-00046o-Ap for qemu-devel@nongnu.org; Mon, 18 Jan 2016 22:19:54 -0500 References: <1451372975-5048-1-git-send-email-zhang.zhanghailiang@huawei.com> <1451372975-5048-35-git-send-email-zhang.zhanghailiang@huawei.com> From: Jason Wang Message-ID: <569DAB42.2010008@redhat.com> Date: Tue, 19 Jan 2016 11:19:30 +0800 MIME-Version: 1.0 In-Reply-To: <1451372975-5048-35-git-send-email-zhang.zhanghailiang@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v13 34/39] net/filter-buffer: Add default filter-buffer for each netdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zhanghailiang , qemu-devel@nongnu.org Cc: xiecl.fnst@cn.fujitsu.com, lizhijian@cn.fujitsu.com, quintela@redhat.com, yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, dgilbert@redhat.com, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com, zhangchen.fnst@cn.fujitsu.com, hongyang.yang@easystack.cn On 12/29/2015 03:09 PM, zhanghailiang wrote: > We add each netdev (except vhost-net) a default filter-buffer, > which will be used for COLO or Micro-checkpoint to buffer VM's packets. > The name of default filter-buffer is 'nop'. > For the default filter-buffer, it will not buffer any packets in default. > So it has no side effect for the netdev. > > Signed-off-by: zhanghailiang > Cc: Jason Wang > Cc: Yang Hongyang This patch did three things: 1) the ability to enable or disable a netfilter 2) the ability to add a default filter 3) default filter attaching for filter-buffer Better to split them into separate small patches. And several questions: For 1), I'm not sure this is real needed, we can in fact disable a filter by removing it. For 2), Instead of a specific code just for filter buffer, I think we need a generic method for an arbitrary filter to be used as default. And if we can achieve 2), 3) is not needed any more. > --- > v12: > - Skip vhost-net when add default filter > - Don't go through filter layer if the filter is disabled. > v11: > - New patch > --- > include/net/filter.h | 10 +++++++ > net/filter-buffer.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > net/filter.c | 6 +++- > net/net.c | 12 ++++++++ > 4 files changed, 109 insertions(+), 1 deletion(-) > > diff --git a/include/net/filter.h b/include/net/filter.h > index 2deda36..40aa38c 100644 > --- a/include/net/filter.h > +++ b/include/net/filter.h > @@ -56,6 +56,8 @@ struct NetFilterState { > NetClientState *netdev; > NetFilterDirection direction; > char info_str[256]; > + bool is_default; > + bool enabled; > QTAILQ_ENTRY(NetFilterState) next; > }; > > @@ -74,4 +76,12 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender, > int iovcnt, > void *opaque); > > +static inline bool qemu_need_skip_netfilter(NetFilterState *nf) > +{ > + return nf->enabled ? false : true; > +} > + > +void netdev_add_default_filter_buffer(const char *netdev_id, > + NetFilterDirection direction, > + Error **errp); > #endif /* QEMU_NET_FILTER_H */ > diff --git a/net/filter-buffer.c b/net/filter-buffer.c > index 57be149..9cf3544 100644 > --- a/net/filter-buffer.c > +++ b/net/filter-buffer.c > @@ -14,6 +14,13 @@ > #include "qapi/qmp/qerror.h" > #include "qapi-visit.h" > #include "qom/object.h" > +#include "net/net.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qmp-output-visitor.h" > +#include "qapi/qmp-input-visitor.h" > +#include "monitor/monitor.h" > +#include "qmp-commands.h" > +#include "net/vhost_net.h" > > #define TYPE_FILTER_BUFFER "filter-buffer" > > @@ -102,6 +109,7 @@ static void filter_buffer_cleanup(NetFilterState *nf) > static void filter_buffer_setup(NetFilterState *nf, Error **errp) > { > FilterBufferState *s = FILTER_BUFFER(nf); > + char *path = object_get_canonical_path_component(OBJECT(nf)); > > /* > * We may want to accept zero interval when VM FT solutions like MC > @@ -114,6 +122,14 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp) > } > > s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf); > + nf->is_default = !strcmp(path, "nop"); > + /* > + * For the default buffer filter, it will be disabled by default, > + * So it will not buffer any packets. > + */ > + if (nf->is_default) { > + nf->enabled = false; > + } > if (s->interval) { > timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL, > filter_buffer_release_timer, nf); > @@ -163,6 +179,72 @@ out: > error_propagate(errp, local_err); > } > > +/* > +* This will be used by COLO or MC FT, for which they will need > +* to buffer the packets of VM's net devices, Here we add a default > +* buffer filter for each netdev. The name of default buffer filter is > +* 'nop' > +*/ > +void netdev_add_default_filter_buffer(const char *netdev_id, > + NetFilterDirection direction, > + Error **errp) > +{ Need a more generic way to add an arbitrary filter as default. E.g during netdev init, query if there's a default and do the initialization there. > + QmpOutputVisitor *qov; > + QmpInputVisitor *qiv; > + Visitor *ov, *iv; > + QObject *obj = NULL; > + QDict *qdict; > + void *dummy = NULL; > + const char *id = "nop"; > + char *queue = g_strdup(NetFilterDirection_lookup[direction]); > + NetClientState *nc = qemu_find_netdev(netdev_id); > + Error *err = NULL; > + > + /* FIXME: Not support multiple queues */ > + if (!nc || nc->queue_index > 1) { > + g_free(queue); > + return; > + } > + /* Not support vhost-net */ > + if (get_vhost_net(nc)) { > + g_free(queue); > + return; > + } > + qov = qmp_output_visitor_new(); > + ov = qmp_output_get_visitor(qov); > + visit_start_struct(ov, &dummy, NULL, NULL, 0, &err); > + if (err) { > + goto out; > + } > + visit_type_str(ov, &nc->name, "netdev", &err); > + if (err) { > + goto out; > + } > + visit_type_str(ov, &queue, "queue", &err); > + if (err) { > + goto out; > + } > + visit_end_struct(ov, &err); > + if (err) { > + goto out; > + } > + obj = qmp_output_get_qobject(qov); > + g_assert(obj != NULL); > + qdict = qobject_to_qdict(obj); > + qmp_output_visitor_cleanup(qov); > + > + qiv = qmp_input_visitor_new(obj); > + iv = qmp_input_get_visitor(qiv); > + object_add(TYPE_FILTER_BUFFER, id, qdict, iv, &err); > + qmp_input_visitor_cleanup(qiv); > + qobject_decref(obj); > +out: > + g_free(queue); > + if (err) { > + error_propagate(errp, err); > + } > +} > + > static void filter_buffer_init(Object *obj) > { > object_property_add(obj, "interval", "int", > diff --git a/net/filter.c b/net/filter.c > index 1365bad..0b1e408 100644 > --- a/net/filter.c > +++ b/net/filter.c > @@ -163,7 +163,8 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) > } > > nf->netdev = ncs[0]; > - > + nf->is_default = false; > + nf->enabled = true; If we really need something like enabled, need more code for user to control this bit. > if (nfc->setup) { > nfc->setup(nf, &local_err); > if (local_err) { > @@ -190,6 +191,9 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) > g_free(info); > } > object_property_iter_free(iter); > + info = g_strdup_printf(",status=%s", nf->enabled ? "on" : "off"); > + g_strlcat(nf->info_str, info, sizeof(nf->info_str)); > + g_free(info); > } > > static void netfilter_finalize(Object *obj) > diff --git a/net/net.c b/net/net.c > index 87dd356..fd53cfc 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc, > NetFilterState *nf = NULL; > > QTAILQ_FOREACH(nf, &nc->filters, next) { > + /* Don't go through filter if it is off */ > + if (qemu_need_skip_netfilter(nf)) { > + continue; > + } Better move the above to qemu_netfilter_receive() > ret = qemu_netfilter_receive(nf, direction, sender, flags, iov, > iovcnt, sent_cb); > if (ret) { > @@ -1028,6 +1032,14 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) > } > return -1; > } > + > + if (is_netdev) { > + const Netdev *netdev = object; > + > + netdev_add_default_filter_buffer(netdev->id, > + NET_FILTER_DIRECTION_RX, > + errp); Several issues here: - Generic code should know nothing about a specific kind of netfilter. So the default filter should be pointed out by user instead of hard-coding it like this. - Need to deal with the netdev hot add. It should still has the default filter. > + } > return 0; > } >