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

[Jason Wang]

On 09/22/2015 04:43 PM, Yang Hongyang wrote:
> On 09/22/2015 04:32 PM, Jason Wang wrote:
>>
>>
>> On 09/22/2015 04:07 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?
>>>
>>> Sorry I don't get it at first. I also thought about make the buffer
>>> filter to
>>> have multiqueue, but there comes problem, how to distinguish which
>>> queue
>>> we should go in when receive the packet, we need to add a mechanism to
>>> distinguish which queue belongs to which net client's queue, that will
>>> be more complex, while current multiqueue implementation of net clients
>>> is multiple net clients with the same name, current solution is the
>>> simplest
>>> solution I can think of...
>>
>> I'm not sure I get this. But there's a queue_index filed in each
>> NetClientState which may help in this case.
>
> Ah, I see, thank you. There's another reason I do this in filter abstract
> layer is that the concrete filter implementation do not need to deal
> with the
> multiqueue, will simplify the filter implement a lot, otherwise, every
> filter need to implement it's own multiqueue support.

Probably not, just pointing all queues to a single netfilter object may
be sufficient (e.g for dump). And it's much easier for some kind of
filter (e.g traffic throttling) if it was multiqueue aware.