On 15 Jul 2023, at 4:49, miter wrote:
> Thank you for your reply. > > On 7/14/2023 11:01 PM, Ilya Maximets wrote: >> On 7/14/23 14:20, Eelco Chaudron wrote: >>> >>> On 14 Jul 2023, at 13:52, Eelco Chaudron wrote: >>> >>>> On 1 Jul 2023, at 16:40, [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 doesn'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 maximum 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. >>>>> >>>>> 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 >>>> Hi Lin, >>>> >>>> I did not review the code, but I’m wondering why you decided to overload >>>> the existing egress-policer type which is byte-based with another >>>> packet-based option. This will allow two configuration options and only >>>> one being used. >>>> >>>> I would suggest creating a new policer type for this, so it’s more clear >>>> from the type what it does. For example type=pkt-policer, as we also have >>>> trtcm-policer. >>> I was looking at some old code, so I guess your goal is to do both at the >>> same time? Then it makes sense. >>> >>> I will do a proper review, as I do think you need a new revision based on >>> feedback on the previous patch and this: >>> >>>>> static int >>>>> egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int >>>>> pkt_cnt, >>>>> bool should_steal) >>>>> { >>>>> - int cnt = 0; >>>>> struct egress_policer *policer = >>>>> CONTAINER_OF(conf, struct egress_policer, qos_conf); >>>>> + int bps_drop = 0, pps_drop = 0; >>>>> + int cnt = 0; >>>>> >>>>> - cnt = srtcm_policer_run_single_packet(&policer->egress_meter, >>>>> - &policer->egress_prof, pkts, >>>>> - pkt_cnt); >>>>> + if (policer->type & POLICER_BPS) { >>>>> + bps_drop = >>>>> srtcm_policer_run_single_packet(&policer->egress_meter, >>>>> + >>>>> &policer->egress_prof, pkts, >>>>> + pkt_cnt); >>>>> + } >>>>> + >>> If packets are dropped by the above, should we still pass them through the >>> below? >>> Don’t think so, or am I missing something? >> I think, it was my suggestion to do it this way. I suppose, the is a >> question >> about semantics, i.e. what does it mean to apply both policers? Should they >> run sequentially or in parallel? >> >> The parallel execution may result in more drops, because both should agree >> to forward in order for packet to be forwarded. In a worst case scenario >> two policers may give opposite alternate results for each subsequent packet. >> e.g. >> 0 1 0 1 0 1 0 1 >> 1 0 1 0 1 0 1 0 >> In this case, even if both policers allow 4 packets to pass, all packets will >> be dropped. >> >> So, I guess, I was wrong suggesting that, sorry. We should process them in >> sequence, i.e. packets dropped by the first policer should not be passed >> through the second one. We should not see the egress rate higher than any >> of the configured rates this way, right? > Yes, packets need to sequential police by both policer. > 1. bps policer. > 2. pps policer. In my opinion, if we are trying to mimic the kernel’s behavior, we should still go for two different policers. From a user’s perspective trying to work out how traffic gets affected by two different policers (parallel or sequential) might be a nightmare, let alone trying to troubleshoot this. From a quick look at the kernel code, they only support one at a time: tcf_police_init() 147 if (tb[TCA_POLICE_PKTRATE64] && R_tab) { 148 NL_SET_ERR_MSG(extack, 149 "packet-per-second and byte-per-second rate limits not allowed in same action"); 150 err = -EINVAL; 151 goto failure; >> Freeing packets in bulk instead of one-by-one still probably beneficial >> performance-wise. >> >>> >>> Also if a packet is forwarded by srtcm, but dropped by the packet policer >>> should the tokens not be returned? >> This essentially the same question regarding parallel vs sequential >> execution. >> >> >>>>> @@ -544,6 +551,7 @@ OVS_DPDK_PRE_CHECK() >>>>> OVS_DPDK_START([--no-pci]) >>>>> >>>>> dnl Add userspace bridge and attach it to OVS and add egress policer >>>>> +AT_CHECK([ovs-appctl vlog/set netdev_dpdk:dbg]) >>>>> AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 >>>>> datapath_type=netdev]) >>>>> AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface >>>>> dpdkvhostuserclient0 type=dpdkvhostuserclient >>>>> options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], >>>>> [stderr]) >>>>> OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0 qos=@newqos -- >>>>> --id=@newqos create qos type=egress-policer other-config:cir=1250000]) >>>>> @@ -563,6 +571,7 @@ AT_CHECK([ovs-vsctl del-port br10 >>>>> dpdkvhostuserclient0], [], [stdout], [stderr]) >>>>> OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ >>>>> \@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No >>>>> such file or directory@d >>>>> \@Could not create rte meter for egress policer@d >>>>> +\@Could not create token bucket for egress policer@d >>>>> \@Failed to set QoS type egress-policer on port dpdkvhostuserclient0: >>>>> Invalid argument@d >>>>> ])") >>>>> AT_CLEANUP >>>>> @@ -570,6 +579,189 @@ dnl >>>>> -------------------------------------------------------------------------- >>>>> >>>>> >>>>> >>> This is only testing the configuration, which can probably also be done in >>> the userspace tests, >> We need dpdk ports for that and dpdk requires some extra configuration to >> be able to run. >> >>> however it would be nice if we can add some actual traffic tests. >> I agree that it would be nice to have actual traffic tests, it is tricky >> though, because they are time-sensitive. We may try to use time warping >> since time_msec() is used for this particular type of the policing, >> but I'm not sure how other parts of DPDK will react to that. > I test the patch by hand. > It's hard to write a autotest scrpt to test the code. maybe i need to write > some new > autotest macro. >> Best regards, Ilya Maximets. > > -- > Best regards, Huang Lin. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
