From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLRpf-0008C5-Ev for qemu-devel@nongnu.org; Tue, 19 Jan 2016 03:40:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLRpc-0002Kt-4c for qemu-devel@nongnu.org; Tue, 19 Jan 2016 03:40:07 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:40664) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLRpb-0002KK-9b for qemu-devel@nongnu.org; Tue, 19 Jan 2016 03:40:04 -0500 References: <1451372975-5048-1-git-send-email-zhang.zhanghailiang@huawei.com> <1451372975-5048-35-git-send-email-zhang.zhanghailiang@huawei.com> <569DAB42.2010008@redhat.com> From: Hailiang Zhang Message-ID: <569DF649.5010601@huawei.com> Date: Tue, 19 Jan 2016 16:39:37 +0800 MIME-Version: 1.0 In-Reply-To: <569DAB42.2010008@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed 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: Jason Wang , 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 Hi Jason, Thanks for your review. On 2016/1/19 11:19, Jason Wang wrote: > > > 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. If we do like this, do we also need to _enable_ the buffer filter by add it dynamically instead of attaching the default filter ? Just like what we do in V10 ? (In that series, you think have a default filter may be better. The main reason for that is to support hot-add nic. Since we didn't support hot-add nic during COLO, it will be OK to add default filter dynamically) > 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. Good idea. > 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. > We call it in net_client_init1(), i don't find a better place to call it, what's your suggestion ? >> + 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. > OK, i will fix it. >> 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() > OK. >> 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. > OK, then, here just answer my above question, we still need to add the default filter, so we still need the ability to enable or disable a netfilter. :) Thanks, Hailiang >> + } >> return 0; >> } >> > > > . >