diff mbox series

[ovs-dev,v6,3/4] netdev-dpdk: Add support for egress packet-per-second policing.

Message ID SY4PR01MB843892DB1C2EDF76B230F18FCD2BA@SY4PR01MB8438.ausprd01.prod.outlook.com
State Changes Requested
Headers show
Series netdev-dpdk: Add support for userspace port-based packet-per-second policing. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

miter July 1, 2023, 2:40 p.m. UTC
From: Lin Huang <linhuang@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

Add some unit tests for egress packet-per-second policing.

Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
---
 Documentation/topics/dpdk/qos.rst |  15 ++-
 NEWS                              |   2 +
 lib/netdev-dpdk.c                 | 132 ++++++++++++++++----
 tests/system-dpdk.at              | 194 +++++++++++++++++++++++++++++-
 vswitchd/vswitch.xml              |  10 ++
 5 files changed, 327 insertions(+), 26 deletions(-)

Comments

Eelco Chaudron July 14, 2023, 11:52 a.m. UTC | #1
On 1 Jul 2023, at 16:40, miterv@outlook.com wrote:

> From: Lin Huang <linhuang@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.

Cheers,

Eelco

>
> Add some unit tests for egress packet-per-second policing.
>
> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
> ---
>  Documentation/topics/dpdk/qos.rst |  15 ++-
>  NEWS                              |   2 +
>  lib/netdev-dpdk.c                 | 132 ++++++++++++++++----
>  tests/system-dpdk.at              | 194 +++++++++++++++++++++++++++++-
>  vswitchd/vswitch.xml              |  10 ++
>  5 files changed, 327 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst
> index a98ec672f..5f0b1469a 100644
> --- a/Documentation/topics/dpdk/qos.rst
> +++ b/Documentation/topics/dpdk/qos.rst
> @@ -36,14 +36,21 @@ QoS (Egress Policing)
>  Single Queue Policer
>  ~~~~~~~~~~~~~~~~~~~~
>
> -Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic
> -consisting of packets of size 64 bytes, the following command would limit the
> -egress transmission rate of the port to ~1,000,000 packets per second::
> +Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic,
> +the following command would limit the egress transmission rate of the port to
> +~1,000,000 bytes per second:
>
>      $ ovs-vsctl set port vhost-user0 qos=@newqos -- \
> -        --id=@newqos create qos type=egress-policer other-config:cir=46000000 \
> +        --id=@newqos create qos type=egress-policer other-config:cir=1000000 \
>          other-config:cbs=2048`
>
> +or, the following command would limit the egress transmission rate of the port
> +to ~1,000,000 packets per second:
> +
> +       ovs-vsctl set port vhost-user0 qos=@newqos -- \
> +          --id=@newqos create qos type=egress-policer \
> +          other-config:kpkts_rate=1000 other-config:kpkts_burst=1000
> +
>  To examine the QoS configuration of the port, run::
>
>      $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0
> diff --git a/NEWS b/NEWS
> index 0b5dc3db1..d88f490b1 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -44,6 +44,8 @@ Post-v3.1.0
>       * IP and L4 checksum offload support is now enabled by default for
>         interfaces that support it.  See the 'status' column in the 'interface'
>         table to check the status.
> +     * Added new configuration options 'kpkts_rate' and 'kpkts_burst' for
> +      'egress-policer' to support kilo packet-per-second policing.
>
>
>  v3.1.0 - 16 Feb 2023
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 2f7f74395..f97153665 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -19,6 +19,7 @@
>
>  #include <errno.h>
>  #include <signal.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> @@ -59,6 +60,7 @@
>  #include "openvswitch/ofp-parse.h"
>  #include "openvswitch/ofp-print.h"
>  #include "openvswitch/shash.h"
> +#include "openvswitch/token-bucket.h"
>  #include "openvswitch/vlog.h"
>  #include "ovs-numa.h"
>  #include "ovs-rcu.h"
> @@ -91,6 +93,8 @@ static bool per_port_memory = false; /* Status of per port memory support */
>  #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
>  #define OVS_VPORT_DPDK "ovs_dpdk"
>
> +#define MAX_KPKTS_PARAMETER 4294967U  /* UINT32_MAX / 1000 */
> +
>  /*
>   * need to reserve tons of extra space in the mbufs so we can align the
>   * DMA addresses to 4KB.
> @@ -400,6 +404,11 @@ struct dpdk_tx_queue {
>      );
>  };
>
> +enum policer_type {
> +    POLICER_BPS   = 1 << 0,   /* Rate value in bytes/sec. */
> +    POLICER_PKTPS = 1 << 1,   /* Rate value in packet/sec. */
> +};
> +
>  struct ingress_policer {
>      struct rte_meter_srtcm_params app_srtcm_params;
>      struct rte_meter_srtcm in_policer;
> @@ -2418,6 +2427,46 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter,
>      return cnt;
>  }
>
> +static int
> +pkts_policer_run_single_packet(struct token_bucket *tb, int pkt_cnt)
> +{
> +    int cnt = 0;
> +    long long int now = time_msec();
> +
> +    for (int i = 0; i < pkt_cnt; i++) {
> +        /* Handle current packet. */
> +        if (!token_bucket_withdraw(tb, 1, now)) {
> +            /* Count dropped packets. */
> +            cnt++;
> +        }
> +    }
> +
> +    return cnt;
> +}
> +
> +static int
> +pkts_policer_profile_config(struct token_bucket *tb,
> +                            uint32_t kpkts_rate, uint32_t kpkts_burst)
> +{
> +    if (kpkts_rate > MAX_KPKTS_PARAMETER ||
> +        kpkts_burst > MAX_KPKTS_PARAMETER) {
> +        return EINVAL;
> +    }
> +
> +   /* Rate in kilo-packets/second, bucket 1000 packets.
> +    * msec * kilo-packets/sec = 1 packets. */
> +    if (kpkts_rate) {
> +        /* Parameters between (1 ~ MAX_KPKTS_PARAMETER). */
> +        token_bucket_init(tb, kpkts_rate, kpkts_burst * 1000);
> +    } else {
> +        /* Zero means not to police the traffic.
> +         * Return a error to not set POLICER_PKTPS valid. */
> +        return EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf **pkts,
>                      int pkt_cnt, bool should_steal)
> @@ -4882,24 +4931,37 @@ netdev_dpdk_queue_dump_done(const struct netdev *netdev OVS_UNUSED,
>
>
>  /* egress-policer details */
> +struct egress_policer_params {
> +    struct rte_meter_srtcm_params app_srtcm_params;
> +    uint32_t kpkts_rate;
> +    uint32_t kpkts_burst;
> +};
>
>  struct egress_policer {
>      struct qos_conf qos_conf;
> -    struct rte_meter_srtcm_params app_srtcm_params;
> +    enum policer_type type;
> +    struct token_bucket egress_tb;
> +    struct egress_policer_params params;
>      struct rte_meter_srtcm egress_meter;
>      struct rte_meter_srtcm_profile egress_prof;
>  };
>
>  static void
>  egress_policer_details_to_param(const struct smap *details,
> -                                struct rte_meter_srtcm_params *params)
> +                                struct egress_policer_params *params)
>  {
> -    memset(params, 0, sizeof *params);
> -    params->cir = smap_get_ullong(details, "cir", 0);
> -    params->cbs = smap_get_ullong(details, "cbs", 0);
> -    params->ebs = 0;
> +    struct rte_meter_srtcm_params *srtcm_params = &params->app_srtcm_params;
> +
> +    memset(srtcm_params, 0, sizeof *srtcm_params);
> +    srtcm_params->cir = smap_get_ullong(details, "cir", 0);
> +    srtcm_params->cbs = smap_get_ullong(details, "cbs", 0);
> +    srtcm_params->ebs = 0;
> +
> +    params->kpkts_rate  = smap_get_uint(details, "kpkts_rate", 0);
> +    params->kpkts_burst = smap_get_uint(details, "kpkts_burst", 0);
>  }
>
> +
>  static int
>  egress_policer_qos_construct(const struct smap *details,
>                               struct qos_conf **conf)
> @@ -4907,23 +4969,40 @@ egress_policer_qos_construct(const struct smap *details,
>      struct egress_policer *policer;
>      int err = 0;
>
> -    policer = xmalloc(sizeof *policer);
> +    policer = xzalloc(sizeof *policer);
>      qos_conf_init(&policer->qos_conf, &egress_policer_ops);
> -    egress_policer_details_to_param(details, &policer->app_srtcm_params);
> +    egress_policer_details_to_param(details, &policer->params);
> +
>      err = rte_meter_srtcm_profile_config(&policer->egress_prof,
> -                                         &policer->app_srtcm_params);
> +                                         &policer->params.app_srtcm_params);
>      if (!err) {
>          err = rte_meter_srtcm_config(&policer->egress_meter,
>                                       &policer->egress_prof);
>      }
> +    if (err) {
> +        VLOG_DBG("Could not create rte meter for egress policer");
> +        err = -err;
> +    } else {
> +        policer->type |= POLICER_BPS;
> +    }
>
> -    if (!err) {
> -        *conf = &policer->qos_conf;
> +    err = pkts_policer_profile_config(&policer->egress_tb,
> +                                      policer->params.kpkts_rate,
> +                                      policer->params.kpkts_burst);
> +    if (err) {
> +        VLOG_DBG("Could not create token bucket for egress policer");
>      } else {
> -        VLOG_ERR("Could not create rte meter for egress policer");
> +        policer->type |= POLICER_PKTPS;
> +    }
> +
> +    if (!policer->type) {
> +        /* Both bps and kpkts contrsruction failed.*/
>          free(policer);
>          *conf = NULL;
> -        err = -err;
> +        VLOG_INFO("Could not create qos for egress policer");
> +    } else {
> +        err = 0;
> +        *conf = &policer->qos_conf;
>      }
>
>      return err;
> @@ -4942,9 +5021,12 @@ egress_policer_qos_get(const struct qos_conf *conf, struct smap *details)
>  {
>      struct egress_policer *policer =
>          CONTAINER_OF(conf, struct egress_policer, qos_conf);
> +    struct egress_policer_params *params = &policer->params;
>
> -    smap_add_format(details, "cir", "%"PRIu64, policer->app_srtcm_params.cir);
> -    smap_add_format(details, "cbs", "%"PRIu64, policer->app_srtcm_params.cbs);
> +    smap_add_format(details, "cir", "%"PRIu64, params->app_srtcm_params.cir);
> +    smap_add_format(details, "cbs", "%"PRIu64, params->app_srtcm_params.cbs);
> +    smap_add_format(details, "kpkts_rate", "%"PRIu32, params->kpkts_rate);
> +    smap_add_format(details, "kpkts_burst", "%"PRIu32, params->kpkts_burst);
>
>      return 0;
>  }
> @@ -4955,25 +5037,33 @@ egress_policer_qos_is_equal(const struct qos_conf *conf,
>  {
>      struct egress_policer *policer =
>          CONTAINER_OF(conf, struct egress_policer, qos_conf);
> -    struct rte_meter_srtcm_params params;
> +    struct egress_policer_params params;
>
>      egress_policer_details_to_param(details, &params);
>
> -    return !memcmp(&params, &policer->app_srtcm_params, sizeof params);
> +    return !memcmp(&params, &policer->params, sizeof params);
>  }
>
>  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 (policer->type & POLICER_PKTPS) {
> +        pps_drop = pkts_policer_run_single_packet(&policer->egress_tb, pkt_cnt);
> +    }
>
> +    cnt = MAX(bps_drop, pps_drop);
>      if (should_steal && cnt) {
>          rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
>      }
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 0f58e8574..233f7e493 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -435,6 +435,7 @@ OVS_DPDK_PRE_PHY_SKIP()
>  OVS_DPDK_START()
>
>  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 phy0 -- set Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
>  OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048])
> @@ -453,7 +454,9 @@ AT_CHECK([grep -E 'QoS not configured on phy0' stdout], [], [stdout])
>
>  dnl Clean up
>  AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@Could not create token bucket for egress policer@d
> +])")
>  AT_CLEANUP
>  dnl --------------------------------------------------------------------------
>
> @@ -468,6 +471,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 \
> @@ -493,6 +497,7 @@ AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [std
>  dnl Clean up
>  AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr])
>  OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@Could not create token bucket for egress policer@d
>  \@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file or directory@d
>  ])")
>  AT_CLEANUP
> @@ -509,6 +514,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:cbs=2048])
> @@ -528,6 +534,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
> @@ -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 --------------------------------------------------------------------------
>
>
>
> +dnl --------------------------------------------------------------------------
> +dnl QoS (kpkts) create delete vport port
> +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +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:kpkts_rate=123 other-config:kpkts_burst=456])
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +sleep 2
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Remove egress policer
> +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port dpdkvhostuserclient0 qos])
> +
> +dnl Check egress policer was removed correctly
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
> +
> +dnl Clean up
> +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
> +\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
> +\@Could not create rte meter for egress policer@d
> +])")
> +AT_CLEANUP
> +dnl --------------------------------------------------------------------------
> +
> +
> +
> +dnl --------------------------------------------------------------------------
> +dnl QoS (kpkts) no rate
> +AT_SETUP([OVS-DPDK - QoS (kpkts) no rate])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +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:kpkts_burst=123])
> +sleep 2
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Check egress policer was not created
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
> +
> +dnl Clean up
> +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@d
> +])")
> +AT_CLEANUP
> +dnl --------------------------------------------------------------------------
> +
> +
> +
> +dnl --------------------------------------------------------------------------
> +dnl QoS (kpkts) no burst
> +AT_SETUP([OVS-DPDK - QoS (kpkts) no burst])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +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:kpkts_rate=123])
> +sleep 2
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Check egress policer was created
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([grep -E 'kpkts_rate: 123' stdout], [], [stdout])
> +
> +dnl Clean up
> +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
> +\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
> +])")
> +AT_CLEANUP
> +dnl --------------------------------------------------------------------------
> +
> +
> +
> +dnl --------------------------------------------------------------------------
> +dnl QoS (kpkts) max rate
> +AT_SETUP([OVS-DPDK - QoS (kpkts) max rate])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +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:kpkts_rate=42949671])
> +sleep 2
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Check egress policer was not created
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
> +
> +dnl Clean up
> +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@d
> +])")
> +AT_CLEANUP
> +dnl --------------------------------------------------------------------------
> +
> +
> +dnl --------------------------------------------------------------------------
> +dnl QoS (kpkts) max burst
> +AT_SETUP([OVS-DPDK - QoS (kpkts) max burst])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +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:kpkts_burst=42949671])
> +sleep 2
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Check egress policer was not created
> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
> +
> +dnl Clean up
> +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@d
> +])")
> +AT_CLEANUP
> +dnl --------------------------------------------------------------------------
>
>  dnl --------------------------------------------------------------------------
>  dnl MTU increase phy port
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 59c404bbb..b94ed2d34 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -4920,6 +4920,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>          bytes/tokens of the packet. If there are not enough tokens in the cbs
>          bucket the packet might be dropped.
>        </column>
> +      <column name="other_config" key="kpkts_rate"
> +          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
> +        The Packets Per Second (PPS) represents the packet per second rate at
> +        which the token bucket will be updated.
> +      </column>
> +      <column name="other_config" key="kpkts_burst"
> +          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
> +        The Packets Per Second Burst Size is measured in count and represents a
> +        token bucket.
> +      </column>
>      </group>
>
>      <group title="Configuration for linux-sfq">
> -- 
> 2.39.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron July 14, 2023, 12:20 p.m. UTC | #2
On 14 Jul 2023, at 13:52, Eelco Chaudron wrote:

> On 1 Jul 2023, at 16:40, miterv@outlook.com wrote:
>
>> From: Lin Huang <linhuang@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?


Also if a packet is forwarded by srtcm, but dropped by the packet policer should the tokens not be returned?

>> +    if (policer->type & POLICER_PKTPS) {
>> +        pps_drop = pkts_policer_run_single_packet(&policer->egress_tb, pkt_cnt);
>> +    }

>> +    cnt = MAX(bps_drop, pps_drop);

As in the previous patch, drops might not have happened in order.

>>      if (should_steal && cnt) {
>>          rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
>>      }

> Cheers,
>
> Eelco
>
>>
>> Add some unit tests for egress packet-per-second policing.
>>
>> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
>> ---
>>  Documentation/topics/dpdk/qos.rst |  15 ++-
>>  NEWS                              |   2 +
>>  lib/netdev-dpdk.c                 | 132 ++++++++++++++++----
>>  tests/system-dpdk.at              | 194 +++++++++++++++++++++++++++++-
>>  vswitchd/vswitch.xml              |  10 ++
>>  5 files changed, 327 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst
>> index a98ec672f..5f0b1469a 100644
>> --- a/Documentation/topics/dpdk/qos.rst
>> +++ b/Documentation/topics/dpdk/qos.rst
>> @@ -36,14 +36,21 @@ QoS (Egress Policing)
>>  Single Queue Policer
>>  ~~~~~~~~~~~~~~~~~~~~
>>
>> -Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic
>> -consisting of packets of size 64 bytes, the following command would limit the
>> -egress transmission rate of the port to ~1,000,000 packets per second::
>> +Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic,
>> +the following command would limit the egress transmission rate of the port to
>> +~1,000,000 bytes per second:
>>
>>      $ ovs-vsctl set port vhost-user0 qos=@newqos -- \
>> -        --id=@newqos create qos type=egress-policer other-config:cir=46000000 \
>> +        --id=@newqos create qos type=egress-policer other-config:cir=1000000 \
>>          other-config:cbs=2048`
>>
>> +or, the following command would limit the egress transmission rate of the port
>> +to ~1,000,000 packets per second:
>> +
>> +       ovs-vsctl set port vhost-user0 qos=@newqos -- \
>> +          --id=@newqos create qos type=egress-policer \
>> +          other-config:kpkts_rate=1000 other-config:kpkts_burst=1000
>> +
>>  To examine the QoS configuration of the port, run::
>>
>>      $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0
>> diff --git a/NEWS b/NEWS
>> index 0b5dc3db1..d88f490b1 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -44,6 +44,8 @@ Post-v3.1.0
>>       * IP and L4 checksum offload support is now enabled by default for
>>         interfaces that support it.  See the 'status' column in the 'interface'
>>         table to check the status.
>> +     * Added new configuration options 'kpkts_rate' and 'kpkts_burst' for
>> +      'egress-policer' to support kilo packet-per-second policing.
>>
>>
>>  v3.1.0 - 16 Feb 2023
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 2f7f74395..f97153665 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -19,6 +19,7 @@
>>
>>  #include <errno.h>
>>  #include <signal.h>
>> +#include <stdint.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <unistd.h>
>> @@ -59,6 +60,7 @@
>>  #include "openvswitch/ofp-parse.h"
>>  #include "openvswitch/ofp-print.h"
>>  #include "openvswitch/shash.h"
>> +#include "openvswitch/token-bucket.h"
>>  #include "openvswitch/vlog.h"
>>  #include "ovs-numa.h"
>>  #include "ovs-rcu.h"
>> @@ -91,6 +93,8 @@ static bool per_port_memory = false; /* Status of per port memory support */
>>  #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
>>  #define OVS_VPORT_DPDK "ovs_dpdk"
>>
>> +#define MAX_KPKTS_PARAMETER 4294967U  /* UINT32_MAX / 1000 */
>> +
>>  /*
>>   * need to reserve tons of extra space in the mbufs so we can align the
>>   * DMA addresses to 4KB.
>> @@ -400,6 +404,11 @@ struct dpdk_tx_queue {
>>      );
>>  };
>>
>> +enum policer_type {
>> +    POLICER_BPS   = 1 << 0,   /* Rate value in bytes/sec. */
>> +    POLICER_PKTPS = 1 << 1,   /* Rate value in packet/sec. */
>> +};
>> +
>>  struct ingress_policer {
>>      struct rte_meter_srtcm_params app_srtcm_params;
>>      struct rte_meter_srtcm in_policer;
>> @@ -2418,6 +2427,46 @@ srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter,
>>      return cnt;
>>  }
>>
>> +static int
>> +pkts_policer_run_single_packet(struct token_bucket *tb, int pkt_cnt)
>> +{
>> +    int cnt = 0;
>> +    long long int now = time_msec();
>> +
>> +    for (int i = 0; i < pkt_cnt; i++) {
>> +        /* Handle current packet. */
>> +        if (!token_bucket_withdraw(tb, 1, now)) {
>> +            /* Count dropped packets. */
>> +            cnt++;
>> +        }
>> +    }
>> +
>> +    return cnt;
>> +}
>> +
>> +static int
>> +pkts_policer_profile_config(struct token_bucket *tb,
>> +                            uint32_t kpkts_rate, uint32_t kpkts_burst)
>> +{
>> +    if (kpkts_rate > MAX_KPKTS_PARAMETER ||
>> +        kpkts_burst > MAX_KPKTS_PARAMETER) {
>> +        return EINVAL;
>> +    }
>> +
>> +   /* Rate in kilo-packets/second, bucket 1000 packets.
>> +    * msec * kilo-packets/sec = 1 packets. */
>> +    if (kpkts_rate) {
>> +        /* Parameters between (1 ~ MAX_KPKTS_PARAMETER). */
>> +        token_bucket_init(tb, kpkts_rate, kpkts_burst * 1000);
>> +    } else {
>> +        /* Zero means not to police the traffic.
>> +         * Return a error to not set POLICER_PKTPS valid. */
>> +        return EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int
>>  ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf **pkts,
>>                      int pkt_cnt, bool should_steal)
>> @@ -4882,24 +4931,37 @@ netdev_dpdk_queue_dump_done(const struct netdev *netdev OVS_UNUSED,
>>
>>
>>  /* egress-policer details */
>> +struct egress_policer_params {
>> +    struct rte_meter_srtcm_params app_srtcm_params;
>> +    uint32_t kpkts_rate;
>> +    uint32_t kpkts_burst;
>> +};
>>
>>  struct egress_policer {
>>      struct qos_conf qos_conf;
>> -    struct rte_meter_srtcm_params app_srtcm_params;
>> +    enum policer_type type;
>> +    struct token_bucket egress_tb;
>> +    struct egress_policer_params params;
>>      struct rte_meter_srtcm egress_meter;
>>      struct rte_meter_srtcm_profile egress_prof;
>>  };
>>
>>  static void
>>  egress_policer_details_to_param(const struct smap *details,
>> -                                struct rte_meter_srtcm_params *params)
>> +                                struct egress_policer_params *params)
>>  {
>> -    memset(params, 0, sizeof *params);
>> -    params->cir = smap_get_ullong(details, "cir", 0);
>> -    params->cbs = smap_get_ullong(details, "cbs", 0);
>> -    params->ebs = 0;
>> +    struct rte_meter_srtcm_params *srtcm_params = &params->app_srtcm_params;
>> +
>> +    memset(srtcm_params, 0, sizeof *srtcm_params);
>> +    srtcm_params->cir = smap_get_ullong(details, "cir", 0);
>> +    srtcm_params->cbs = smap_get_ullong(details, "cbs", 0);
>> +    srtcm_params->ebs = 0;
>> +
>> +    params->kpkts_rate  = smap_get_uint(details, "kpkts_rate", 0);
>> +    params->kpkts_burst = smap_get_uint(details, "kpkts_burst", 0);
>>  }
>>
>> +
>>  static int
>>  egress_policer_qos_construct(const struct smap *details,
>>                               struct qos_conf **conf)
>> @@ -4907,23 +4969,40 @@ egress_policer_qos_construct(const struct smap *details,
>>      struct egress_policer *policer;
>>      int err = 0;
>>
>> -    policer = xmalloc(sizeof *policer);
>> +    policer = xzalloc(sizeof *policer);
>>      qos_conf_init(&policer->qos_conf, &egress_policer_ops);
>> -    egress_policer_details_to_param(details, &policer->app_srtcm_params);
>> +    egress_policer_details_to_param(details, &policer->params);
>> +
>>      err = rte_meter_srtcm_profile_config(&policer->egress_prof,
>> -                                         &policer->app_srtcm_params);
>> +                                         &policer->params.app_srtcm_params);
>>      if (!err) {
>>          err = rte_meter_srtcm_config(&policer->egress_meter,
>>                                       &policer->egress_prof);
>>      }
>> +    if (err) {
>> +        VLOG_DBG("Could not create rte meter for egress policer");
>> +        err = -err;
>> +    } else {
>> +        policer->type |= POLICER_BPS;
>> +    }
>>
>> -    if (!err) {
>> -        *conf = &policer->qos_conf;
>> +    err = pkts_policer_profile_config(&policer->egress_tb,
>> +                                      policer->params.kpkts_rate,
>> +                                      policer->params.kpkts_burst);
>> +    if (err) {
>> +        VLOG_DBG("Could not create token bucket for egress policer");
>>      } else {
>> -        VLOG_ERR("Could not create rte meter for egress policer");
>> +        policer->type |= POLICER_PKTPS;
>> +    }
>> +
>> +    if (!policer->type) {
>> +        /* Both bps and kpkts contrsruction failed.*/
>>          free(policer);
>>          *conf = NULL;
>> -        err = -err;
>> +        VLOG_INFO("Could not create qos for egress policer");
>> +    } else {
>> +        err = 0;
>> +        *conf = &policer->qos_conf;
>>      }
>>
>>      return err;
>> @@ -4942,9 +5021,12 @@ egress_policer_qos_get(const struct qos_conf *conf, struct smap *details)
>>  {
>>      struct egress_policer *policer =
>>          CONTAINER_OF(conf, struct egress_policer, qos_conf);
>> +    struct egress_policer_params *params = &policer->params;
>>
>> -    smap_add_format(details, "cir", "%"PRIu64, policer->app_srtcm_params.cir);
>> -    smap_add_format(details, "cbs", "%"PRIu64, policer->app_srtcm_params.cbs);
>> +    smap_add_format(details, "cir", "%"PRIu64, params->app_srtcm_params.cir);
>> +    smap_add_format(details, "cbs", "%"PRIu64, params->app_srtcm_params.cbs);
>> +    smap_add_format(details, "kpkts_rate", "%"PRIu32, params->kpkts_rate);
>> +    smap_add_format(details, "kpkts_burst", "%"PRIu32, params->kpkts_burst);

If it’s not configured we should not return it, as it might confuse the user?

>>
>>      return 0;
>>  }
>> @@ -4955,25 +5037,33 @@ egress_policer_qos_is_equal(const struct qos_conf *conf,
>>  {
>>      struct egress_policer *policer =
>>          CONTAINER_OF(conf, struct egress_policer, qos_conf);
>> -    struct rte_meter_srtcm_params params;
>> +    struct egress_policer_params params;
>>
>>      egress_policer_details_to_param(details, &params);
>>
>> -    return !memcmp(&params, &policer->app_srtcm_params, sizeof params);
>> +    return !memcmp(&params, &policer->params, sizeof params);
>>  }
>>
>>  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 (policer->type & POLICER_PKTPS) {
>> +        pps_drop = pkts_policer_run_single_packet(&policer->egress_tb, pkt_cnt);
>> +    }
>>
>> +    cnt = MAX(bps_drop, pps_drop);
>>      if (should_steal && cnt) {
>>          rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
>>      }
>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
>> index 0f58e8574..233f7e493 100644
>> --- a/tests/system-dpdk.at
>> +++ b/tests/system-dpdk.at
>> @@ -435,6 +435,7 @@ OVS_DPDK_PRE_PHY_SKIP()
>>  OVS_DPDK_START()
>>
>>  dnl Add userspace bridge and attach it to OVS and add egress policer
>> +AT_CHECK([ovs-appctl vlog/set netdev_dpdk:dbg])

Should we still not get another error we should check for from a user perspective (same for all other occurrences)?


>>  AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
>>  AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
>>  OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048])
>> @@ -453,7 +454,9 @@ AT_CHECK([grep -E 'QoS not configured on phy0' stdout], [], [stdout])
>>
>>  dnl Clean up
>>  AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
>> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
>> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
>> +\@Could not create token bucket for egress policer@d
>> +])")
>>  AT_CLEANUP
>>  dnl --------------------------------------------------------------------------
>>
>> @@ -468,6 +471,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 \
>> @@ -493,6 +497,7 @@ AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [std
>>  dnl Clean up
>>  AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr])
>>  OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
>> +\@Could not create token bucket for egress policer@d
>>  \@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file or directory@d
>>  ])")
>>  AT_CLEANUP
>> @@ -509,6 +514,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:cbs=2048])
>> @@ -528,6 +534,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
>> @@ -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, however it would be nice if we can add some actual traffic tests.

