[ovs-dev] [PATCH] netdev_dpdk.c: Add QoS functionality.

Daniele Di Proietto diproiettod at vmware.com
Tue Oct 13 16:14:27 UTC 2015



On 12/10/2015 18:02, "Stokes, Ian" <ian.stokes at intel.com> wrote:

>Hi Daniele,
>
>Thanks for providing feedback, answers inline.
>
>> -----Original Message-----
>> From: Daniele Di Proietto [mailto:diproiettod at vmware.com]
>> Sent: Friday, October 09, 2015 6:53 PM
>> To: Stokes, Ian
>> Cc: dev at openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH] netdev_dpdk.c: Add QoS functionality.
>> 
>> Hi,
>> 
>> Thanks for the patch, the implementation looks simpler than I expected.
>> 
>> General questions:
>> 
>> * It appears that we're using the srCTM, but
>>   * We're only sending the green packets and dropping the rest.
>>   * The `input_color` is always green.
>> 
>>   Therefore
>>   * `other-config:ebs` is ignored (isn't it?). I would rather not put in
>> the
>>     database a value that is ignored.
>
>Although we do not use the ebs value it is a requirement for the
>rte_meter initialization function. DPDK expects an ebs value
>greater than 0 or it fails to initialize the rte_meter.
>
>We can remove the other_config:ebs from the command line but
>will have to add a define in netdev_dpdk so as to supply some ebs
>value.
>
>Would this be acceptable?

If there aren't plans to use different input/output colors, that
would be fine for me.

Since that parameter is useless, I would even avoid a define and always
set it
to 0.

>>   * The `us_policer_table` seems redundant
>>   * We just need two `us_policer_action`s, or perhaps just a boolean.
>> 
>>   Can we just call this a token bucket? Are there any good reasons not
>> to?
>>   (Maybe you're planning to expand this, I'm not sure)
>
>Is this in relation to the name 'us-policer'or the name for the cbs and
>ebs paramaters? ?

The name is fine.  My point is:

This patch implements a token bucket, using (internally) srTCM.  Is it
necessary
to expose to the user the `ebs` parameter, or references to RFC2697, since
we're
not using the colors at all?

>
>I think we should change the us-policer name anyhow to signify that it
>is egress. Would 'egress_policer' be acceptable? Failing that we ca use
>token_bucket can be used. Once it's clear to the end user.
>
>> 
>> * Out of curiosity, are there plans to support different output queues?
>
>There are plans to introduce user space queues at a later date to allow
>buffering of traffic. This would be included as maybe a rate limiting type
>implementation where traffic is not dropped but instead buffered. This
>will be a separate submission in the future however.

Sorry, I wasn't clear enough.  By `different output queues` I meant having
different instances of a meter on the same port (that can be configured
using
the `Queue` table, and can be used via the `enqueue` OpenFlow action).

> 
>> 
>> * I did't experience any performance degradation when QoS is not
>> configured,
>>   but one can imagine that taking a spinlock might have some performance
>> impact
>>   when multiple threads insist on the same NIC. Have you considered
>> using
>> RCU
>>   for the `qos_conf` pointer? (I'm not saying that we absolutely should
>> at
>> this
>>   point)
>> 
>I haven't tried an RCU for this. I can look at this but if it's not a
>blocking issue perhaps it could be implemented at a later stage?

Absolutely, we can implement that later, if we find a bottleneck.

Thanks,

Daniele




More information about the dev mailing list