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

[zhanghailiang]

On 2015/11/5 17:19, Jason Wang wrote:
> 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.

Yes, and the packets will go through the default buffer filter before
the ones that added by users. We only control the default buffer filter.

> 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

Actually, for now, we only do step 1) in COLO's init process, and do step 3)
while exit COLO.

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

Agreed. We will not let the default buffer filter to buffer any packets, before
we go into COLO process.

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

Er, maybe i didn't describe clearly. I mean the solution of adding default 
filter
buffer will not break the ones that added by users. We only manage the default 
filter
buffer, but the delay time of the buffer filter that added by users will be 
change :
New delay time = Checkpoint period + configured delay time.

Thanks,
zhanghailiang

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