Hi Simon,
Thanks for your reviewing.
Sorry for the late reply.
I will update a new patch after some modification.
On 4/20/2023 4:52 PM, Simon Horman wrote:
Hi Lin Huang,
thanks for your patch.
On Sun, Apr 09, 2023 at 07:15:08PM +0800,mit...@outlook.com wrote:
From: Lin Huang<linhu...@ruijie.com.cn>
OvS has supported packet-per-second policer which can be set at ingress and
egress
side in kernel datapath. But the userspace datapath dosen't support for ingress
and
egress packet-per-second policing now.
So, this patch add support for userspace egress pps policing by using native ovs
token bucket library. Token bucket is accumulated by 'rate' tokens per
millisecond
and store maxiumim tokens at 'burst' bucket size. One token in the bucket means
one packet (1 kpkts * millisecond) which will drop or pass by policer.
This patch add new configuration option 'kpkts_rate' and 'kpkts_burst' for
egress-policer QoS type which now supports setting packet-per-second limits in
addition to the previously configurable byte rate settings.
nit: Please keep the width of the patch description narrower
(I think 76 characters wide or less).
Applied, thanks.
Examples:
$ovs-vsctl set port vhost-user0 qos=@newqos --
--id=@newqos create qos type=egress-policer \
other-config:cir=123000 other-config:cbs=123000
other-config:kpkts_rate=123 other-config:kpkts_burst=123
Add some unit tests for egress packet-per-second policing.
...
@@ -4757,6 +4787,10 @@ netdev_dpdk_queue_dump_done(const struct netdev *netdev
OVS_UNUSED,
struct egress_policer {
struct qos_conf qos_conf;
+ enum policer_type type;
+ uint32_t kpkts_rate;
+ uint32_t kpkts_burst;
+ struct token_bucket egress_tb;
struct rte_meter_srtcm_params app_srtcm_params;
struct rte_meter_srtcm egress_meter;
struct rte_meter_srtcm_profile egress_prof;
@@ -4776,10 +4810,11 @@ static int
egress_policer_qos_construct(const struct smap *details,
struct qos_conf **conf)
{
+ uint32_t kpkts_burst, kpkts_rate;
struct egress_policer *policer;
int err = 0;
- policer = xmalloc(sizeof *policer);
+ policer = xcalloc(1, sizeof *policer);
I'm not sure of the motivation for this change.
But if it is to zero the allocation then perhaps xzalloc() is more appropriate.
Applied, thanks.
qos_conf_init(&policer->qos_conf, &egress_policer_ops);
egress_policer_details_to_param(details, &policer->app_srtcm_params);
err = rte_meter_srtcm_profile_config(&policer->egress_prof,
@@ -4787,15 +4822,32 @@ egress_policer_qos_construct(const struct smap *details,
if (!err) {
err = rte_meter_srtcm_config(&policer->egress_meter,
&policer->egress_prof);
+ if (!err) {
+ policer->type |= POLICER_BPS;
+ } else {
+ VLOG_ERR("Could not create rte meter for egress policer");
I have a few questions about this:
1. Prior to this patch the error message above was logged if
either call to rte_meter_srtcm_config failed. Now it
is only logged if the first call succeeds and the second
one fails. Is that intended?
2. Prior to this patch the err, returned by a failed called to
rte_meter_srtcm_config was returned. Now EINVAL is returned
if either call to rte_meter_srtcm_config() fails, is that intended?
Emmm. It seems to be a problem.
+ }
}
- if (!err) {
- *conf = &policer->qos_conf;
- } else {
- VLOG_ERR("Could not create rte meter for egress policer");
+ kpkts_rate = smap_get_uint(details, "kpkts_rate", 0);
+ kpkts_burst = smap_get_uint(details, "kpkts_burst", 0);
+ if (kpkts_rate && kpkts_burst) {
3. Here we configure pps if both parameters are non zero.
But above bps is configured (using rte_meter_srtcm_config())
unconditionally. This asymmetry strikes me as odd.
I just want to keep the bps code it is.
If kpkts_rate is zero, there is no need to alloc and set configure to
tocken bucket.
You are right. There are reasons to keep both of the same.
+ /* Rate in kilo-packets/second, bucket 1000 packets. */
+ /* msec * kilo-packets/sec = 1 packets. */
+ token_bucket_set(&policer->egress_tb, kpkts_rate, kpkts_burst * 1000);
+ policer->kpkts_burst = kpkts_burst;
+ policer->kpkts_rate = kpkts_rate;
+ policer->type |= POLICER_PKTPS;
+ }
+
+ if (!policer->type) {
+ /* both bps and kpkts contrsruct failed.*/
4. If a) BPS configuration fails and b) PPS is not configured
then this condition is reached, which is treated as an error.
But is b) an error? And if b) is true does it make a) not an error?
Token_bucket_setket() is a process never ruten error code.
It seems to me that there are four non error states.
a) Neither BPS nor PPS are configured
b) Only BPS is configured (we detect errors instantiating it)
c) Only PPS is configured.
d) Both BPS and PPS are configured.
I assume that egress_policer_qos_construct is called for b, c and d.
But is it also called for a?
egress_policer_qos_construct() called when BPS and PPS parameter are
different to the old one.
So, when neither BPS nor PPS are configured,
egress_policer_qos_construct() will not be called.
How do we detect the absence of BPS vs errors configuring BPS?
Likewise, how do we detect errors configuring PPS, or is that
not a concern?
It's a not a concen.
Setting parameters to PPS policer will never return a error code, unless
policer pointer is NULL.
So we only need to check the bps error.
free(policer);
*conf = NULL;
- err = -err;
+ err = EINVAL;
+ } else {
+ err = 0;
+ *conf = &policer->qos_conf;
}
...
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index edb5eafa0..93c387e2a 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -4894,6 +4894,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
bytes/tokens of the packet. If there are not enough tokens in the cbs
bucket the packet might be dropped.
</column>
+ <column name="other_config" key="kpkts_rate"
+ type='{"type": "integer", "minInteger": 0, "maxInteger":
4294967}'>
+ The Packets Per Second (pps) represents the packet per second rate at
nit: s/pps/PPS/
Applied, thanks.
+ which the token bucket will be updated.
+ </column>
+ <column name="other_config" key="kpkts_burst"
+ type='{"type": "integer", "minInteger": 0, "maxInteger":
4294967}'>
+ The Packets Per Second Burst Size is measured in count and represents a
+ token bucket.
+ </column>
</group>
<group title="Configuration for linux-sfq">
--
2.31.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
