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

[Jason Wang]

On 09/22/2015 03:49 PM, Yang Hongyang wrote:
> 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.

Maybe I miss something. But there's still refcnt for objects created in
previous iters? (I mean object_property_add_child() will add another
refcnt?)

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