Hi Lin Huang, thanks for your patch.
On Sun, Apr 09, 2023 at 07:15:08PM +0800, [email protected] wrote: > From: Lin Huang <[email protected]> > > 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). > 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. > 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? > + } > } > > - 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. > + /* 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? 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? 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? > 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/ > + 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