>> +dnl --------------------------------------------------------------------------
>> +dnl QoS (kpkts) create delete vport port
>> +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port])
>> +AT_KEYWORDS([dpdk])
>> +
>> +OVS_DPDK_PRE_CHECK()
>> +OVS_DPDK_START()
>> +
>> +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:kpkts_rate=123 other-config:kpkts_burst=456])
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +sleep 2
>> +
>> +dnl Parse log file
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
>> +
>> +dnl Remove egress policer
>> +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port dpdkvhostuserclient0 qos])
>> +
>> +dnl Check egress policer was removed correctly
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
>> +
>> +dnl Clean up
>> +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
>> +\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
>> +\@Could not create rte meter for egress policer@d
>> +])")
>> +AT_CLEANUP
>> +dnl --------------------------------------------------------------------------
>> +
>> +
>> +
>> +dnl --------------------------------------------------------------------------
>> +dnl QoS (kpkts) no rate
>> +AT_SETUP([OVS-DPDK - QoS (kpkts) no rate])
>> +AT_KEYWORDS([dpdk])
>> +
>> +OVS_DPDK_PRE_CHECK()
>> +OVS_DPDK_START()
>> +
>> +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:kpkts_burst=123])
>> +sleep 2
>> +
>> +dnl Parse log file
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
>> +
>> +dnl Check egress policer was not created
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
>> +
>> +dnl Clean up
>> +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@d
>> +])")
>> +AT_CLEANUP
>> +dnl --------------------------------------------------------------------------
>> +
>> +
>> +
>> +dnl --------------------------------------------------------------------------
>> +dnl QoS (kpkts) no burst
>> +AT_SETUP([OVS-DPDK - QoS (kpkts) no burst])
>> +AT_KEYWORDS([dpdk])
>> +
>> +OVS_DPDK_PRE_CHECK()
>> +OVS_DPDK_START()
>> +
>> +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:kpkts_rate=123])
>> +sleep 2
>> +
>> +dnl Parse log file
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
>> +
>> +dnl Check egress policer was created
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +AT_CHECK([grep -E 'kpkts_rate: 123' stdout], [], [stdout])
>> +
>> +dnl Clean up
>> +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
>> +\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
>> +])")
>> +AT_CLEANUP
>> +dnl --------------------------------------------------------------------------
>> +
>> +
>> +
>> +dnl --------------------------------------------------------------------------
>> +dnl QoS (kpkts) max rate
>> +AT_SETUP([OVS-DPDK - QoS (kpkts) max rate])
>> +AT_KEYWORDS([dpdk])
>> +
>> +OVS_DPDK_PRE_CHECK()
>> +OVS_DPDK_START()
>> +
>> +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:kpkts_rate=42949671])
>> +sleep 2
>> +
>> +dnl Parse log file
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
>> +
>> +dnl Check egress policer was not created
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
>> +
>> +dnl Clean up
>> +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@d
>> +])")
>> +AT_CLEANUP
>> +dnl --------------------------------------------------------------------------
>> +
>> +
>> +dnl --------------------------------------------------------------------------
>> +dnl QoS (kpkts) max burst
>> +AT_SETUP([OVS-DPDK - QoS (kpkts) max burst])
>> +AT_KEYWORDS([dpdk])
>> +
>> +OVS_DPDK_PRE_CHECK()
>> +OVS_DPDK_START()
>> +
>> +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:kpkts_burst=42949671])
>> +sleep 2
>> +
>> +dnl Parse log file
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
>> +
>> +dnl Check egress policer was not created
>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
>> +AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
>> +
>> +dnl Clean up
>> +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@d
>> +])")
>> +AT_CLEANUP
>> +dnl --------------------------------------------------------------------------
>>
>>  dnl --------------------------------------------------------------------------
>>  dnl MTU increase phy port
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 59c404bbb..b94ed2d34 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -4920,6 +4920,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>>          bytes/tokens of the packet. If there are not enough tokens in the cbs
>>          bucket the packet might be dropped.
>>        </column>
>> +      <column name="other_config" key="kpkts_rate"
>> +          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
>> +        The Packets Per Second (PPS) represents the packet per second rate at
>> +        which the token bucket will be updated.
>> +      </column>
>> +      <column name="other_config" key="kpkts_burst"
>> +          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
>> +        The Packets Per Second Burst Size is measured in count and represents a
>> +        token bucket.
>> +      </column>
>>      </group>
>>
>>      <group title="Configuration for linux-sfq">
>> -- 
>> 2.39.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets July 14, 2023, 3:01 p.m. UTC | #3
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, miterv@outlook.com wrote:
>>
>>> From: Lin Huang <linhuang@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?

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.
miter July 15, 2023, 2:49 a.m. UTC | #4
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, miterv@outlook.com wrote:
>>>
>>>> From: Lin Huang <linhuang@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.
>
> 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.
Eelco Chaudron July 17, 2023, 9:14 a.m. UTC | #5
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, miterv@outlook.com wrote:
>>>>
>>>>> From: Lin Huang <linhuang@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;



