On Wed, May 10, 2023 at 11:43:31PM +0800, miter wrote:
> Hi simon,
>
>
> Thanks for your reviewing.
>
>
> On 5/10/2023 10:34 PM, Simon Horman wrote:
> > On Mon, May 01, 2023 at 07:33:38PM +0800, [email protected] wrote:
> > > From: Lin Huang <[email protected]>
...
> > > 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
> > If I understand things correctly, for BPS rate and burst
> > are set by 'cir' and 'cbs'. While for PPS they are set
> > by 'kpkts_rate' and 'kpkts_burst'.
> >
> > I wonder if we could make the BPS and PPS configuration more symmetric.
>
> Yes, you are right.
>
> "rate" or "burst" ?
>
> What do you think ?
I'm not sure, naming things is hard.
'cir' and 'cbs' seem non-intuitive to me, but they are part of the UAPI,
and perhaps they are meaningful to others.
So perhaps one way is to have:
A. BPS: cir, cbs (current)
B. PPS: kpkts_cir, kpkts_cbs
Or perhaps, looking at the parameters for ingress, we could.
1. Keep, for BBS: cir, cbs
2. Add aliases for BBS: rate, burst
3. Add for PPS: kpkts_rate, kpkts_burst
Or we could go wild and have 1, 2, 3, and B.
But none of these ideas seem particularly awesome to me.
...
> > > @@ -4776,26 +4805,53 @@ 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 = xzalloc(sizeof *policer);
> > > + if (!policer) {
> > xzmalloc never returns NULL - it abort()s on error.
> >
> > > + return ENOMEM;
> > > + }
> > > +
> > > 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,
> > > - &policer->app_srtcm_params);
> > > + &policer->app_srtcm_params);
> > The line above was correctly indented, now it is not.
> >
> > > if (!err) {
> > > err = rte_meter_srtcm_config(&policer->egress_meter,
> > > - &policer->egress_prof);
> > > + &policer->egress_prof);
> > Ditto.
> Applied. Thanks a lot.
> > > + }
> > > + if (err) {
> > > + VLOG_ERR("Could not create rte meter for egress policer");
> > This function may or may not return an error for this condition.
> > So I don't think it is right to log an error (always).
> > Perhaps a debug message is appropriate.
> > Or an error only if the function returns an error.
>
> Emmm,
>
> I think it's appropriate for us to debug a message (VLOG_DBG).
Sure, I think that would be fine.
Likewise for any similar logging for PPS.
> > > + err = -err;
> > > + } else {
> > > + policer->type |= POLICER_BPS;
> > > }
> > > - if (!err) {
> > > - *conf = &policer->qos_conf;
> > > + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0);
> > > + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0);
> > I think this configuration should be read in
> > trtcm_policer_details_to_param(), which is where the configuration for
> > BPS rate limiting is read.
>
> trtcm_policer_details_to_param() is only for trtcm policer or bps policer.
>
> I think we need to rewirte a new function to read both parameters.
Yes, agreed.
> > > + if (kpkts_rate > MAX_KPKTS_PARAMETER || kpkts_burst >
> > > MAX_KPKTS_PARAMETER
> > > + || kpkts_rate == 0 || kpkts_burst == 0) {
> > > + /* Paramters between (1 ~ MAX_KPKTS_PARAMETER). */
> > s/Paramters/Parameters/
> >
> > > + err = EINVAL;
> > > + VLOG_ERR("Could not create tocken bucket for egress policer");
> > s/tocken/token/
> >
> > 1. I think out of range values should be rejected elsewhere,
> > perhaps in trtcm_policer_details_to_param().
> > 2. I think 0 probably means not configured, which is not n error condition.
> Applied. Thanks a lot.
> > > } else {
> > > - VLOG_ERR("Could not create rte meter for egress policer");
> > > + /* 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.*/
> > s/contrsruct/construction/
> >
> > > free(policer);
> > > *conf = NULL;
> > > - err = -err;
> > > + } else {
> > > + err = 0;
> > > + *conf = &policer->qos_conf;
> > > }
> > > return err;
...
> > > static int
> > > egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int
> > > pkt_cnt,
> > > bool should_steal)
> > > {
> > > - int cnt = 0;
> > Using pkt_cnt in place of cnt doesn't seem strictly related to this patch.
> >
> > > struct egress_policer *policer =
> > > CONTAINER_OF(conf, struct egress_policer, qos_conf);
> > > - cnt = srtcm_policer_run_single_packet(&policer->egress_meter,
> > > - &policer->egress_prof, pkts,
> > > - pkt_cnt, should_steal);
> > > + if (policer->type & POLICER_BPS) {
> > > + pkt_cnt = srtcm_policer_run_single_packet(&policer->egress_meter,
> > > + &policer->egress_prof,
> > > pkts,
> > > + pkt_cnt, should_steal);
> > > + }
> > > - return cnt;
> > > + if (policer->type & POLICER_PKTPS) {
> > > + pkt_cnt = pkts_policer_run_single_packet(&policer->egress_tb,
> > > pkts,
> > > + pkt_cnt, should_steal);
> > > + }
> > If both BPS and PPS are configured then both
> > srtcm_policer_run_single_packet() and pkts_policer_run_single_packet()
> > are run. But:
> >
> > 1. The pkt_cnt input to the latter is the output of the former.
> > 2. Only the pkt_cnt of the latter is returned.
> >
> > Is this correct?
> Yes, packets should check by both policer. and return the latter pkt_cnt
> value.
Thanks for confirming.
> > > +
> > > + return pkt_cnt;
> > > }
> > > static const struct dpdk_qos_ops egress_policer_ops = {
> > ...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
