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

[Jason Wang]

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 <address@hidden>
>>>>> Cc: Jason Wang <address@hidden>
>>>>> Cc: Yang Hongyang <address@hidden>
>>>>
>>>> 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.

>
>>>
>>>>> +    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.