From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLp37-0005J5-7y for qemu-devel@nongnu.org; Wed, 20 Jan 2016 04:27:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLp34-0005OS-0Z for qemu-devel@nongnu.org; Wed, 20 Jan 2016 04:27:33 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:47844) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLp33-0005O7-5m for qemu-devel@nongnu.org; Wed, 20 Jan 2016 04:27:29 -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> <569F33DE.2080809@huawei.com> <569F5017.6060803@redhat.com> From: Hailiang Zhang Message-ID: <569F52E8.2020606@huawei.com> Date: Wed, 20 Jan 2016 17:27:04 +0800 MIME-Version: 1.0 In-Reply-To: <569F5017.6060803@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 17:15, Jason Wang wrote: > > > On 01/20/2016 03:14 PM, Hailiang Zhang wrote: >> 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(-) >>>>>> > > [...] > >>>>>> +/* >>>>>> +* 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 ? :) > > I mean you need to know the type of default filter before calling > netdev_add_filter(). May need a global pointer for this, and colo may > set this during initialization. And in the future, we may allow this to > be set from cli. > Got it, i will fix it, thanks! >> >>>> >>>>>> + 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 > > It's ok. > > > . >