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

[Yang Hongyang <hongyang.yang@easystack.cn>]

On 2015年12月07日 15:38, Hailiang Zhang wrote:
> On 2015/12/3 15:21, Yang Hongyang wrote:
>>
>>
>> On 2015年12月03日 14:48, Hailiang Zhang wrote:
>>> On 2015/12/3 14:25, Wen Congyang wrote:
>>>> On 12/03/2015 11:53 AM, Hailiang Zhang wrote:
>>>>> On 2015/12/3 9:17, Wen Congyang wrote:
>>>>>> On 11/24/2015 05:25 PM, zhanghailiang wrote:
>>>>>>> We add each netdev 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.
>>>>>>
>>>>>> No, filter-buffer doesn't support vhost, so if you add default
>>>>>> filter-buffer
>>>>>> for each netdev, you can't use vhost.
>>>>>>
>>>>>
>>>>> Have you tested it ? Did the default filter-buffer break vhost ?
>>>>> It's not supposed to break vhost, I will look into it. Thanks.
>>>>
>>>> Yes, I have tested it. When I want to start a normal vm with vhost,
>>>> I get
>>>> the following error messages:
>>>>
>>>> qemu-system-x86_64: -netdev tap,id=hn0,queues=1,vhost=on: Vhost is not
>>>> supported
>>>>
>>>
>>> Hmm, that is reported by filter in nextfilter_complete(), i will
>>> investigate it.
>>> We hope the default buffer filter has no side effect on netdev.
>>
>> I think you'd better add an option to netdev to turn on/off default
>> filter, that will solve the problem.
>>
>
> Hi Hongyang,
>
> I have fix this problem in another way, just skip add filter for vhost-user
> netdev. And by the way, I didn't add any option to netdev to control
> the default filter's on/off, since i'd like to control the default filter
> just in filter layer, i add an 'enabled' and an 'is-default' flag in
> struct NetFilterState, and for the default filter buffer, its status is
> 'off' (disabled),
> so it will not buffer any packets, we will skip it in
> filter_receive_iov() to ensure
> the packets will not go through the process of filter.
>
> Besides, for hmp command 'info network', we will show the on/off status
> for filter, just
> like: (Or, maybe we don't show the filter that is disabled ?)
> (qemu) info network
> net-pci0: index=0,type=nic,model=virtio-net-pci,macaddr=52:54:00:12:34:56
>   \ bn0:
> index=0,type=tap,ifname=tap0,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
>
> filters:
>    - nop: type=filter-buffer,interval=0,netdev=bn0,queue=rx,status=off
>    - f0: type=filter-buffer,interval=1000,netdev=bn0,queue=rx,status=on
>
> Is this acceptable?

Hi Hailiang, thanks for the explanation.
It's up to Jason though, cause he's the network Maintainer :)

>
> Thanks.
> Hailiang
>
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>>>
>>>>>> Thanks
>>>>>> Wen Congyang
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>>>>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>>>>>>> ---
>>>>>>> v11:
>>>>>>> - New patch
>>>>>>> ---
>>>>>>>    include/net/filter.h |  3 +++
>>>>>>>    net/filter-buffer.c  | 74
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>    net/net.c            |  8 ++++++
>>>>>>>    3 files changed, 85 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>>>> index 2deda36..01a7e90 100644
>>>>>>> --- a/include/net/filter.h
>>>>>>> +++ b/include/net/filter.h
>>>>>>> @@ -74,4 +74,7 @@ ssize_t
>>>>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>>>>                                        int iovcnt,
>>>>>>>                                        void *opaque);
>>>>>>>
>>>>>>> +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..195af68 100644
>>>>>>> --- a/net/filter-buffer.c
>>>>>>> +++ b/net/filter-buffer.c
>>>>>>> @@ -14,6 +14,12 @@
>>>>>>>    #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"
>>>>>>>
>>>>>>>    #define TYPE_FILTER_BUFFER "filter-buffer"
>>>>>>>
>>>>>>> @@ -26,6 +32,8 @@ typedef struct FilterBufferState {
>>>>>>>        NetQueue *incoming_queue;
>>>>>>>        uint32_t interval;
>>>>>>>        QEMUTimer release_timer;
>>>>>>> +    bool is_default;
>>>>>>> +    bool enable_buffer;
>>>>>>>    } FilterBufferState;
>>>>>>>
>>>>>>>    static void filter_buffer_flush(NetFilterState *nf)
>>>>>>> @@ -65,6 +73,10 @@ static ssize_t
>>>>>>> filter_buffer_receive_iov(NetFilterState *nf,
>>>>>>>    {
>>>>>>>        FilterBufferState *s = FILTER_BUFFER(nf);
>>>>>>>
>>>>>>> +    /* Don't buffer any packets if the filter is not enabled */
>>>>>>> +    if (!s->enable_buffer) {
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>>        /*
>>>>>>>         * We return size when buffer a packet, the sender will take
>>>>>>> it as
>>>>>>>         * a already sent packet, so sent_cb should not be called
>>>>>>> later.
>>>>>>> @@ -102,6 +114,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 +127,7 @@ static void filter_buffer_setup(NetFilterState
>>>>>>> *nf, Error **errp)
>>>>>>>        }
>>>>>>>
>>>>>>>        s->incoming_queue =
>>>>>>> qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
>>>>>>> +    s->is_default = !strcmp(path, "nop");
>>>>>>>        if (s->interval) {
>>>>>>>            timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
>>>>>>>                          filter_buffer_release_timer, nf);
>>>>>>> @@ -163,6 +177,66 @@ 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)
>>>>>>> +{
>>>>>>> +    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) {
>>>>>>> +        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/net.c b/net/net.c
>>>>>>> index ade6051..b36d49f 100644
>>>>>>> --- a/net/net.c
>>>>>>> +++ b/net/net.c
>>>>>>> @@ -1028,6 +1028,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);
>>>>>>> +    }
>>>>>>>        return 0;
>>>>>>>    }
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>>
>>
>
>
>

-- 
Thanks,
Yang