> -----Original Message----- > From: Daniele Di Proietto [mailto:diproiet...@vmware.com] > Sent: Tuesday, October 13, 2015 5:14 PM > To: Stokes, Ian > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] netdev_dpdk.c: Add QoS functionality. > > > > On 12/10/2015 18:02, "Stokes, Ian" <ian.sto...@intel.com> wrote: > > >Hi Daniele, > > > >Thanks for providing feedback, answers inline. > > > >> -----Original Message----- > >> From: Daniele Di Proietto [mailto:diproiet...@vmware.com] > >> Sent: Friday, October 09, 2015 6:53 PM > >> To: Stokes, Ian > >> Cc: dev@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.
Agreed. > >> * 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 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. > > Thanks, > > Daniele _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
