Re: [Qemu-devel] [PATCH COLO-Frame v10 35/38] netfilter: Introduce a API to automatically add filter-buffer for each netdev

[Jason Wang]

On 11/05/2015 03:43 PM, zhanghailiang wrote:
> Hi Jason,
>
> On 2015/11/4 10:56, Jason Wang wrote:
>>
>>
>> On 11/03/2015 07:56 PM, zhanghailiang wrote:
>>> Signed-off-by: zhanghailiang <address@hidden>
>>> Cc: Jason Wang <address@hidden>
>>
>> Commit log please.
>>
>>> ---
>>> v10: new patch
>>> ---
>>>   include/net/filter.h |  1 +
>>>   include/net/net.h    |  3 ++
>>>   net/filter-buffer.c  | 84
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   net/net.c            | 20 +++++++++++++
>>>   4 files changed, 108 insertions(+)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index 4499d60..b0954ba 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -75,5 +75,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
>>> *sender,
>>>                                       void *opaque);
>>>   void filter_buffer_release_all(void);
>>>   void  filter_buffer_del_all_timers(void);
>>> +void qemu_auto_add_filter_buffer(NetFilterDirection direction,
>>> Error **errp);
>>>
>>>   #endif /* QEMU_NET_FILTER_H */
>>> diff --git a/include/net/net.h b/include/net/net.h
>>> index 5c65c45..e32bd90 100644
>>> --- a/include/net/net.h
>>> +++ b/include/net/net.h
>>> @@ -129,6 +129,9 @@ typedef void
>>> (*qemu_netfilter_foreach)(NetFilterState *nf, void *opaque,
>>>                                          Error **errp);
>>>   void qemu_foreach_netfilter(qemu_netfilter_foreach func, void
>>> *opaque,
>>>                               Error **errp);
>>> +typedef void (*qemu_netdev_foreach)(NetClientState *nc, void *opaque,
>>> +                                    Error **errp);
>>> +void qemu_foreach_netdev(qemu_netdev_foreach func, void *opaque,
>>> Error **errp);
>>>   int qemu_can_send_packet(NetClientState *nc);
>>>   ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec
>>> *iov,
>>>                             int iovcnt);
>>> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
>>> index 05313de..0dc1efb 100644
>>> --- a/net/filter-buffer.c
>>> +++ b/net/filter-buffer.c
>>> @@ -15,6 +15,11 @@
>>>   #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"
>>> +
>>>
>>>   #define TYPE_FILTER_BUFFER "filter-buffer"
>>>
>>> @@ -185,6 +190,85 @@ void filter_buffer_del_all_timers(void)
>>>       qemu_foreach_netfilter(filter_buffer_del_timer, NULL, NULL);
>>>   }
>>>
>>> +static void netdev_add_filter_buffer(NetClientState *nc, void *opaque,
>>> +                                     Error **errp)
>>> +{
>>> +    NetFilterState *nf;
>>> +    bool found = false;
>>> +
>>> +    QTAILQ_FOREACH(nf, &nc->filters, next) {
>>> +        if (!strcmp(object_get_typename(OBJECT(nf)),
>>> TYPE_FILTER_BUFFER)) {
>>> +            found = true;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (!found) {
>>> +        QmpOutputVisitor *qov;
>>> +        QmpInputVisitor *qiv;
>>> +        Visitor *ov, *iv;
>>> +        QObject *obj = NULL;
>>> +        QDict *qdict;
>>> +        void *dummy = NULL;
>>> +        char *id = g_strdup_printf("%s-%s.0", nc->name,
>>> TYPE_FILTER_BUFFER);
>>> +        char *queue = (char *) opaque;
>>> +        bool auto_add = true;
>>> +        Error *err = NULL;
>>> +
>>> +        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_type_bool(ov, &auto_add, "auto", &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(id);
>>> +        if (err) {
>>> +            error_propagate(errp, err);
>>> +        }
>>> +    }
>>> +}
>>> +/*
>>> +* This will be used by COLO or MC FT, for which they will need
>>> +* to buffer all the packets of all VM's net devices, Here we check
>>> +* and automatically add netfilter for netdev that doesn't attach
>>> any buffer
>>> +* netfilter.
>>> +*/
>>> +void qemu_auto_add_filter_buffer(NetFilterDirection direction,
>>> Error **errp)
>>> +{
>>> +    char *queue = g_strdup(NetFilterDirection_lookup[direction]);
>>> +
>>> +    qemu_foreach_netdev(netdev_add_filter_buffer, queue,
>>> +                                        errp);
>>> +    g_free(queue);
>>> +}
>>> +
>>
>> This make me think for following questions:
>>
>> - What if a nic is hot added after this "automatically" filter add?
>> - Maybe a better way is to have a default filter? It could be specified
>> through qemu cli or other (And default filter could be 'nop' which means
>> no filter) ?
>>
>
> I have thought about this. I'd like to add this default buffer filter
> quietly,
> not through qemu cli. In this way, we can still keep the buffer filter
> that configured by users,

Actually, this does not break the ones that added by user. We support
attach more than one filters to be attached to a single netdev.

If I understand the case correctly (I was only partially cced in this
series). Before each synchronization, you need:

1) add a buffer filter to each netdev
2) release all buffers on demand
3) delete all buffer filters

You can just remove step 1 if you know all device has a default buffer
filter. And step 3 could be also removed if you can let buffer filter
won't buffer any packet through a new command or other.

> and keep its delay release packets capability. Though the delay time
> is not what
> users suppose. (This is only happened in COLO's periodic mode, in
> normal colo mode, the delay time
> is almost same with user's configure.)

This is not good unless you want to limit the buffer filter only for
COLO. And I want also know the role of management: technically it can do
all the above 3 steps ( And looks like management was a better place to
do this).

Thanks

>
> What about call netdev_add_filter_buffer() in each netdev's init() ?
> I didn't found a common code path for every netdev in their init path.
>
> Thanks,
> zhanghailiang
>
>>>   static void filter_buffer_init(Object *obj)
>>>   {
>>>       object_property_add(obj, "interval", "int",
>>> diff --git a/net/net.c b/net/net.c
>>> index a333b01..4fbe0af 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -283,6 +283,26 @@ void
>>> qemu_foreach_netfilter(qemu_netfilter_foreach func, void *opaque,
>>>       }
>>>   }
>>>
>>> +void qemu_foreach_netdev(qemu_netdev_foreach func, void *opaque,
>>> Error **errp)
>>> +{
>>> +    NetClientState *nc;
>>> +
>>> +    QTAILQ_FOREACH(nc, &net_clients, next) {
>>> +        if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>>> +            continue;
>>> +        }
>>> +        if (func) {
>>> +            Error *local_err = NULL;
>>> +
>>> +            func(nc, opaque, &local_err);
>>> +            if (local_err) {
>>> +                error_propagate(errp, local_err);
>>> +                return;
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   static void qemu_net_client_destructor(NetClientState *nc)
>>>   {
>>>       g_free(nc);
>>
>>
>> .
>>
>
>
>