On 5/1/23 13:33, [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 ingress 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 reuse 'ingress_policing_kpkts_rate' and
> 'ingress_policing_kpkts_burst' options at interface table. Now userspace
> ingress policer supports setting packet-per-second limits in addition to
> the previously configurable byte rate settings.
>
> Examples:
> $ ovs-vsctl set interface dpdk0 ingress_policing_rate=12300
> $ ovs-vsctl set interface dpdk0 ingress_policing_burst=12300
> $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_rate=123
> $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_burst=123
>
> Add some unit tests for ingress packet-per-second policing.
>
> Signed-off-by: Lin Huang <[email protected]>
Thanks for the new version. Not a full review but a few comments inline.
Also, you may drop the 'userspace port-based' part from patch names.
'netdev-dpdk' area already means that it is userpsace and that it is
for ports.
> ---
> NEWS | 4 +
> lib/netdev-dpdk.c | 78 +++++++++++++---
> tests/system-dpdk.at | 216 ++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 285 insertions(+), 13 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c41c6adf6..53adbed82 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -30,6 +30,10 @@ Post-v3.1.0
> ovs-vsctl set port vhost-user0 qos=@newqos -- \
> --id=@newqos create qos type=egress-policer \
> other-config:kpkts_rate=123 other-config:kpkts_burst=123
> + * Added support for ingress packet-per-second policing.
> + Examples:
> + $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_rate=123
> + $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_burst=123
Examples are not really necessary in the NEWS file in most cases.
You may just say that the feature can be configured via
ingress_policing_kpkts_rate/burst columns in the Interface table.
>
>
> v3.1.0 - 16 Feb 2023
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index bcd13c116..a717edc5e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -413,6 +413,8 @@ struct ingress_policer {
> struct rte_meter_srtcm_params app_srtcm_params;
> struct rte_meter_srtcm in_policer;
> struct rte_meter_srtcm_profile in_prof;
> + struct token_bucket tb;
> + enum policer_type type;
> rte_spinlock_t policer_lock;
> };
>
> @@ -496,6 +498,9 @@ struct netdev_dpdk {
> uint32_t policer_rate;
> uint32_t policer_burst;
>
> + uint32_t policer_kpkts_rate;
> + uint32_t policer_kpkts_burst;
> +
> /* Array of vhost rxq states, see vring_state_changed. */
> bool *vhost_rxq_enabled;
> );
> @@ -1315,6 +1320,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> ovsrcu_init(&dev->ingress_policer, NULL);
> dev->policer_rate = 0;
> dev->policer_burst = 0;
> + dev->policer_kpkts_rate = 0;
> + dev->policer_kpkts_burst = 0;
>
> netdev->n_rxq = 0;
> netdev->n_txq = 0;
> @@ -2367,9 +2374,16 @@ ingress_policer_run(struct ingress_policer *policer,
> struct rte_mbuf **pkts,
> int cnt = 0;
>
> rte_spinlock_lock(&policer->policer_lock);
> - cnt = srtcm_policer_run_single_packet(&policer->in_policer,
> - &policer->in_prof,
> - pkts, pkt_cnt, should_steal);
> + if (policer->type & POLICER_BPS) {
> + cnt = srtcm_policer_run_single_packet(&policer->in_policer,
> + &policer->in_prof,
> + pkts, pkt_cnt, should_steal);
> + }
> +
> + if (policer->type & POLICER_PKTPS) {
> + cnt = pkts_policer_run_single_packet(&policer->tb, pkts, pkt_cnt,
> + should_steal);
> + }
We're running 2 policers here. If the POLICER_BPS drops some packets,
we can't use pkt_cnt as an argument for the POLICER_PKTPS, otherwise
we may get use-after-free.
> rte_spinlock_unlock(&policer->policer_lock);
>
> return cnt;
And if both policers drop some packets, this return value will also
be incorrect.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
