On 7/17/23 11:14, Eelco Chaudron wrote:
>
>
> 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, 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 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;
Hrm, the netdev-linux actually adds both parameters into the policer
configuration. But it seems like that will never actually work.
So, this bit of documentation is incorrect:
vswitch.xml:
Policing settings can be set with byte rate or packet rate, and they
can be configured together, in which case they take effect together,
that means the smaller speed limit of them is in effect.
>
>
>
>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
