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. >> * 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
