Re: [Qemu-devel] [PATCH COLO-Frame v13 34/39] net/filter-buffer: Add default filter-buffer for each netdev

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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 <zhang.zhanghailiang@huawei.com>
>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>>>>
>>>>> 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.
>
>
> .
>