From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46963) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLmz8-0005KP-4U for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:15:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLmz3-00012E-32 for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:15:18 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:8682) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLmz2-0000zI-1I for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:15:13 -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> <569DF649.5010601@huawei.com> <569EF351.2020608@redhat.com> From: Hailiang Zhang Message-ID: <569F33DE.2080809@huawei.com> Date: Wed, 20 Jan 2016 15:14:38 +0800 MIME-Version: 1.0 In-Reply-To: <569EF351.2020608@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, zhangchen.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, hongyang.yang@easystack.cn On 2016/1/20 10:39, Jason Wang wrote: > > > On 01/19/2016 04:39 PM, Hailiang Zhang wrote: >> 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) > > Aha, right. So rethinking of this, enabling/disabling a filter looks ok > to me. > >> >>> 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 ? > > net_client_init1() is ok, just need a generic way. E.g a string which > stores the default filter and its parameters which could be queried by > default filter initialization function. > Hmm, i'm still confused about how to realize this, do you mean change this helper to a more generic function ? Just like : void netdev_add_filter(const char *netdev_id, const char *filter_type, char *id, Error **errp) Could you please give me more detail about how to realize it ? :) >> >>>> + 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. > > Or if you want to minimize the change set. You can make something like > enabled locally to filter buffer, and let it to be used by colo only. > I have convert it to use object property to set/get the value, is that OK ? Thanks, Hailiang >> >>>> 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. :) > > Right. So the issue is just to have a way to let an arbitrary filter to > be default :). > > Thanks > >> >> Thanks, >> Hailiang >> >>>> + } >>>> return 0; >>>> } >>>> >>> >>> >>> . >>> >> >> >> > > > . >