[ovs-dev] [PATCH] netdev_dpdk.c: Add QoS functionality.
ian.stokes at intel.com
Wed Oct 14 15:22:33 UTC 2015
> -----Original Message-----
> From: Daniele Di Proietto [mailto:diproiettod at vmware.com]
> Sent: Tuesday, October 13, 2015 5:14 PM
> To: Stokes, Ian
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev_dpdk.c: Add QoS functionality.
> 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
> >> 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
> >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 haven't looked at this yet, although there is potential for work around this.
I had a similar thought to how multiple pmds could work with QoS? i.e. could there be a situation where to avoid the spin lock you could have a QoS conf per configured pmd (in this case there would be a meter per pmd). There's definitely some work to be done here in the future.
> >> * 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.
More information about the dev