Re: [Qemu-devel] [PATCH v11 12/12] netfilter: add multiqueue support

[Yang Hongyang]

On 09/22/2015 03:36 PM, Jason Wang wrote:
> On 09/16/2015 08:16 PM, Yang Hongyang wrote:
> > From: Yang Hongyang <address@hidden>
> >
> > add multiqueue support, if there's multiqueue, we add multi netfilter
> > objects, other netfilter objects is the child of the first added netfilter
> > object. So when we delete a netfilter, the other netfilter objects we
> > added will be automatically deleted.
> >
> > Signed-off-by: Yang Hongyang <address@hidden>
> > ---
> > v11: initial patch
> > ---
> >   net/filter.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 79 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/filter.c b/net/filter.c
> > index aea619a..cc27528 100644
> > --- a/net/filter.c
> > +++ b/net/filter.c
> > @@ -142,16 +142,25 @@ static void netfilter_finalize(Object *obj)
> >       g_free(nf->name);
> >   }
> >
> > +static void proptb_free_val_func(gpointer data)
> > +{
> > +    g_free(data);
> > +}
> > +
> >   static void netfilter_complete(UserCreatable *uc, Error **errp)
> >   {
> > -    NetFilterState *nf = NETFILTER(uc);
> > +    NetFilterState *nf = NETFILTER(uc), *nfq = NULL;
> >       NetClientState *ncs[MAX_QUEUE_NUM];
> > -    NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
> > -    int queues;
> > +    NetFilterClass *nfc = NETFILTER_GET_CLASS(uc), *nfqc = NULL;
> > +    int queues, i;
> >       Error *local_err = NULL;
> > -    char *str, *info;
> > +    char *str, *info, *name;
> >       ObjectProperty *prop;
> >       StringOutputVisitor *ov;
> > +    Object *obj = NULL;
> > +    GHashTable *proptable = NULL;
> > +    GHashTableIter iter;
> > +    gpointer key, value;
> >
> >       if (!nf->netdev_id) {
> >           error_setg(errp, "Parameter 'netdev' is required");
> > @@ -165,9 +174,6 @@ static void netfilter_complete(UserCreatable *uc, Error 
> > **errp)
> >           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "netdev",
> >                      "a network backend id");
> >           return;
> > -    } else if (queues > 1) {
> > -        error_setg(errp, "Multi queue is not supported");
> > -        return;
> >       }
> >
> >       if (get_vhost_net(ncs[0])) {
> > @@ -187,6 +193,17 @@ static void netfilter_complete(UserCreatable *uc, Error 
> > **errp)
> >       }
> >       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> >
> > +    if (queues > 1) {
> > +        /*
> > +         * Store the properties of the filter except "type" property.
> > +         * When there's multiqueue, we will create a new filter object
> > +         * of the same type and same properties. this hashtable is used
> > +         * to set newly created object properties.
> > +         */
> > +        proptable = g_hash_table_new_full(NULL, NULL, NULL,
> > +                                          proptb_free_val_func);
> > +    }
>
> I'm thinking whether or not duplicate all the properties in each
> netfilters is a good method. Maybe we can have a another ojbect with
> array of pointers to NetFilter objects embedded? Another question is
> whether or not we need to do this at this level. Maybe we can make the
> necessary Netfilter multiqueue aware. E.g let buffer filter to have
> multiqueue also? Then you may only need a single timer?

netfilter_complete() only called once when we add a netfilter. So in this
func, we can create the same filter object if there's multiqueue.

> > +
> >       /* generate info str */
> >       QTAILQ_FOREACH(prop, &OBJECT(nf)->properties, node) {
> >           if (!strcmp(prop->name, "type")) {
> > @@ -199,9 +216,64 @@ static void netfilter_complete(UserCreatable *uc, Error 
> > **errp)
> >           string_output_visitor_cleanup(ov);
> >           info = g_strdup_printf(",%s=%s", prop->name, str);
> >           g_strlcat(nf->info_str, info, sizeof(nf->info_str));
> > +        if (queues > 1) {
> > +            g_hash_table_insert(proptable, prop->name, g_strdup(str));
> > +        }
> >           g_free(str);
> >           g_free(info);
> >       }
> > +
> > +    for (i = 1; i < queues; i++) {
> > +        obj = object_new(object_get_typename(OBJECT(nf)));
> > +        /* set filter properties */
> > +        nfq = NETFILTER(obj);
> > +        nfq->name = g_strdup(nf->name);
> > +        nfq->netdev = ncs[i];
> > +        snprintf(nfq->info_str, sizeof(nfq->info_str), "%s", nf->info_str);
> > +
> > +        g_hash_table_iter_init(&iter, proptable);
> > +        while (g_hash_table_iter_next(&iter, &key, &value)) {
> > +            object_property_parse(obj, (char *)value, (char *)key, &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                goto out;
> > +            }
> > +        }
> > +
> > +        /* add other queue's filter object as the first object's child */
> > +        name = g_strdup_printf("%s-queue-%d", nf->name, i);
> > +        object_property_add_child(OBJECT(nf), name, obj, &local_err);
> > +        g_free(name);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            goto out;
> > +        }
> > +
> > +        /* setup filter */
> > +        nfqc = NETFILTER_GET_CLASS(obj);
> > +        if (nfqc->setup) {
> > +            nfqc->setup(nfq, &local_err);
> > +            if (local_err) {
> > +                error_propagate(errp, local_err);
> > +                goto out;
> > +            }
> > +        }
> > +        QTAILQ_INSERT_TAIL(&nfq->netdev->filters, nfq, next);
> > +        object_unref(obj);
            ^^^^^^^^^^^^

> > +    }
> > +
> > +    if (proptable) {
> > +        g_hash_table_unref(proptable);
> > +    }
> > +    return;
> > +
> > +out:
>
> We may leak objects here.

Seems not. see above object_unref(), if we come to
here, object create in previous iter will be unrefed.

> > +    if (proptable) {
> > +        g_hash_table_unref(proptable);
> > +    }
> > +    if (obj) {
> > +        object_unref(obj);
> > +    }
> >   }
> >
> >   static void netfilter_class_init(ObjectClass *oc, void *data)
>
> To reduce the review iterations, I suggest to drop the patch also.
> Multiqueue support could be another series on top.

I agree, thank you.

> Thanks
>
>
> .

--
Thanks,
Yang.