From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeIKj-0002VW-HQ for qemu-devel@nongnu.org; Tue, 22 Sep 2015 03:49:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeIKg-0004Kz-81 for qemu-devel@nongnu.org; Tue, 22 Sep 2015 03:49:49 -0400 Received: from [59.151.112.132] (port=22771 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeIKc-0004K9-4k for qemu-devel@nongnu.org; Tue, 22 Sep 2015 03:49:46 -0400 Message-ID: <5601080F.9090305@cn.fujitsu.com> Date: Tue, 22 Sep 2015 15:49:35 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1442405768-23019-1-git-send-email-yanghy@cn.fujitsu.com> <1442405768-23019-13-git-send-email-yanghy@cn.fujitsu.com> <56010511.2040303@redhat.com> In-Reply-To: <56010511.2040303@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 12/12] netfilter: add multiqueue support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, armbru@redhat.com, Yang Hongyang , stefanha@redhat.com On 09/22/2015 03:36 PM, Jason Wang wrote: > > > On 09/16/2015 08:16 PM, Yang Hongyang wrote: >> From: Yang Hongyang >> >> 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 >> --- >> 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.