>> 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.
Ilya Maximets July 17, 2023, 9:39 a.m. UTC | #6
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, miterv@outlook.com wrote:
>>>>>
>>>>>> From: Lin Huang <linhuang@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.
>
diff mbox series

Patch

diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst
index a98ec672f..5f0b1469a 100644
--- a/Documentation/topics/dpdk/qos.rst
+++ b/Documentation/topics/dpdk/qos.rst
@@ -36,14 +36,21 @@  QoS (Egress Policing)
 Single Queue Policer
 ~~~~~~~~~~~~~~~~~~~~
 
-Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic
-consisting of packets of size 64 bytes, the following command would limit the
-egress transmission rate of the port to ~1,000,000 packets per second::
+Assuming you have a :doc:`vhost-user port <vhost-user>` transmitting traffic,
+the following command would limit the egress transmission rate of the port to
+~1,000,000 bytes per second:
 
     $ ovs-vsctl set port vhost-user0 qos=@newqos -- \
-        --id=@newqos create qos type=egress-policer other-config:cir=46000000 \
+        --id=@newqos create qos type=egress-policer other-config:cir=1000000 \
         other-config:cbs=2048`
 
+or, the following command would limit the egress transmission rate of the port
+to ~1,000,000 packets per second:
+
+       ovs-vsctl set port vhost-user0 qos=@newqos -- \
+          --id=@newqos create qos type=egress-policer \
+          other-config:kpkts_rate=1000 other-config:kpkts_burst=1000
+
 To examine the QoS configuration of the port, run::
 
     $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0
diff --git a/NEWS b/NEWS
index 0b5dc3db1..d88f490b1 100644
--- a/NEWS
+++ b/NEWS
@@ -44,6 +44,8 @@  Post-v3.1.0
      * IP and L4 checksum offload support is now enabled by default for
        interfaces that support it.  See the 'status' column in the 'interface'
        table to check the status.
+     * Added new configuration options 'kpkts_rate' and 'kpkts_burst' for
+      'egress-policer' to support kilo packet-per-second policing.
 
 
 v3.1.0 - 16 Feb 2023
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 2f7f74395..f97153665 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -19,6 +19,7 @@ 
 
 #include <errno.h>
 #include <signal.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -59,6 +60,7 @@ 
 #include "openvswitch/ofp-parse.h"
 #include "openvswitch/ofp-print.h"
 #include "openvswitch/shash.h"
+#include "openvswitch/token-bucket.h"
 #include "openvswitch/vlog.h"
 #include "ovs-numa.h"
 #include "ovs-rcu.h"
@@ -91,6 +93,8 @@  static bool per_port_memory = false; /* Status of per port memory support */
 #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
 #define OVS_VPORT_DPDK "ovs_dpdk"
 
+#define MAX_KPKTS_PARAMETER 4294967U  /* UINT32_MAX / 1000 */
+
 /*
  * need to reserve tons of extra space in the mbufs so we can align the
  * DMA addresses to 4KB.
@@ -400,6 +404,11 @@  struct dpdk_tx_queue {
     );
 };
 
+enum policer_type {
+    POLICER_BPS   = 1 << 0,   /* Rate value in bytes/sec. */
+    POLICER_PKTPS = 1 << 1,   /* Rate value in packet/sec. */
+};
+
 struct ingress_policer {
     struct rte_meter_srtcm_params app_srtcm_params;
     struct rte_meter_srtcm in_policer;
@@ -2418,6 +2427,46 @@  srtcm_policer_run_single_packet(struct rte_meter_srtcm *meter,
     return cnt;
 }
 
+static int
+pkts_policer_run_single_packet(struct token_bucket *tb, int pkt_cnt)
+{
+    int cnt = 0;
+    long long int now = time_msec();
+
+    for (int i = 0; i < pkt_cnt; i++) {
+        /* Handle current packet. */
+        if (!token_bucket_withdraw(tb, 1, now)) {
+            /* Count dropped packets. */
+            cnt++;
+        }
+    }
+
+    return cnt;
+}
+
+static int
+pkts_policer_profile_config(struct token_bucket *tb,
+                            uint32_t kpkts_rate, uint32_t kpkts_burst)
+{
+    if (kpkts_rate > MAX_KPKTS_PARAMETER ||
+        kpkts_burst > MAX_KPKTS_PARAMETER) {
+        return EINVAL;
+    }
+
+   /* Rate in kilo-packets/second, bucket 1000 packets.
+    * msec * kilo-packets/sec = 1 packets. */
+    if (kpkts_rate) {
+        /* Parameters between (1 ~ MAX_KPKTS_PARAMETER). */
+        token_bucket_init(tb, kpkts_rate, kpkts_burst * 1000);
+    } else {
+        /* Zero means not to police the traffic.
+         * Return a error to not set POLICER_PKTPS valid. */
+        return EINVAL;
+    }
+
+    return 0;
+}
+
 static int
 ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf **pkts,
                     int pkt_cnt, bool should_steal)
@@ -4882,24 +4931,37 @@  netdev_dpdk_queue_dump_done(const struct netdev *netdev OVS_UNUSED,
 
 
 /* egress-policer details */
+struct egress_policer_params {
+    struct rte_meter_srtcm_params app_srtcm_params;
+    uint32_t kpkts_rate;
+    uint32_t kpkts_burst;
+};
 
 struct egress_policer {
     struct qos_conf qos_conf;
-    struct rte_meter_srtcm_params app_srtcm_params;
+    enum policer_type type;
+    struct token_bucket egress_tb;
+    struct egress_policer_params params;
     struct rte_meter_srtcm egress_meter;
     struct rte_meter_srtcm_profile egress_prof;
 };
 
 static void
 egress_policer_details_to_param(const struct smap *details,
-                                struct rte_meter_srtcm_params *params)
+                                struct egress_policer_params *params)
 {
-    memset(params, 0, sizeof *params);
-    params->cir = smap_get_ullong(details, "cir", 0);
-    params->cbs = smap_get_ullong(details, "cbs", 0);
-    params->ebs = 0;
+    struct rte_meter_srtcm_params *srtcm_params = &params->app_srtcm_params;
+
+    memset(srtcm_params, 0, sizeof *srtcm_params);
+    srtcm_params->cir = smap_get_ullong(details, "cir", 0);
+    srtcm_params->cbs = smap_get_ullong(details, "cbs", 0);
+    srtcm_params->ebs = 0;
+
+    params->kpkts_rate  = smap_get_uint(details, "kpkts_rate", 0);
+    params->kpkts_burst = smap_get_uint(details, "kpkts_burst", 0);
 }
 
+
 static int
 egress_policer_qos_construct(const struct smap *details,
                              struct qos_conf **conf)
@@ -4907,23 +4969,40 @@  egress_policer_qos_construct(const struct smap *details,
     struct egress_policer *policer;
     int err = 0;
 
-    policer = xmalloc(sizeof *policer);
+    policer = xzalloc(sizeof *policer);
     qos_conf_init(&policer->qos_conf, &egress_policer_ops);
-    egress_policer_details_to_param(details, &policer->app_srtcm_params);
+    egress_policer_details_to_param(details, &policer->params);
+
     err = rte_meter_srtcm_profile_config(&policer->egress_prof,
-                                         &policer->app_srtcm_params);
+                                         &policer->params.app_srtcm_params);
     if (!err) {
         err = rte_meter_srtcm_config(&policer->egress_meter,
                                      &policer->egress_prof);
     }
+    if (err) {
+        VLOG_DBG("Could not create rte meter for egress policer");
+        err = -err;
+    } else {
+        policer->type |= POLICER_BPS;
+    }
 
-    if (!err) {
-        *conf = &policer->qos_conf;
+    err = pkts_policer_profile_config(&policer->egress_tb,
+                                      policer->params.kpkts_rate,
+                                      policer->params.kpkts_burst);
+    if (err) {
+        VLOG_DBG("Could not create token bucket for egress policer");
     } else {
-        VLOG_ERR("Could not create rte meter for egress policer");
+        policer->type |= POLICER_PKTPS;
+    }
+
+    if (!policer->type) {
+        /* Both bps and kpkts contrsruction failed.*/
         free(policer);
         *conf = NULL;
-        err = -err;
+        VLOG_INFO("Could not create qos for egress policer");
+    } else {
+        err = 0;
+        *conf = &policer->qos_conf;
     }
 
     return err;
@@ -4942,9 +5021,12 @@  egress_policer_qos_get(const struct qos_conf *conf, struct smap *details)
 {
     struct egress_policer *policer =
         CONTAINER_OF(conf, struct egress_policer, qos_conf);
+    struct egress_policer_params *params = &policer->params;
 
-    smap_add_format(details, "cir", "%"PRIu64, policer->app_srtcm_params.cir);
-    smap_add_format(details, "cbs", "%"PRIu64, policer->app_srtcm_params.cbs);
+    smap_add_format(details, "cir", "%"PRIu64, params->app_srtcm_params.cir);
+    smap_add_format(details, "cbs", "%"PRIu64, params->app_srtcm_params.cbs);
+    smap_add_format(details, "kpkts_rate", "%"PRIu32, params->kpkts_rate);
+    smap_add_format(details, "kpkts_burst", "%"PRIu32, params->kpkts_burst);
 
     return 0;
 }
@@ -4955,25 +5037,33 @@  egress_policer_qos_is_equal(const struct qos_conf *conf,
 {
     struct egress_policer *policer =
         CONTAINER_OF(conf, struct egress_policer, qos_conf);
-    struct rte_meter_srtcm_params params;
+    struct egress_policer_params params;
 
     egress_policer_details_to_param(details, &params);
 
-    return !memcmp(&params, &policer->app_srtcm_params, sizeof params);
+    return !memcmp(&params, &policer->params, sizeof params);
 }
 
 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 (policer->type & POLICER_PKTPS) {
+        pps_drop = pkts_policer_run_single_packet(&policer->egress_tb, pkt_cnt);
+    }
 
+    cnt = MAX(bps_drop, pps_drop);
     if (should_steal && cnt) {
         rte_pktmbuf_free_bulk(&pkts[pkt_cnt - cnt], cnt);
     }
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 0f58e8574..233f7e493 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -435,6 +435,7 @@  OVS_DPDK_PRE_PHY_SKIP()
 OVS_DPDK_START()
 
 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 phy0 -- set Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
 OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048])
@@ -453,7 +454,9 @@  AT_CHECK([grep -E 'QoS not configured on phy0' stdout], [], [stdout])
 
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@Could not create token bucket for egress policer@d
+])")
 AT_CLEANUP
 dnl --------------------------------------------------------------------------
 
@@ -468,6 +471,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 \
@@ -493,6 +497,7 @@  AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [std
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], [stderr])
 OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@Could not create token bucket for egress policer@d
 \@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file or directory@d
 ])")
 AT_CLEANUP
@@ -509,6 +514,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:cbs=2048])
@@ -528,6 +534,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
@@ -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 --------------------------------------------------------------------------
 
 
 
+dnl --------------------------------------------------------------------------
+dnl QoS (kpkts) create delete vport port
+AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+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:kpkts_rate=123 other-config:kpkts_burst=456])
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+sleep 2
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
+
+dnl Remove egress policer
+AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port dpdkvhostuserclient0 qos])
+
+dnl Check egress policer was removed correctly
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
+
+dnl Clean up
+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
+\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
+\@Could not create rte meter for egress policer@d
+])")
+AT_CLEANUP
+dnl --------------------------------------------------------------------------
+
+
+
+dnl --------------------------------------------------------------------------
+dnl QoS (kpkts) no rate
+AT_SETUP([OVS-DPDK - QoS (kpkts) no rate])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+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:kpkts_burst=123])
+sleep 2
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
+
+dnl Check egress policer was not created
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
+
+dnl Clean up
+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@d
+])")
+AT_CLEANUP
+dnl --------------------------------------------------------------------------
+
+
+
+dnl --------------------------------------------------------------------------
+dnl QoS (kpkts) no burst
+AT_SETUP([OVS-DPDK - QoS (kpkts) no burst])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+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:kpkts_rate=123])
+sleep 2
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
+
+dnl Check egress policer was created
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([grep -E 'kpkts_rate: 123' stdout], [], [stdout])
+
+dnl Clean up
+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
+\@Failed to set QoS type egress-policer on port dpdkvhostuserclient0@d
+])")
+AT_CLEANUP
+dnl --------------------------------------------------------------------------
+
+
+
+dnl --------------------------------------------------------------------------
+dnl QoS (kpkts) max rate
+AT_SETUP([OVS-DPDK - QoS (kpkts) max rate])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+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:kpkts_rate=42949671])
+sleep 2
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
+
+dnl Check egress policer was not created
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
+
+dnl Clean up
+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@d
+])")
+AT_CLEANUP
+dnl --------------------------------------------------------------------------
+
+
+dnl --------------------------------------------------------------------------
+dnl QoS (kpkts) max burst
+AT_SETUP([OVS-DPDK - QoS (kpkts) max burst])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+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:kpkts_burst=42949671])
+sleep 2
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user client: socket created" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." ovs-vswitchd.log], [], [stdout])
+
+dnl Check egress policer was not created
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([grep -E 'QoS not configured on dpdkvhostuserclient0' stdout], [], [stdout])
+
+dnl Clean up
+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@d
+])")
+AT_CLEANUP
+dnl --------------------------------------------------------------------------
 
 dnl --------------------------------------------------------------------------
 dnl MTU increase phy port
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 59c404bbb..b94ed2d34 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -4920,6 +4920,16 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
         bytes/tokens of the packet. If there are not enough tokens in the cbs
         bucket the packet might be dropped.
       </column>
+      <column name="other_config" key="kpkts_rate"
+          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
+        The Packets Per Second (PPS) represents the packet per second rate at
+        which the token bucket will be updated.
+      </column>
+      <column name="other_config" key="kpkts_burst"
+          type='{"type": "integer", "minInteger": 0, "maxInteger": 4294967}'>
+        The Packets Per Second Burst Size is measured in count and represents a
+        token bucket.
+      </column>
     </group>
 
     <group title="Configuration for linux-sfq">