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?
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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
