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 <bur...@gmail.com>
>>>
>>> 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 <bur...@gmail.com>
>>> ---
>>> 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
>>
>>
>> .
>>
>
