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 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(-)
>>>>
>>>> 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;
>>>>    }
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
> .
>