| Message ID | SY4PR01MB843851E840EA0B91312B17D6CDE2A@SY4PR01MB8438.ausprd01.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev-dpdk: Add support for userspace port-based packet-per-second policing. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 26 Aug 2023, at 8:01, 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 a new egress Qos type called 'kpkts-policer'. > the policer police the kilo-packet per second at which the token > bucket > be updated by 'kpkts_rate'. and the policer's burst size is defined by > 'kpkts_burst'. > > Examples: > $ovs-vsctl set port vhost-user0 qos=@newqos -- > --id=@newqos create qos type=kpkts-policer \ > other-config:kpkts_rate=123 other-config:kpkts_burst=123 I think the name of the policer is odd, it should reflect the actual policer type. Maybe it should be called pps-policer? Also, the parameters should be more inline with what the are. No need to add kpkts in front of it. Other egress policers use things like cir, max-rate, etc., without specifying the unit. So you example would become: $ovs-vsctl set port vhost-user0 qos=@newqos -- --id=@newqos create qos type=pps-policer \ other-config:rate=123 other-config: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 | 21 +++ > NEWS | 1 + > lib/netdev-dpdk.c | 159 +++++++++++++++++++ > tests/system-dpdk.at | 255 > ++++++++++++++++++++++++++++++ > vswitchd/vswitch.xml | 32 ++++ > 5 files changed, 468 insertions(+) > > diff --git a/Documentation/topics/dpdk/qos.rst > b/Documentation/topics/dpdk/qos.rst > index a98ec672f..37f482cc5 100644 > --- a/Documentation/topics/dpdk/qos.rst > +++ b/Documentation/topics/dpdk/qos.rst > @@ -36,6 +36,9 @@ QoS (Egress Policing) > Single Queue Policer > ~~~~~~~~~~~~~~~~~~~~ > > +Bytes Per Second 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:: > @@ -52,6 +55,24 @@ To clear the QoS configuration from the port and > ovsdb, run:: > > $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos > > +Packets Per Second Policer > +++++++++++++++++++++++++++ > + > +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 packets per second:: > + > + ovs-vsctl set port vhost-user0 qos=@newqos -- \ > + --id=@newqos create qos type=kpkts-policer \ > + other-config:kpkts_rate=1000 other-config:kpkts_burst=1000 I will only add this once, but if we move to pps-policer rather than kpkts-policer we have to change the naming in the documentation and troughout the code to make it consitant. This sounds obvious, but there are some confusing pieces of code in OVS due to last minute naming change of the commands :) > +To examine the QoS configuration of the port, run:: > + > + $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0 > + > +To clear the QoS configuration from the port and ovsdb, run:: > + > + $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos > > Multi Queue Policer > ~~~~~~~~~~~~~~~~~~~ > diff --git a/NEWS b/NEWS > index b582bdbbc..430c3daaf 100644 > --- a/NEWS > +++ b/NEWS > @@ -63,6 +63,7 @@ v3.2.0 - 17 Aug 2023 > * 'ovs-appctl dpif-netdev/pmd-sleep-show' command was added to > get the > max sleep configuration of PMD thread cores. > * Removed experimental tag from PMD load based sleeping. > + * Added new Qos type 'pkts-policer' to support kilo > packet-per-second policing. > - Linux TC offload: > * Add support for offloading VXLAN tunnels with the GBP > extensions. > - Python > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index bc8204f7e..c6a26dc7e 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 */ If UINT32_MAX / 1000 is what you need, why no just define it that way? #define MAX_KPKTS_PARAMETER UINT32_MAX / 1000 > + > /* > * need to reserve tons of extra space in the mbufs so we can align > the > * DMA addresses to 4KB. > @@ -346,6 +350,7 @@ struct dpdk_qos_ops { > /* dpdk_qos_ops for each type of user space QoS implementation. */ > static const struct dpdk_qos_ops egress_policer_ops; > static const struct dpdk_qos_ops trtcm_policer_ops; > +static const struct dpdk_qos_ops kpkts_policer_ops; New name, pps_policer_ops. > > /* > * Array of dpdk_qos_ops, contains pointer to all supported QoS > @@ -354,6 +359,7 @@ static const struct dpdk_qos_ops > trtcm_policer_ops; > static const struct dpdk_qos_ops *const qos_confs[] = { > &egress_policer_ops, > &trtcm_policer_ops, > + &kpkts_policer_ops, > NULL > }; > > @@ -5571,6 +5577,159 @@ static const struct dpdk_qos_ops > trtcm_policer_ops = { > .qos_queue_dump_state_init = > trtcm_policer_qos_queue_dump_state_init > }; > > +/* kpkts-policer details */ > +struct kpkts_policer { > + struct qos_conf qos_conf; > + struct token_bucket tb; > + uint32_t kpkts_rate; > + uint32_t kpkts_burst; These can just be rate and burst. > +}; > + > +static int > +kpkts_policer_run_single_packet(struct token_bucket *tb, struct > rte_mbuf **pkts, I know this name was copied from the other policer, but maybe it should be renamed to better represent it's function. Somehting like: pps_policer_run_on_packet_batch()? > + int pkt_cnt, bool should_steal) > +{ > + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; > + long long int now = time_msec(); > + int i = 0, n = 0; > + int cnt = 0; > + > + for (i = 0; i < pkt_cnt; i++) { > + struct rte_mbuf *pkt = pkts[i]; > + /* Handle current packet. */ > + if (token_bucket_withdraw(tb, 1, now)) { I think this funciont can be optimized, as 'now' is not chaning for this batch. So, if we fail one packet, the rest of the batch would also fail. Maybe we can even add a new token_bucket_withdraw_max() like function to get max buckets available giving the number you want. For example you request 100 buckets, but if only 50 buckets are available it will take 50 and return 50. Now you know the first 50 packets are a pass, and another 50 need to be dropped. This will allow you to do something like: tokens = token_bucket_withdraw_max(pkt_cnt); if (should_steal && tokens != pkt_cnt) { rte_pktmbuf_free_bulk(pkts + tokens, pkt_cnt - tokens); } return tokens; This also make the token_bucket_withdraw() patch obsolete. > + /* Count passed packets. */ > + if (cnt != i) { > + pkts[cnt] = pkt; > + } > + cnt++; > + } else if (should_steal) { See comment in patch 2. > + /* Count dropped packets. */ > + should_steal_batch[n++] = pkt; > + } > + } > + > + if (n) { > + rte_pktmbuf_free_bulk(should_steal_batch, n); > + } > + > + return cnt; > +} > + > +static int > +kpkts_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) { If you add the zero check for kpkts_rate here you can remove the if/else below: > + return EINVAL; > + } > + > + /* Rate in kilo-packets/second, bucket 1000 packets. > + * msec * kilo-packets/sec = 1 packets. */ Be consistent with multi line comments. > + 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 EINVAL; > + } > + > + return 0; So with this the function looks like this: static int kpkts_policer_profile_config(struct token_bucket *tb, uint32_t rate, uint32_t burst) { if (!rate || rate > MAX_PPS_PARAMETER || burst > MAX_PPS_PARAMETER) { return EINVAL; } /* * Rate in kilo-packets/second, bucket 1000 packets. * msec * kilo-packets/sec = 1 packets. */ token_bucket_init(tb, rate, burst * 1000); return 0; } > +} > + > +static int > +kpkts_policer_qos_construct(const struct smap *details, struct > qos_conf **conf) > +{ > + uint32_t kpkts_rate, kpkts_burst; > + struct kpkts_policer *policer; > + int err; > + > + policer = xzalloc(sizeof *policer); > + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0); > + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0); > + > + /* > + * Force to 0 if no rate specified, > + * default to rate if burst is 0, > + * else stick with user-specified value. > + */ This copmments can be on two lines: /* * Force to 0 if no rate specified, default to rate if burst is 0, * else stick with user-specified value. */ > + kpkts_burst = (!kpkts_rate ? 0 : !kpkts_burst ? kpkts_rate : > kpkts_burst); I would move this is bit to be more clear: kpkts_burst = !kpkts_rate ? 0 : (!kpkts_burst ? kpkts_rate : kpkts_burst); > + qos_conf_init(&policer->qos_conf, &kpkts_policer_ops); > + err = kpkts_policer_profile_config(&policer->tb, kpkts_rate, > kpkts_burst); > + if (!err) { > + policer->kpkts_rate = kpkts_rate; > + policer->kpkts_burst = kpkts_burst; Why do we not pass policer to kpkts_policer_profile_config() and we can move this above part there also? I see that this is because the kpkts_policer_profile_config() function is re-used in the ingress patch. > + > + *conf = &policer->qos_conf; > + } else { > + VLOG_DBG("Could not create token bucket for egress policer"); > + free(policer); > + *conf = NULL; > + } > + > + return err; > +} > + > +static void > +kpkts_policer_qos_destruct(struct qos_conf *conf) > +{ > + struct kpkts_policer *policer = CONTAINER_OF(conf, struct > kpkts_policer, > + qos_conf); > + > + free(policer); > +} > + > +static int > +kpkts_policer_qos_get(const struct qos_conf *conf, struct smap > *details) > +{ > + struct kpkts_policer *policer = CONTAINER_OF(conf, struct > kpkts_policer, > + qos_conf); > + > + smap_add_format(details, "kpkts_rate", "%"PRIu32, > policer->kpkts_rate); > + smap_add_format(details, "kpkts_burst", "%"PRIu32, > policer->kpkts_burst); > + > + return 0; > +} > + > +static bool > +kpkts_pkts_policer_qos_is_equal(const struct qos_conf *conf, > + const struct smap *details) > +{ > + uint32_t rate, burst; > + struct kpkts_policer *policer = CONTAINER_OF(conf, struct > kpkts_policer, > + qos_conf); > + > + rate = smap_get_uint(details, "pkts_rate", 0); > + burst = smap_get_uint(details, "pkts_burst", 0); I think you where using kpkts_? But this need to change anyway. > + > + return (policer->tb.rate == rate && policer->tb.burst == burst); > +} > + > +static int > +kpkts_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int > pkt_cnt, > + bool should_steal) > +{ > + int cnt; > + struct kpkts_policer *policer = CONTAINER_OF(conf, struct > kpkts_policer, > + qos_conf); > + > + cnt = kpkts_policer_run_single_packet(&policer->tb, pkts, > pkt_cnt, > + should_steal); > + > + return cnt; > +} > + > +static const struct dpdk_qos_ops kpkts_policer_ops = { > + .qos_name = "kpkts-policer", > + .qos_construct = kpkts_policer_qos_construct, > + .qos_destruct = kpkts_policer_qos_destruct, > + .qos_get = kpkts_policer_qos_get, > + .qos_is_equal = kpkts_pkts_policer_qos_is_equal, > + .qos_run = kpkts_policer_run > +}; > + > static int > dpdk_rx_steer_add_flow(struct netdev_dpdk *dev, > const struct rte_flow_item items[], > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > index 0f58e8574..8b80a31e6 100644 > --- a/tests/system-dpdk.at > +++ b/tests/system-dpdk.at > @@ -570,6 +570,261 @@ dnl > -------------------------------------------------------------------------- > > > > +dnl > -------------------------------------------------------------------------- > +dnl QoS (kpkts) create delete vport port > +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port]) I would include the policer name in full, so it will become: AT_SETUP([OVS-DPDK - QoS (pps-policer) 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 Missing dot at the end of the comment. True in other places also, so please fix all instances. > +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=kpkts-policer > other-config:kpkts_rate=123 other-config:kpkts_burst=456]) > +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], > [], [stdout]) We should check for the exact output. > +sleep 2 Why a sleep we should avoid this. please use OVS_WAIT_UNTIL on the below greps. > + > +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]) Can we fold these two lines into one. > + > +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 kpkts-policer on port dpdkvhostuserclient0: > Invalid argument@d Why excluding this error message, it should not be generated here, or do I miss something? > +])") > +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=kpkts-policer > other-config:kpkts_burst=123]) > +sleep 2 > + See above test case > +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 > +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: > Invalid argument@d Maybe we should also check above to make sure we see this error message. > +])") > +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=kpkts-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]) What about the burst? As we no the full output, why not do it as part of the first AT_CHECK instead of passing it to stdout. > + > +dnl Check egress policer was deleted > +QOS_UUID=`ovs-vsctl get port dpdkvhostuserclient0 qos` > +AT_CHECK([ovs-vsctl set qos $QOS_UUID other_config:kpkts_rate=0]) > +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 kpkts-policer on port dpdkvhostuserclient0: > Invalid argument@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=kpkts-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 > +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: > Invalid argument@d > +])") > +AT_CLEANUP > +dnl > -------------------------------------------------------------------------- > + Add one more newline. As it looks like all have 3. > + > +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=kpkts-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 > +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: > Invalid argument@d > +])") > +AT_CLEANUP > +dnl > -------------------------------------------------------------------------- > + Add one more newline. As it looks like all have 3. > + > +dnl > -------------------------------------------------------------------------- > +dnl QoS (kpkts) testpmd flowgen test > +AT_SETUP([OVS-DPDK - QoS (kpkts) police]) > +AT_KEYWORDS([dpdk]) > + > +OVS_DPDK_PRE_CHECK() > +AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null]) > +OVS_DPDK_START([--no-pci]) > + > +dnl Find number of sockets > +AT_CHECK([lscpu], [], [stdout]) > +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while > (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) > + > +dnl Add userspace bridge and attach it to OVS > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 > datapath_type=netdev]) > + > +dnl Parse log file > +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface > dpdkvhostuser0 type=dpdkvhostuser], [], [stdout], [stderr]) > +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser1 -- set Interface > dpdkvhostuser1 type=dpdkvhostuser], [], [stdout], [stderr]) > +AT_CHECK([ovs-vsctl show], [], [stdout]) Why execute the show? Nothing is done with the output? > + > +dnl Parse log file > +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) vhost-user > server: socket created" ovs-vswitchd.log], [], [stdout]) > +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for > vhost-user port dpdkvhostuser0" ovs-vswitchd.log], [], [stdout]) > +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding > succeeded" ovs-vswitchd.log], [], [stdout]) > + > +dnl Parse log file > +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) vhost-user > server: socket created" ovs-vswitchd.log], [], [stdout]) > +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser1 created for > vhost-user port dpdkvhostuser1" ovs-vswitchd.log], [], [stdout]) > +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) binding > succeeded" ovs-vswitchd.log], [], [stdout]) > + > +dnl Configure the same QoS for both ports. > +AT_CHECK([ovs-vsctl set port dpdkvhostuser0 qos=@newqos -- > --id=@newqos create qos type=kpkts-policer other-config:kpkts_rate=1 > other-config:kpkts_burst=1], [0], [ignore]) > + > +dnl add flows, only send packets from dpdkvhostuser1 to > dpdkvhostuser0. Start all commented with a Capital. > +AT_DATA([flows.txt], [dnl > +priority=100,in_port=dpdkvhostuser1,ip,actions=dpdkvhostuser0 > +]) > + > +AT_CHECK([ovs-ofctl del-flows br10]) > +AT_CHECK([ovs-ofctl add-flows br10 flows.txt]) If it's a single flow just do it in the command itself no need to create a flows.txt file. > + > +dnl Execute testpmd in background > +on_exit "pkill -f -x -9 'tail -f /dev/null'" > +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" > --no-pci\ > + --vdev="net_virtio_user0,path=$OVS_RUNDIR/dpdkvhostuser0" > \ > + --vdev="net_virtio_user1,path=$OVS_RUNDIR/dpdkvhostuser1" > \ > + --single-file-segments -- --forward-mode=flowgen -a > > $OVS_RUNDIR/testpmd-dpdkvhostuser.log 2>&1 & > + > +dnl sent packet 10 second. > +AT_CHECK([sleep 10]) Do we need 10 seconds, can we make this shorter? > + > +dnl Clean up the testpmd now > +pkill -f -x -9 'tail -f /dev/null' > + > +dnl ---------------------- Forward statistics for port 0 > ---------------------- > +dnl RX-packets: 9911 RX-dropped: 0 RX-total: > 9911 > +dnl TX-packets: 15937632 TX-dropped: 226661984 TX-total: > 242599616 > +dnl > ---------------------------------------------------------------------------- > +port0_rx_packets=`cat testpmd-dpdkvhostuser.log | grep "Forward > statistics for port 0" -A 1 | grep "RX-packets:" | awk '{print $2}'` > +echo "port0_rx_packets=$port0_rx_packets" > + > +AT_CHECK([test $port0_rx_packets -lt 10500]) > +AT_CHECK([test $port0_rx_packets -gt 9500]) > + > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ > +\@dpdkvhostuser ports are considered deprecated; please migrate to > dpdkvhostuserclient ports.@d Can we migrate to dpdkvhostuserclient ports? > +])") > +AT_CLEANUP > +dnl > -------------------------------------------------------------------------- > + > + > > dnl > -------------------------------------------------------------------------- > dnl MTU increase phy port > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index cfcde34ff..19fc13873 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -4874,6 +4874,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 > type=patch options:peer=p1 \ > created with the same <code>other_config</code> values as > the > physical port. > </dd> > + <dt><code>kpkts-policer</code></dt> > + <dd> > + A DPDK egress packet per second policer algorithm using the > ovs > + token bucket library. The token bucket library provides an > + implementation which allows the policing of packets > traffic. The > + implementation in OVS essentially creates a single token > bucket used > + to police traffic. It should be noted that when the token > bucket is > + configured as part of QoS there will be a performance > overhead as the > + token bucket itself will consume CPU cycles in order to > police > + traffic. These CPU cycles ordinarily are used for packet > proccessing. > + As such the drop in performance will be noticed in terms of > overall > + aggregate traffic throughput. > + </dd> > </dl> > </column> > > @@ -4966,6 +4979,25 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 > type=patch options:peer=p1 \ > </column> > </group> > > + <group title="Configuration for kpkts-policer QoS"> > + <p> > + <ref table="QoS"/> <ref table="QoS" column="type"/> > + <code>kpkts-policer</code> provides egress pkts policing for > userspace > + port types with DPDK. > + > + It has the following key-value pairs defined. > + </p> > + > + <column name="other_config" key="kpkts_rate" type='{"type": > "integer"}'> > + The Kilo Packets Per Second (kpps) 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"}'> > + The Packets Per Second Burst Size is measured in count and > represents a > + token bucket. > + </column> > + </group> > + > <group title="Configuration for linux-sfq"> > <p> > The <code>linux-sfq</code> QoS supports the following > key-value pairs: > -- > 2.39.3
Hi Eelco, Some comments below. On 9/19/2023 10:58 PM, Eelco Chaudron wrote: > On 26 Aug 2023, at 8:01, 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 a new egress Qos type called 'kpkts-policer'. >> the policer police the kilo-packet per second at which the token bucket >> be updated by 'kpkts_rate'. and the policer's burst size is defined by >> 'kpkts_burst'. >> >> Examples: >> $ovs-vsctl set port vhost-user0 qos=@newqos -- >> --id=@newqos create qos type=kpkts-policer \ >> other-config:kpkts_rate=123 other-config:kpkts_burst=123 > > I think the name of the policer is odd, it should reflect the actual > policer type. > Maybe it should be called pps-policer? Becouse the Ingress Policing use "kpkts" to name. In order to keep ingress and egress more symmetric. $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_rate=1000 ingress_policing_kpkts_burst=1000` > > Also, the parameters should be more inline with what the are. No need > to add kpkts in front of it. Other egress policers use things like > cir, max-rate, etc., without specifying the unit. Yes, I think this will be better. Examples: $ovs-vsctl set port vhost-user0 qos=@newqos -- --id=@newqos create qos type=kpkts-policer \ other-config:rate=123 other-config:burst=123 > > So you example would become: > > $ovs-vsctl set port vhost-user0 qos=@newqos -- > --id=@newqos create qos type=pps-policer \ > other-config:rate=123 other-config: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 | 21 +++ >> NEWS | 1 + >> lib/netdev-dpdk.c | 159 +++++++++++++++++++ >> tests/system-dpdk.at | 255 ++++++++++++++++++++++++++++++ >> vswitchd/vswitch.xml | 32 ++++ >> 5 files changed, 468 insertions(+) >> >> diff --git a/Documentation/topics/dpdk/qos.rst >> b/Documentation/topics/dpdk/qos.rst >> index a98ec672f..37f482cc5 100644 >> --- a/Documentation/topics/dpdk/qos.rst >> +++ b/Documentation/topics/dpdk/qos.rst >> @@ -36,6 +36,9 @@ QoS (Egress Policing) >> Single Queue Policer >> ~~~~~~~~~~~~~~~~~~~~ >> >> +Bytes Per Second 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:: >> @@ -52,6 +55,24 @@ To clear the QoS configuration from the port and >> ovsdb, run:: >> >> $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >> >> +Packets Per Second Policer >> +++++++++++++++++++++++++++ >> + >> +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 packets per second:: >> + >> + ovs-vsctl set port vhost-user0 qos=@newqos -- \ >> + --id=@newqos create qos type=kpkts-policer \ >> + other-config:kpkts_rate=1000 other-config:kpkts_burst=1000 > > I will only add this once, but if we move to pps-policer rather than > kpkts-policer we have to change the naming in the documentation and > troughout the code to make it consitant. > > This sounds obvious, but there are some confusing pieces of code in > OVS due to last minute naming change of the commands :) > >> +To examine the QoS configuration of the port, run:: >> + >> + $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0 >> + >> +To clear the QoS configuration from the port and ovsdb, run:: >> + >> + $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >> >> Multi Queue Policer >> ~~~~~~~~~~~~~~~~~~~ >> diff --git a/NEWS b/NEWS >> index b582bdbbc..430c3daaf 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -63,6 +63,7 @@ v3.2.0 - 17 Aug 2023 >> * 'ovs-appctl dpif-netdev/pmd-sleep-show' command was added to >> get the >> max sleep configuration of PMD thread cores. >> * Removed experimental tag from PMD load based sleeping. >> + * Added new Qos type 'pkts-policer' to support kilo >> packet-per-second policing. >> - Linux TC offload: >> * Add support for offloading VXLAN tunnels with the GBP >> extensions. >> - Python >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index bc8204f7e..c6a26dc7e 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 */ > > If UINT32_MAX / 1000 is what you need, why no just define it that way? > > #define MAX_KPKTS_PARAMETER UINT32_MAX / 1000 > >> + >> /* >> * need to reserve tons of extra space in the mbufs so we can align the >> * DMA addresses to 4KB. >> @@ -346,6 +350,7 @@ struct dpdk_qos_ops { >> /* dpdk_qos_ops for each type of user space QoS implementation. */ >> static const struct dpdk_qos_ops egress_policer_ops; >> static const struct dpdk_qos_ops trtcm_policer_ops; >> +static const struct dpdk_qos_ops kpkts_policer_ops; > > New name, pps_policer_ops. > >> >> /* >> * Array of dpdk_qos_ops, contains pointer to all supported QoS >> @@ -354,6 +359,7 @@ static const struct dpdk_qos_ops trtcm_policer_ops; >> static const struct dpdk_qos_ops *const qos_confs[] = { >> &egress_policer_ops, >> &trtcm_policer_ops, >> + &kpkts_policer_ops, >> NULL >> }; >> >> @@ -5571,6 +5577,159 @@ static const struct dpdk_qos_ops >> trtcm_policer_ops = { >> .qos_queue_dump_state_init = >> trtcm_policer_qos_queue_dump_state_init >> }; >> >> +/* kpkts-policer details */ >> +struct kpkts_policer { >> + struct qos_conf qos_conf; >> + struct token_bucket tb; >> + uint32_t kpkts_rate; >> + uint32_t kpkts_burst; > > These can just be rate and burst. > >> +}; >> + >> +static int >> +kpkts_policer_run_single_packet(struct token_bucket *tb, struct >> rte_mbuf **pkts, > > I know this name was copied from the other policer, but maybe it > should be renamed to better represent it's function. Somehting like: > > pps_policer_run_on_packet_batch()? > >> + int pkt_cnt, bool should_steal) >> +{ >> + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; >> + long long int now = time_msec(); >> + int i = 0, n = 0; >> + int cnt = 0; >> + >> + for (i = 0; i < pkt_cnt; i++) { >> + struct rte_mbuf *pkt = pkts[i]; >> + /* Handle current packet. */ >> + if (token_bucket_withdraw(tb, 1, now)) { > > I think this funciont can be optimized, as 'now' is not chaning for > this batch. So, if we fail one packet, the rest of the batch would > also fail. > > Maybe we can even add a new token_bucket_withdraw_max() like function > to get max buckets available giving the number you want. For example > you request 100 buckets, but if only 50 buckets are available it will > take 50 and return 50. Now you know the first 50 packets are a pass, > and another 50 need to be dropped. > > This will allow you to do something like: > > tokens = token_bucket_withdraw_max(pkt_cnt); > if (should_steal && tokens != pkt_cnt) { > rte_pktmbuf_free_bulk(pkts + tokens, pkt_cnt - tokens); > } > return tokens; > > This also make the token_bucket_withdraw() patch obsolete. > >> + /* Count passed packets. */ >> + if (cnt != i) { >> + pkts[cnt] = pkt; >> + } >> + cnt++; >> + } else if (should_steal) { > > See comment in patch 2. > >> + /* Count dropped packets. */ >> + should_steal_batch[n++] = pkt; >> + } >> + } >> + >> + if (n) { >> + rte_pktmbuf_free_bulk(should_steal_batch, n); >> + } >> + >> + return cnt; >> +} >> + Maybe the following code will work better. long long int now = time_msec(); int i = 0, n = 0; int cnt = 0; for (i = 0; i < pkt_cnt; i++) { if (token_bucket_withdraw(tb, 1, now)) { /* Count passed packets. */ cnt++; } else { /* The rest of packets should be dropped. */ if (should_steal) { /* Count dropped packets. */ n = pkt_cnt - cnt; } } } if (n) { rte_pktmbuf_free_bulk(&pkts[cnt], n); } return cnt; >> +static int >> +kpkts_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) { > > If you add the zero check for kpkts_rate here you can remove the > if/else below: > >> + return EINVAL; >> + } >> + >> + /* Rate in kilo-packets/second, bucket 1000 packets. >> + * msec * kilo-packets/sec = 1 packets. */ > > Be consistent with multi line comments. > >> + 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 EINVAL; >> + } >> + >> + return 0; > > So with this the function looks like this: > > static int > kpkts_policer_profile_config(struct token_bucket *tb, > uint32_t rate, uint32_t burst) > { > if (!rate || > rate > MAX_PPS_PARAMETER || > burst > MAX_PPS_PARAMETER) { > return EINVAL; > } > > /* > * Rate in kilo-packets/second, bucket 1000 packets. > * msec * kilo-packets/sec = 1 packets. > */ > token_bucket_init(tb, rate, burst * 1000); > return 0; > } > > >> +} >> + >> +static int >> +kpkts_policer_qos_construct(const struct smap *details, struct >> qos_conf **conf) >> +{ >> + uint32_t kpkts_rate, kpkts_burst; >> + struct kpkts_policer *policer; >> + int err; >> + >> + policer = xzalloc(sizeof *policer); >> + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0); >> + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0); >> + >> + /* >> + * Force to 0 if no rate specified, >> + * default to rate if burst is 0, >> + * else stick with user-specified value. >> + */ > > This copmments can be on two lines: > > /* > * Force to 0 if no rate specified, default to rate if burst is 0, > * else stick with user-specified value. > */ > >> + kpkts_burst = (!kpkts_rate ? 0 : !kpkts_burst ? kpkts_rate : >> kpkts_burst); > > I would move this is bit to be more clear: > > kpkts_burst = !kpkts_rate ? 0 : (!kpkts_burst ? kpkts_rate : > kpkts_burst); > > >> + qos_conf_init(&policer->qos_conf, &kpkts_policer_ops); >> + err = kpkts_policer_profile_config(&policer->tb, kpkts_rate, >> kpkts_burst); >> + if (!err) { >> + policer->kpkts_rate = kpkts_rate; >> + policer->kpkts_burst = kpkts_burst; > > Why do we not pass policer to kpkts_policer_profile_config() and we > can move this above part there also? > > I see that this is because the kpkts_policer_profile_config() function > is re-used in the ingress patch. > >> + >> + *conf = &policer->qos_conf; >> + } else { >> + VLOG_DBG("Could not create token bucket for egress policer"); >> + free(policer); >> + *conf = NULL; >> + } >> + >> + return err; >> +} >> + >> +static void >> +kpkts_policer_qos_destruct(struct qos_conf *conf) >> +{ >> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct >> kpkts_policer, >> + qos_conf); >> + >> + free(policer); >> +} >> + >> +static int >> +kpkts_policer_qos_get(const struct qos_conf *conf, struct smap >> *details) >> +{ >> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct >> kpkts_policer, >> + qos_conf); >> + >> + smap_add_format(details, "kpkts_rate", "%"PRIu32, >> policer->kpkts_rate); >> + smap_add_format(details, "kpkts_burst", "%"PRIu32, >> policer->kpkts_burst); >> + >> + return 0; >> +} >> + >> +static bool >> +kpkts_pkts_policer_qos_is_equal(const struct qos_conf *conf, >> + const struct smap *details) >> +{ >> + uint32_t rate, burst; >> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct >> kpkts_policer, >> + qos_conf); >> + >> + rate = smap_get_uint(details, "pkts_rate", 0); >> + burst = smap_get_uint(details, "pkts_burst", 0); > > I think you where using kpkts_? But this need to change anyway. > >> + >> + return (policer->tb.rate == rate && policer->tb.burst == burst); >> +} >> + >> +static int >> +kpkts_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int >> pkt_cnt, >> + bool should_steal) >> +{ >> + int cnt; >> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct >> kpkts_policer, >> + qos_conf); >> + >> + cnt = kpkts_policer_run_single_packet(&policer->tb, pkts, pkt_cnt, >> + should_steal); >> + >> + return cnt; >> +} >> + >> +static const struct dpdk_qos_ops kpkts_policer_ops = { >> + .qos_name = "kpkts-policer", >> + .qos_construct = kpkts_policer_qos_construct, >> + .qos_destruct = kpkts_policer_qos_destruct, >> + .qos_get = kpkts_policer_qos_get, >> + .qos_is_equal = kpkts_pkts_policer_qos_is_equal, >> + .qos_run = kpkts_policer_run >> +}; >> + >> static int >> dpdk_rx_steer_add_flow(struct netdev_dpdk *dev, >> const struct rte_flow_item items[], >> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >> index 0f58e8574..8b80a31e6 100644 >> --- a/tests/system-dpdk.at >> +++ b/tests/system-dpdk.at >> @@ -570,6 +570,261 @@ dnl >> -------------------------------------------------------------------------- >> >> >> >> +dnl >> -------------------------------------------------------------------------- >> +dnl QoS (kpkts) create delete vport port >> +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port]) > > I would include the policer name in full, so it will become: > > AT_SETUP([OVS-DPDK - QoS (pps-policer) 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 > > Missing dot at the end of the comment. True in other places also, so > please fix all instances. > >> +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=kpkts-policer >> other-config:kpkts_rate=123 other-config:kpkts_burst=456]) >> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], >> [], [stdout]) > > We should check for the exact output. > >> +sleep 2 > > Why a sleep we should avoid this. please use OVS_WAIT_UNTIL on the > below greps. > >> + >> +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]) > > Can we fold these two lines into one. > >> + >> +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 kpkts-policer on port dpdkvhostuserclient0: >> Invalid argument@d > > Why excluding this error message, it should not be generated here, or > do I miss something? > >> +])") >> +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=kpkts-policer >> other-config:kpkts_burst=123]) >> +sleep 2 >> + > See above test case > >> +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 >> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: >> Invalid argument@d > > Maybe we should also check above to make sure we see this error message. > >> +])") >> +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=kpkts-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]) > > What about the burst? As we no the full output, why not do it as part > of the first AT_CHECK instead of passing it to stdout. > >> + >> +dnl Check egress policer was deleted >> +QOS_UUID=`ovs-vsctl get port dpdkvhostuserclient0 qos` >> +AT_CHECK([ovs-vsctl set qos $QOS_UUID other_config:kpkts_rate=0]) >> +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 kpkts-policer on port dpdkvhostuserclient0: >> Invalid argument@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=kpkts-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 >> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: >> Invalid argument@d >> +])") >> +AT_CLEANUP >> +dnl >> -------------------------------------------------------------------------- >> + > > Add one more newline. As it looks like all have 3. > >> + >> +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=kpkts-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 >> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: >> Invalid argument@d >> +])") >> +AT_CLEANUP >> +dnl >> -------------------------------------------------------------------------- >> + > > Add one more newline. As it looks like all have 3. > >> + >> +dnl >> -------------------------------------------------------------------------- >> +dnl QoS (kpkts) testpmd flowgen test >> +AT_SETUP([OVS-DPDK - QoS (kpkts) police]) >> +AT_KEYWORDS([dpdk]) >> + >> +OVS_DPDK_PRE_CHECK() >> +AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null]) >> +OVS_DPDK_START([--no-pci]) >> + >> +dnl Find number of sockets >> +AT_CHECK([lscpu], [], [stdout]) >> +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while >> (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) >> + >> +dnl Add userspace bridge and attach it to OVS >> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 >> datapath_type=netdev]) >> + >> +dnl Parse log file >> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface >> dpdkvhostuser0 type=dpdkvhostuser], [], [stdout], [stderr]) >> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser1 -- set Interface >> dpdkvhostuser1 type=dpdkvhostuser], [], [stdout], [stderr]) >> +AT_CHECK([ovs-vsctl show], [], [stdout]) > > Why execute the show? Nothing is done with the output? > >> + >> +dnl Parse log file >> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) >> vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for >> vhost-user port dpdkvhostuser0" ovs-vswitchd.log], [], [stdout]) >> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding >> succeeded" ovs-vswitchd.log], [], [stdout]) >> + >> +dnl Parse log file >> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) >> vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser1 created for >> vhost-user port dpdkvhostuser1" ovs-vswitchd.log], [], [stdout]) >> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) binding >> succeeded" ovs-vswitchd.log], [], [stdout]) >> + >> +dnl Configure the same QoS for both ports. >> +AT_CHECK([ovs-vsctl set port dpdkvhostuser0 qos=@newqos -- >> --id=@newqos create qos type=kpkts-policer other-config:kpkts_rate=1 >> other-config:kpkts_burst=1], [0], [ignore]) >> + >> +dnl add flows, only send packets from dpdkvhostuser1 to dpdkvhostuser0. > > Start all commented with a Capital. > >> +AT_DATA([flows.txt], [dnl >> +priority=100,in_port=dpdkvhostuser1,ip,actions=dpdkvhostuser0 >> +]) >> + >> +AT_CHECK([ovs-ofctl del-flows br10]) >> +AT_CHECK([ovs-ofctl add-flows br10 flows.txt]) > > If it's a single flow just do it in the command itself no need to > create a flows.txt file. > >> + >> +dnl Execute testpmd in background >> +on_exit "pkill -f -x -9 'tail -f /dev/null'" >> +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" >> --no-pci\ >> + --vdev="net_virtio_user0,path=$OVS_RUNDIR/dpdkvhostuser0" \ >> + --vdev="net_virtio_user1,path=$OVS_RUNDIR/dpdkvhostuser1" \ >> + --single-file-segments -- --forward-mode=flowgen -a > >> $OVS_RUNDIR/testpmd-dpdkvhostuser.log 2>&1 & >> + >> +dnl sent packet 10 second. >> +AT_CHECK([sleep 10]) > > Do we need 10 seconds, can we make this shorter? > >> + >> +dnl Clean up the testpmd now >> +pkill -f -x -9 'tail -f /dev/null' >> + >> +dnl ---------------------- Forward statistics for port 0 >> ---------------------- >> +dnl RX-packets: 9911 RX-dropped: 0 RX-total: 9911 >> +dnl TX-packets: 15937632 TX-dropped: 226661984 TX-total: >> 242599616 >> +dnl >> ---------------------------------------------------------------------------- >> +port0_rx_packets=`cat testpmd-dpdkvhostuser.log | grep "Forward >> statistics for port 0" -A 1 | grep "RX-packets:" | awk '{print $2}'` >> +echo "port0_rx_packets=$port0_rx_packets" >> + >> +AT_CHECK([test $port0_rx_packets -lt 10500]) >> +AT_CHECK([test $port0_rx_packets -gt 9500]) >> + >> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ >> +\@dpdkvhostuser ports are considered deprecated; please migrate to >> dpdkvhostuserclient ports.@d > > Can we migrate to dpdkvhostuserclient ports? > >> +])") >> +AT_CLEANUP >> +dnl >> -------------------------------------------------------------------------- >> + >> + >> >> dnl >> -------------------------------------------------------------------------- >> dnl MTU increase phy port >> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >> index cfcde34ff..19fc13873 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -4874,6 +4874,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 >> type=patch options:peer=p1 \ >> created with the same <code>other_config</code> values as the >> physical port. >> </dd> >> + <dt><code>kpkts-policer</code></dt> >> + <dd> >> + A DPDK egress packet per second policer algorithm using >> the ovs >> + token bucket library. The token bucket library provides an >> + implementation which allows the policing of packets >> traffic. The >> + implementation in OVS essentially creates a single token >> bucket used >> + to police traffic. It should be noted that when the token >> bucket is >> + configured as part of QoS there will be a performance >> overhead as the >> + token bucket itself will consume CPU cycles in order to >> police >> + traffic. These CPU cycles ordinarily are used for packet >> proccessing. >> + As such the drop in performance will be noticed in terms >> of overall >> + aggregate traffic throughput. >> + </dd> >> </dl> >> </column> >> >> @@ -4966,6 +4979,25 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 >> type=patch options:peer=p1 \ >> </column> >> </group> >> >> + <group title="Configuration for kpkts-policer QoS"> >> + <p> >> + <ref table="QoS"/> <ref table="QoS" column="type"/> >> + <code>kpkts-policer</code> provides egress pkts policing for >> userspace >> + port types with DPDK. >> + >> + It has the following key-value pairs defined. >> + </p> >> + >> + <column name="other_config" key="kpkts_rate" type='{"type": >> "integer"}'> >> + The Kilo Packets Per Second (kpps) 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"}'> >> + The Packets Per Second Burst Size is measured in count and >> represents a >> + token bucket. >> + </column> >> + </group> >> + >> <group title="Configuration for linux-sfq"> >> <p> >> The <code>linux-sfq</code> QoS supports the following >> key-value pairs: >> -- >> 2.39.3 >
On 3 Dec 2023, at 15:19, miter wrote: > Hi Eelco, > > > Some comments below. > > > On 9/19/2023 10:58 PM, Eelco Chaudron wrote: >> On 26 Aug 2023, at 8:01, 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 a new egress Qos type called 'kpkts-policer'. >>> the policer police the kilo-packet per second at which the token bucket >>> be updated by 'kpkts_rate'. and the policer's burst size is defined by >>> 'kpkts_burst'. >>> >>> Examples: >>> $ovs-vsctl set port vhost-user0 qos=@newqos -- >>> --id=@newqos create qos type=kpkts-policer \ >>> other-config:kpkts_rate=123 other-config:kpkts_burst=123 >> >> I think the name of the policer is odd, it should reflect the actual policer type. >> Maybe it should be called pps-policer? > > Becouse the Ingress Policing use "kpkts" to name. In order to keep ingress and egress more symmetric. > $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_rate=1000 ingress_policing_kpkts_burst=1000` Yes, it will be more inline, but less clear to me. I would still prefer to move it to pps-policer. Others? >> >> Also, the parameters should be more inline with what the are. No need to add kpkts in front of it. Other egress policers use things like cir, max-rate, etc., without specifying the unit. > Yes, I think this will be better. > > Examples: > $ovs-vsctl set port vhost-user0 qos=@newqos -- > --id=@newqos create qos type=kpkts-policer \ > other-config:rate=123 other-config:burst=123 > >> >> So you example would become: >> >> $ovs-vsctl set port vhost-user0 qos=@newqos -- >> --id=@newqos create qos type=pps-policer \ >> other-config:rate=123 other-config: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 | 21 +++ >>> NEWS | 1 + >>> lib/netdev-dpdk.c | 159 +++++++++++++++++++ >>> tests/system-dpdk.at | 255 ++++++++++++++++++++++++++++++ >>> vswitchd/vswitch.xml | 32 ++++ >>> 5 files changed, 468 insertions(+) >>> >>> diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst >>> index a98ec672f..37f482cc5 100644 >>> --- a/Documentation/topics/dpdk/qos.rst >>> +++ b/Documentation/topics/dpdk/qos.rst >>> @@ -36,6 +36,9 @@ QoS (Egress Policing) >>> Single Queue Policer >>> ~~~~~~~~~~~~~~~~~~~~ >>> >>> +Bytes Per Second 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:: >>> @@ -52,6 +55,24 @@ To clear the QoS configuration from the port and ovsdb, run:: >>> >>> $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >>> >>> +Packets Per Second Policer >>> +++++++++++++++++++++++++++ >>> + >>> +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 packets per second:: >>> + >>> + ovs-vsctl set port vhost-user0 qos=@newqos -- \ >>> + --id=@newqos create qos type=kpkts-policer \ >>> + other-config:kpkts_rate=1000 other-config:kpkts_burst=1000 >> >> I will only add this once, but if we move to pps-policer rather than kpkts-policer we have to change the naming in the documentation and troughout the code to make it consitant. >> >> This sounds obvious, but there are some confusing pieces of code in OVS due to last minute naming change of the commands :) >> >>> +To examine the QoS configuration of the port, run:: >>> + >>> + $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0 >>> + >>> +To clear the QoS configuration from the port and ovsdb, run:: >>> + >>> + $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >>> >>> Multi Queue Policer >>> ~~~~~~~~~~~~~~~~~~~ >>> diff --git a/NEWS b/NEWS >>> index b582bdbbc..430c3daaf 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -63,6 +63,7 @@ v3.2.0 - 17 Aug 2023 >>> * 'ovs-appctl dpif-netdev/pmd-sleep-show' command was added to get the >>> max sleep configuration of PMD thread cores. >>> * Removed experimental tag from PMD load based sleeping. >>> + * Added new Qos type 'pkts-policer' to support kilo packet-per-second policing. >>> - Linux TC offload: >>> * Add support for offloading VXLAN tunnels with the GBP extensions. >>> - Python >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index bc8204f7e..c6a26dc7e 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 */ >> >> If UINT32_MAX / 1000 is what you need, why no just define it that way? >> >> #define MAX_KPKTS_PARAMETER UINT32_MAX / 1000 >> >>> + >>> /* >>> * need to reserve tons of extra space in the mbufs so we can align the >>> * DMA addresses to 4KB. >>> @@ -346,6 +350,7 @@ struct dpdk_qos_ops { >>> /* dpdk_qos_ops for each type of user space QoS implementation. */ >>> static const struct dpdk_qos_ops egress_policer_ops; >>> static const struct dpdk_qos_ops trtcm_policer_ops; >>> +static const struct dpdk_qos_ops kpkts_policer_ops; >> >> New name, pps_policer_ops. >> >>> >>> /* >>> * Array of dpdk_qos_ops, contains pointer to all supported QoS >>> @@ -354,6 +359,7 @@ static const struct dpdk_qos_ops trtcm_policer_ops; >>> static const struct dpdk_qos_ops *const qos_confs[] = { >>> &egress_policer_ops, >>> &trtcm_policer_ops, >>> + &kpkts_policer_ops, >>> NULL >>> }; >>> >>> @@ -5571,6 +5577,159 @@ static const struct dpdk_qos_ops trtcm_policer_ops = { >>> .qos_queue_dump_state_init = trtcm_policer_qos_queue_dump_state_init >>> }; >>> >>> +/* kpkts-policer details */ >>> +struct kpkts_policer { >>> + struct qos_conf qos_conf; >>> + struct token_bucket tb; >>> + uint32_t kpkts_rate; >>> + uint32_t kpkts_burst; >> >> These can just be rate and burst. >> >>> +}; >>> + >>> +static int >>> +kpkts_policer_run_single_packet(struct token_bucket *tb, struct rte_mbuf **pkts, >> >> I know this name was copied from the other policer, but maybe it should be renamed to better represent it's function. Somehting like: >> >> pps_policer_run_on_packet_batch()? >> >>> + int pkt_cnt, bool should_steal) >>> +{ >>> + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; >>> + long long int now = time_msec(); >>> + int i = 0, n = 0; >>> + int cnt = 0; >>> + >>> + for (i = 0; i < pkt_cnt; i++) { >>> + struct rte_mbuf *pkt = pkts[i]; >>> + /* Handle current packet. */ >>> + if (token_bucket_withdraw(tb, 1, now)) { >> >> I think this funciont can be optimized, as 'now' is not chaning for this batch. So, if we fail one packet, the rest of the batch would also fail. >> >> Maybe we can even add a new token_bucket_withdraw_max() like function to get max buckets available giving the number you want. For example you request 100 buckets, but if only 50 buckets are available it will take 50 and return 50. Now you know the first 50 packets are a pass, and another 50 need to be dropped. >> >> This will allow you to do something like: >> >> tokens = token_bucket_withdraw_max(pkt_cnt); >> if (should_steal && tokens != pkt_cnt) { >> rte_pktmbuf_free_bulk(pkts + tokens, pkt_cnt - tokens); >> } >> return tokens; >> >> This also make the token_bucket_withdraw() patch obsolete. >> >>> + /* Count passed packets. */ >>> + if (cnt != i) { >>> + pkts[cnt] = pkt; >>> + } >>> + cnt++; >>> + } else if (should_steal) { >> >> See comment in patch 2. >> >>> + /* Count dropped packets. */ >>> + should_steal_batch[n++] = pkt; >>> + } >>> + } >>> + >>> + if (n) { >>> + rte_pktmbuf_free_bulk(should_steal_batch, n); >>> + } >>> + >>> + return cnt; >>> +} >>> + > > Maybe the following code will work better. I still would prefer the token_bucket_withdraw_max() idea, as it avoids the loop, and it’s more clean. > long long int now = time_msec(); > int i = 0, n = 0; > int cnt = 0; > > for (i = 0; i < pkt_cnt; i++) { > if (token_bucket_withdraw(tb, 1, now)) { > /* Count passed packets. */ > cnt++; > } else { > /* The rest of packets should be dropped. */ > if (should_steal) { > /* Count dropped packets. */ > n = pkt_cnt - cnt; > } > } > } > > if (n) { > rte_pktmbuf_free_bulk(&pkts[cnt], n); > } > > return cnt; > > >>> +static int >>> +kpkts_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) { >> >> If you add the zero check for kpkts_rate here you can remove the if/else below: >> >>> + return EINVAL; >>> + } >>> + >>> + /* Rate in kilo-packets/second, bucket 1000 packets. >>> + * msec * kilo-packets/sec = 1 packets. */ >> >> Be consistent with multi line comments. >> >>> + 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 EINVAL; >>> + } >>> + >>> + return 0; >> >> So with this the function looks like this: >> >> static int >> kpkts_policer_profile_config(struct token_bucket *tb, >> uint32_t rate, uint32_t burst) >> { >> if (!rate || >> rate > MAX_PPS_PARAMETER || >> burst > MAX_PPS_PARAMETER) { >> return EINVAL; >> } >> >> /* >> * Rate in kilo-packets/second, bucket 1000 packets. >> * msec * kilo-packets/sec = 1 packets. >> */ >> token_bucket_init(tb, rate, burst * 1000); >> return 0; >> } >> >> >>> +} >>> + >>> +static int >>> +kpkts_policer_qos_construct(const struct smap *details, struct qos_conf **conf) >>> +{ >>> + uint32_t kpkts_rate, kpkts_burst; >>> + struct kpkts_policer *policer; >>> + int err; >>> + >>> + policer = xzalloc(sizeof *policer); >>> + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0); >>> + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0); >>> + >>> + /* >>> + * Force to 0 if no rate specified, >>> + * default to rate if burst is 0, >>> + * else stick with user-specified value. >>> + */ >> >> This copmments can be on two lines: >> >> /* >> * Force to 0 if no rate specified, default to rate if burst is 0, >> * else stick with user-specified value. >> */ >> >>> + kpkts_burst = (!kpkts_rate ? 0 : !kpkts_burst ? kpkts_rate : kpkts_burst); >> >> I would move this is bit to be more clear: >> >> kpkts_burst = !kpkts_rate ? 0 : (!kpkts_burst ? kpkts_rate : kpkts_burst); >> >> >>> + qos_conf_init(&policer->qos_conf, &kpkts_policer_ops); >>> + err = kpkts_policer_profile_config(&policer->tb, kpkts_rate, kpkts_burst); >>> + if (!err) { >>> + policer->kpkts_rate = kpkts_rate; >>> + policer->kpkts_burst = kpkts_burst; >> >> Why do we not pass policer to kpkts_policer_profile_config() and we can move this above part there also? >> >> I see that this is because the kpkts_policer_profile_config() function is re-used in the ingress patch. >> >>> + >>> + *conf = &policer->qos_conf; >>> + } else { >>> + VLOG_DBG("Could not create token bucket for egress policer"); >>> + free(policer); >>> + *conf = NULL; >>> + } >>> + >>> + return err; >>> +} >>> + >>> +static void >>> +kpkts_policer_qos_destruct(struct qos_conf *conf) >>> +{ >>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>> + qos_conf); >>> + >>> + free(policer); >>> +} >>> + >>> +static int >>> +kpkts_policer_qos_get(const struct qos_conf *conf, struct smap *details) >>> +{ >>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>> + qos_conf); >>> + >>> + smap_add_format(details, "kpkts_rate", "%"PRIu32, policer->kpkts_rate); >>> + smap_add_format(details, "kpkts_burst", "%"PRIu32, policer->kpkts_burst); >>> + >>> + return 0; >>> +} >>> + >>> +static bool >>> +kpkts_pkts_policer_qos_is_equal(const struct qos_conf *conf, >>> + const struct smap *details) >>> +{ >>> + uint32_t rate, burst; >>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>> + qos_conf); >>> + >>> + rate = smap_get_uint(details, "pkts_rate", 0); >>> + burst = smap_get_uint(details, "pkts_burst", 0); >> >> I think you where using kpkts_? But this need to change anyway. >> >>> + >>> + return (policer->tb.rate == rate && policer->tb.burst == burst); >>> +} >>> + >>> +static int >>> +kpkts_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt, >>> + bool should_steal) >>> +{ >>> + int cnt; >>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>> + qos_conf); >>> + >>> + cnt = kpkts_policer_run_single_packet(&policer->tb, pkts, pkt_cnt, >>> + should_steal); >>> + >>> + return cnt; >>> +} >>> + >>> +static const struct dpdk_qos_ops kpkts_policer_ops = { >>> + .qos_name = "kpkts-policer", >>> + .qos_construct = kpkts_policer_qos_construct, >>> + .qos_destruct = kpkts_policer_qos_destruct, >>> + .qos_get = kpkts_policer_qos_get, >>> + .qos_is_equal = kpkts_pkts_policer_qos_is_equal, >>> + .qos_run = kpkts_policer_run >>> +}; >>> + >>> static int >>> dpdk_rx_steer_add_flow(struct netdev_dpdk *dev, >>> const struct rte_flow_item items[], >>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >>> index 0f58e8574..8b80a31e6 100644 >>> --- a/tests/system-dpdk.at >>> +++ b/tests/system-dpdk.at >>> @@ -570,6 +570,261 @@ dnl -------------------------------------------------------------------------- >>> >>> >>> >>> +dnl -------------------------------------------------------------------------- >>> +dnl QoS (kpkts) create delete vport port >>> +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port]) >> >> I would include the policer name in full, so it will become: >> >> AT_SETUP([OVS-DPDK - QoS (pps-policer) 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 >> >> Missing dot at the end of the comment. True in other places also, so please fix all instances. >> >>> +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=kpkts-policer other-config:kpkts_rate=123 other-config:kpkts_burst=456]) >>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout]) >> >> We should check for the exact output. >> >>> +sleep 2 >> >> Why a sleep we should avoid this. please use OVS_WAIT_UNTIL on the below greps. >> >>> + >>> +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]) >> >> Can we fold these two lines into one. >> >>> + >>> +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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >> >> Why excluding this error message, it should not be generated here, or do I miss something? >> >>> +])") >>> +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=kpkts-policer other-config:kpkts_burst=123]) >>> +sleep 2 >>> + >> See above test case >> >>> +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 >>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >> >> Maybe we should also check above to make sure we see this error message. >> >>> +])") >>> +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=kpkts-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]) >> >> What about the burst? As we no the full output, why not do it as part of the first AT_CHECK instead of passing it to stdout. >> >>> + >>> +dnl Check egress policer was deleted >>> +QOS_UUID=`ovs-vsctl get port dpdkvhostuserclient0 qos` >>> +AT_CHECK([ovs-vsctl set qos $QOS_UUID other_config:kpkts_rate=0]) >>> +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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 >>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>> +])") >>> +AT_CLEANUP >>> +dnl -------------------------------------------------------------------------- >>> + >> >> Add one more newline. As it looks like all have 3. >> >>> + >>> +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=kpkts-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 >>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>> +])") >>> +AT_CLEANUP >>> +dnl -------------------------------------------------------------------------- >>> + >> >> Add one more newline. As it looks like all have 3. >> >>> + >>> +dnl -------------------------------------------------------------------------- >>> +dnl QoS (kpkts) testpmd flowgen test >>> +AT_SETUP([OVS-DPDK - QoS (kpkts) police]) >>> +AT_KEYWORDS([dpdk]) >>> + >>> +OVS_DPDK_PRE_CHECK() >>> +AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null]) >>> +OVS_DPDK_START([--no-pci]) >>> + >>> +dnl Find number of sockets >>> +AT_CHECK([lscpu], [], [stdout]) >>> +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) >>> + >>> +dnl Add userspace bridge and attach it to OVS >>> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev]) >>> + >>> +dnl Parse log file >>> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface dpdkvhostuser0 type=dpdkvhostuser], [], [stdout], [stderr]) >>> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser1 -- set Interface dpdkvhostuser1 type=dpdkvhostuser], [], [stdout], [stderr]) >>> +AT_CHECK([ovs-vsctl show], [], [stdout]) >> >> Why execute the show? Nothing is done with the output? >> >>> + >>> +dnl Parse log file >>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >>> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for vhost-user port dpdkvhostuser0" ovs-vswitchd.log], [], [stdout]) >>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding succeeded" ovs-vswitchd.log], [], [stdout]) >>> + >>> +dnl Parse log file >>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >>> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser1 created for vhost-user port dpdkvhostuser1" ovs-vswitchd.log], [], [stdout]) >>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) binding succeeded" ovs-vswitchd.log], [], [stdout]) >>> + >>> +dnl Configure the same QoS for both ports. >>> +AT_CHECK([ovs-vsctl set port dpdkvhostuser0 qos=@newqos -- --id=@newqos create qos type=kpkts-policer other-config:kpkts_rate=1 other-config:kpkts_burst=1], [0], [ignore]) >>> + >>> +dnl add flows, only send packets from dpdkvhostuser1 to dpdkvhostuser0. >> >> Start all commented with a Capital. >> >>> +AT_DATA([flows.txt], [dnl >>> +priority=100,in_port=dpdkvhostuser1,ip,actions=dpdkvhostuser0 >>> +]) >>> + >>> +AT_CHECK([ovs-ofctl del-flows br10]) >>> +AT_CHECK([ovs-ofctl add-flows br10 flows.txt]) >> >> If it's a single flow just do it in the command itself no need to create a flows.txt file. >> >>> + >>> +dnl Execute testpmd in background >>> +on_exit "pkill -f -x -9 'tail -f /dev/null'" >>> +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\ >>> + --vdev="net_virtio_user0,path=$OVS_RUNDIR/dpdkvhostuser0" \ >>> + --vdev="net_virtio_user1,path=$OVS_RUNDIR/dpdkvhostuser1" \ >>> + --single-file-segments -- --forward-mode=flowgen -a > $OVS_RUNDIR/testpmd-dpdkvhostuser.log 2>&1 & >>> + >>> +dnl sent packet 10 second. >>> +AT_CHECK([sleep 10]) >> >> Do we need 10 seconds, can we make this shorter? >> >>> + >>> +dnl Clean up the testpmd now >>> +pkill -f -x -9 'tail -f /dev/null' >>> + >>> +dnl ---------------------- Forward statistics for port 0 ---------------------- >>> +dnl RX-packets: 9911 RX-dropped: 0 RX-total: 9911 >>> +dnl TX-packets: 15937632 TX-dropped: 226661984 TX-total: 242599616 >>> +dnl ---------------------------------------------------------------------------- >>> +port0_rx_packets=`cat testpmd-dpdkvhostuser.log | grep "Forward statistics for port 0" -A 1 | grep "RX-packets:" | awk '{print $2}'` >>> +echo "port0_rx_packets=$port0_rx_packets" >>> + >>> +AT_CHECK([test $port0_rx_packets -lt 10500]) >>> +AT_CHECK([test $port0_rx_packets -gt 9500]) >>> + >>> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ >>> +\@dpdkvhostuser ports are considered deprecated; please migrate to dpdkvhostuserclient ports.@d >> >> Can we migrate to dpdkvhostuserclient ports? >> >>> +])") >>> +AT_CLEANUP >>> +dnl -------------------------------------------------------------------------- >>> + >>> + >>> >>> dnl -------------------------------------------------------------------------- >>> dnl MTU increase phy port >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>> index cfcde34ff..19fc13873 100644 >>> --- a/vswitchd/vswitch.xml >>> +++ b/vswitchd/vswitch.xml >>> @@ -4874,6 +4874,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>> created with the same <code>other_config</code> values as the >>> physical port. >>> </dd> >>> + <dt><code>kpkts-policer</code></dt> >>> + <dd> >>> + A DPDK egress packet per second policer algorithm using the ovs >>> + token bucket library. The token bucket library provides an >>> + implementation which allows the policing of packets traffic. The >>> + implementation in OVS essentially creates a single token bucket used >>> + to police traffic. It should be noted that when the token bucket is >>> + configured as part of QoS there will be a performance overhead as the >>> + token bucket itself will consume CPU cycles in order to police >>> + traffic. These CPU cycles ordinarily are used for packet proccessing. >>> + As such the drop in performance will be noticed in terms of overall >>> + aggregate traffic throughput. >>> + </dd> >>> </dl> >>> </column> >>> >>> @@ -4966,6 +4979,25 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>> </column> >>> </group> >>> >>> + <group title="Configuration for kpkts-policer QoS"> >>> + <p> >>> + <ref table="QoS"/> <ref table="QoS" column="type"/> >>> + <code>kpkts-policer</code> provides egress pkts policing for userspace >>> + port types with DPDK. >>> + >>> + It has the following key-value pairs defined. >>> + </p> >>> + >>> + <column name="other_config" key="kpkts_rate" type='{"type": "integer"}'> >>> + The Kilo Packets Per Second (kpps) 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"}'> >>> + The Packets Per Second Burst Size is measured in count and represents a >>> + token bucket. >>> + </column> >>> + </group> >>> + >>> <group title="Configuration for linux-sfq"> >>> <p> >>> The <code>linux-sfq</code> QoS supports the following key-value pairs: >>> -- >>> 2.39.3 >> > -- > Best regards, Huang Lin.
Hi Eelco, On 12/4/2023 5:01 PM, Eelco Chaudron wrote: > > On 3 Dec 2023, at 15:19, miter wrote: > >> Hi Eelco, >> >> >> Some comments below. >> >> >> On 9/19/2023 10:58 PM, Eelco Chaudron wrote: >>> On 26 Aug 2023, at 8:01, 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 a new egress Qos type called 'kpkts-policer'. >>>> the policer police the kilo-packet per second at which the token bucket >>>> be updated by 'kpkts_rate'. and the policer's burst size is defined by >>>> 'kpkts_burst'. >>>> >>>> Examples: >>>> $ovs-vsctl set port vhost-user0 qos=@newqos -- >>>> --id=@newqos create qos type=kpkts-policer \ >>>> other-config:kpkts_rate=123 other-config:kpkts_burst=123 >>> I think the name of the policer is odd, it should reflect the actual policer type. >>> Maybe it should be called pps-policer? >> Becouse the Ingress Policing use "kpkts" to name. In order to keep ingress and egress more symmetric. >> $ ovs-vsctl set interface dpdk0 ingress_policing_kpkts_rate=1000 ingress_policing_kpkts_burst=1000` > Yes, it will be more inline, but less clear to me. I would still prefer to move it to pps-policer. Others? Applied. >>> Also, the parameters should be more inline with what the are. No need to add kpkts in front of it. Other egress policers use things like cir, max-rate, etc., without specifying the unit. >> Yes, I think this will be better. >> >> Examples: >> $ovs-vsctl set port vhost-user0 qos=@newqos -- >> --id=@newqos create qos type=kpkts-policer \ >> other-config:rate=123 other-config:burst=123 >> >>> So you example would become: >>> >>> $ovs-vsctl set port vhost-user0 qos=@newqos -- >>> --id=@newqos create qos type=pps-policer \ >>> other-config:rate=123 other-config: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 | 21 +++ >>>> NEWS | 1 + >>>> lib/netdev-dpdk.c | 159 +++++++++++++++++++ >>>> tests/system-dpdk.at | 255 ++++++++++++++++++++++++++++++ >>>> vswitchd/vswitch.xml | 32 ++++ >>>> 5 files changed, 468 insertions(+) >>>> >>>> diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst >>>> index a98ec672f..37f482cc5 100644 >>>> --- a/Documentation/topics/dpdk/qos.rst >>>> +++ b/Documentation/topics/dpdk/qos.rst >>>> @@ -36,6 +36,9 @@ QoS (Egress Policing) >>>> Single Queue Policer >>>> ~~~~~~~~~~~~~~~~~~~~ >>>> >>>> +Bytes Per Second 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:: >>>> @@ -52,6 +55,24 @@ To clear the QoS configuration from the port and ovsdb, run:: >>>> >>>> $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >>>> >>>> +Packets Per Second Policer >>>> +++++++++++++++++++++++++++ >>>> + >>>> +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 packets per second:: >>>> + >>>> + ovs-vsctl set port vhost-user0 qos=@newqos -- \ >>>> + --id=@newqos create qos type=kpkts-policer \ >>>> + other-config:kpkts_rate=1000 other-config:kpkts_burst=1000 >>> I will only add this once, but if we move to pps-policer rather than kpkts-policer we have to change the naming in the documentation and troughout the code to make it consitant. >>> >>> This sounds obvious, but there are some confusing pieces of code in OVS due to last minute naming change of the commands :) >>> >>>> +To examine the QoS configuration of the port, run:: >>>> + >>>> + $ ovs-appctl -t ovs-vswitchd qos/show vhost-user0 >>>> + >>>> +To clear the QoS configuration from the port and ovsdb, run:: >>>> + >>>> + $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos >>>> >>>> Multi Queue Policer >>>> ~~~~~~~~~~~~~~~~~~~ >>>> diff --git a/NEWS b/NEWS >>>> index b582bdbbc..430c3daaf 100644 >>>> --- a/NEWS >>>> +++ b/NEWS >>>> @@ -63,6 +63,7 @@ v3.2.0 - 17 Aug 2023 >>>> * 'ovs-appctl dpif-netdev/pmd-sleep-show' command was added to get the >>>> max sleep configuration of PMD thread cores. >>>> * Removed experimental tag from PMD load based sleeping. >>>> + * Added new Qos type 'pkts-policer' to support kilo packet-per-second policing. >>>> - Linux TC offload: >>>> * Add support for offloading VXLAN tunnels with the GBP extensions. >>>> - Python >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>>> index bc8204f7e..c6a26dc7e 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 */ >>> If UINT32_MAX / 1000 is what you need, why no just define it that way? >>> >>> #define MAX_KPKTS_PARAMETER UINT32_MAX / 1000 >>> >>>> + >>>> /* >>>> * need to reserve tons of extra space in the mbufs so we can align the >>>> * DMA addresses to 4KB. >>>> @@ -346,6 +350,7 @@ struct dpdk_qos_ops { >>>> /* dpdk_qos_ops for each type of user space QoS implementation. */ >>>> static const struct dpdk_qos_ops egress_policer_ops; >>>> static const struct dpdk_qos_ops trtcm_policer_ops; >>>> +static const struct dpdk_qos_ops kpkts_policer_ops; >>> New name, pps_policer_ops. >>> >>>> /* >>>> * Array of dpdk_qos_ops, contains pointer to all supported QoS >>>> @@ -354,6 +359,7 @@ static const struct dpdk_qos_ops trtcm_policer_ops; >>>> static const struct dpdk_qos_ops *const qos_confs[] = { >>>> &egress_policer_ops, >>>> &trtcm_policer_ops, >>>> + &kpkts_policer_ops, >>>> NULL >>>> }; >>>> >>>> @@ -5571,6 +5577,159 @@ static const struct dpdk_qos_ops trtcm_policer_ops = { >>>> .qos_queue_dump_state_init = trtcm_policer_qos_queue_dump_state_init >>>> }; >>>> >>>> +/* kpkts-policer details */ >>>> +struct kpkts_policer { >>>> + struct qos_conf qos_conf; >>>> + struct token_bucket tb; >>>> + uint32_t kpkts_rate; >>>> + uint32_t kpkts_burst; >>> These can just be rate and burst. >>> >>>> +}; >>>> + >>>> +static int >>>> +kpkts_policer_run_single_packet(struct token_bucket *tb, struct rte_mbuf **pkts, >>> I know this name was copied from the other policer, but maybe it should be renamed to better represent it's function. Somehting like: >>> >>> pps_policer_run_on_packet_batch()? >>> >>>> + int pkt_cnt, bool should_steal) >>>> +{ >>>> + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; >>>> + long long int now = time_msec(); >>>> + int i = 0, n = 0; >>>> + int cnt = 0; >>>> + >>>> + for (i = 0; i < pkt_cnt; i++) { >>>> + struct rte_mbuf *pkt = pkts[i]; >>>> + /* Handle current packet. */ >>>> + if (token_bucket_withdraw(tb, 1, now)) { >>> I think this funciont can be optimized, as 'now' is not chaning for this batch. So, if we fail one packet, the rest of the batch would also fail. >>> >>> Maybe we can even add a new token_bucket_withdraw_max() like function to get max buckets available giving the number you want. For example you request 100 buckets, but if only 50 buckets are available it will take 50 and return 50. Now you know the first 50 packets are a pass, and another 50 need to be dropped. >>> >>> This will allow you to do something like: >>> >>> tokens = token_bucket_withdraw_max(pkt_cnt); >>> if (should_steal && tokens != pkt_cnt) { >>> rte_pktmbuf_free_bulk(pkts + tokens, pkt_cnt - tokens); >>> } >>> return tokens; >>> >>> This also make the token_bucket_withdraw() patch obsolete. >>> >>>> + /* Count passed packets. */ >>>> + if (cnt != i) { >>>> + pkts[cnt] = pkt; >>>> + } >>>> + cnt++; >>>> + } else if (should_steal) { >>> See comment in patch 2. >>> >>>> + /* Count dropped packets. */ >>>> + should_steal_batch[n++] = pkt; >>>> + } >>>> + } >>>> + >>>> + if (n) { >>>> + rte_pktmbuf_free_bulk(should_steal_batch, n); >>>> + } >>>> + >>>> + return cnt; >>>> +} >>>> + >> Maybe the following code will work better. > I still would prefer the token_bucket_withdraw_max() idea, as it avoids the loop, and it’s more clean. Applied. >> long long int now = time_msec(); >> int i = 0, n = 0; >> int cnt = 0; >> >> for (i = 0; i < pkt_cnt; i++) { >> if (token_bucket_withdraw(tb, 1, now)) { >> /* Count passed packets. */ >> cnt++; >> } else { >> /* The rest of packets should be dropped. */ >> if (should_steal) { >> /* Count dropped packets. */ >> n = pkt_cnt - cnt; >> } >> } >> } >> >> if (n) { >> rte_pktmbuf_free_bulk(&pkts[cnt], n); >> } >> >> return cnt; >> >> >>>> +static int >>>> +kpkts_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) { >>> If you add the zero check for kpkts_rate here you can remove the if/else below: >>> >>>> + return EINVAL; >>>> + } >>>> + >>>> + /* Rate in kilo-packets/second, bucket 1000 packets. >>>> + * msec * kilo-packets/sec = 1 packets. */ >>> Be consistent with multi line comments. >>> >>>> + 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 EINVAL; >>>> + } >>>> + >>>> + return 0; >>> So with this the function looks like this: >>> >>> static int >>> kpkts_policer_profile_config(struct token_bucket *tb, >>> uint32_t rate, uint32_t burst) >>> { >>> if (!rate || >>> rate > MAX_PPS_PARAMETER || >>> burst > MAX_PPS_PARAMETER) { >>> return EINVAL; >>> } >>> >>> /* >>> * Rate in kilo-packets/second, bucket 1000 packets. >>> * msec * kilo-packets/sec = 1 packets. >>> */ >>> token_bucket_init(tb, rate, burst * 1000); >>> return 0; >>> } >>> >>> >>>> +} >>>> + >>>> +static int >>>> +kpkts_policer_qos_construct(const struct smap *details, struct qos_conf **conf) >>>> +{ >>>> + uint32_t kpkts_rate, kpkts_burst; >>>> + struct kpkts_policer *policer; >>>> + int err; >>>> + >>>> + policer = xzalloc(sizeof *policer); >>>> + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0); >>>> + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0); >>>> + >>>> + /* >>>> + * Force to 0 if no rate specified, >>>> + * default to rate if burst is 0, >>>> + * else stick with user-specified value. >>>> + */ >>> This copmments can be on two lines: >>> >>> /* >>> * Force to 0 if no rate specified, default to rate if burst is 0, >>> * else stick with user-specified value. >>> */ >>> >>>> + kpkts_burst = (!kpkts_rate ? 0 : !kpkts_burst ? kpkts_rate : kpkts_burst); >>> I would move this is bit to be more clear: >>> >>> kpkts_burst = !kpkts_rate ? 0 : (!kpkts_burst ? kpkts_rate : kpkts_burst); >>> >>> >>>> + qos_conf_init(&policer->qos_conf, &kpkts_policer_ops); >>>> + err = kpkts_policer_profile_config(&policer->tb, kpkts_rate, kpkts_burst); >>>> + if (!err) { >>>> + policer->kpkts_rate = kpkts_rate; >>>> + policer->kpkts_burst = kpkts_burst; >>> Why do we not pass policer to kpkts_policer_profile_config() and we can move this above part there also? >>> >>> I see that this is because the kpkts_policer_profile_config() function is re-used in the ingress patch. >>> >>>> + >>>> + *conf = &policer->qos_conf; >>>> + } else { >>>> + VLOG_DBG("Could not create token bucket for egress policer"); >>>> + free(policer); >>>> + *conf = NULL; >>>> + } >>>> + >>>> + return err; >>>> +} >>>> + >>>> +static void >>>> +kpkts_policer_qos_destruct(struct qos_conf *conf) >>>> +{ >>>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>>> + qos_conf); >>>> + >>>> + free(policer); >>>> +} >>>> + >>>> +static int >>>> +kpkts_policer_qos_get(const struct qos_conf *conf, struct smap *details) >>>> +{ >>>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>>> + qos_conf); >>>> + >>>> + smap_add_format(details, "kpkts_rate", "%"PRIu32, policer->kpkts_rate); >>>> + smap_add_format(details, "kpkts_burst", "%"PRIu32, policer->kpkts_burst); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static bool >>>> +kpkts_pkts_policer_qos_is_equal(const struct qos_conf *conf, >>>> + const struct smap *details) >>>> +{ >>>> + uint32_t rate, burst; >>>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>>> + qos_conf); >>>> + >>>> + rate = smap_get_uint(details, "pkts_rate", 0); >>>> + burst = smap_get_uint(details, "pkts_burst", 0); >>> I think you where using kpkts_? But this need to change anyway. >>> >>>> + >>>> + return (policer->tb.rate == rate && policer->tb.burst == burst); >>>> +} >>>> + >>>> +static int >>>> +kpkts_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt, >>>> + bool should_steal) >>>> +{ >>>> + int cnt; >>>> + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, >>>> + qos_conf); >>>> + >>>> + cnt = kpkts_policer_run_single_packet(&policer->tb, pkts, pkt_cnt, >>>> + should_steal); >>>> + >>>> + return cnt; >>>> +} >>>> + >>>> +static const struct dpdk_qos_ops kpkts_policer_ops = { >>>> + .qos_name = "kpkts-policer", >>>> + .qos_construct = kpkts_policer_qos_construct, >>>> + .qos_destruct = kpkts_policer_qos_destruct, >>>> + .qos_get = kpkts_policer_qos_get, >>>> + .qos_is_equal = kpkts_pkts_policer_qos_is_equal, >>>> + .qos_run = kpkts_policer_run >>>> +}; >>>> + >>>> static int >>>> dpdk_rx_steer_add_flow(struct netdev_dpdk *dev, >>>> const struct rte_flow_item items[], >>>> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at >>>> index 0f58e8574..8b80a31e6 100644 >>>> --- a/tests/system-dpdk.at >>>> +++ b/tests/system-dpdk.at >>>> @@ -570,6 +570,261 @@ dnl -------------------------------------------------------------------------- >>>> >>>> >>>> >>>> +dnl -------------------------------------------------------------------------- >>>> +dnl QoS (kpkts) create delete vport port >>>> +AT_SETUP([OVS-DPDK - QoS (kpkts) create delete vport port]) >>> I would include the policer name in full, so it will become: >>> >>> AT_SETUP([OVS-DPDK - QoS (pps-policer) 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 >>> Missing dot at the end of the comment. True in other places also, so please fix all instances. >>> >>>> +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=kpkts-policer other-config:kpkts_rate=123 other-config:kpkts_burst=456]) >>>> +AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout]) >>> We should check for the exact output. >>> >>>> +sleep 2 >>> Why a sleep we should avoid this. please use OVS_WAIT_UNTIL on the below greps. >>> >>>> + >>>> +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]) >>> Can we fold these two lines into one. >>> >>>> + >>>> +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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>> Why excluding this error message, it should not be generated here, or do I miss something? >>> >>>> +])") >>>> +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=kpkts-policer other-config:kpkts_burst=123]) >>>> +sleep 2 >>>> + >>> See above test case >>> >>>> +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 >>>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>> Maybe we should also check above to make sure we see this error message. >>> >>>> +])") >>>> +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=kpkts-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]) >>> What about the burst? As we no the full output, why not do it as part of the first AT_CHECK instead of passing it to stdout. >>> >>>> + >>>> +dnl Check egress policer was deleted >>>> +QOS_UUID=`ovs-vsctl get port dpdkvhostuserclient0 qos` >>>> +AT_CHECK([ovs-vsctl set qos $QOS_UUID other_config:kpkts_rate=0]) >>>> +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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 >>>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>>> +])") >>>> +AT_CLEANUP >>>> +dnl -------------------------------------------------------------------------- >>>> + >>> Add one more newline. As it looks like all have 3. >>> >>>> + >>>> +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=kpkts-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 >>>> +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d >>>> +])") >>>> +AT_CLEANUP >>>> +dnl -------------------------------------------------------------------------- >>>> + >>> Add one more newline. As it looks like all have 3. >>> >>>> + >>>> +dnl -------------------------------------------------------------------------- >>>> +dnl QoS (kpkts) testpmd flowgen test >>>> +AT_SETUP([OVS-DPDK - QoS (kpkts) police]) >>>> +AT_KEYWORDS([dpdk]) >>>> + >>>> +OVS_DPDK_PRE_CHECK() >>>> +AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null]) >>>> +OVS_DPDK_START([--no-pci]) >>>> + >>>> +dnl Find number of sockets >>>> +AT_CHECK([lscpu], [], [stdout]) >>>> +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) >>>> + >>>> +dnl Add userspace bridge and attach it to OVS >>>> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev]) >>>> + >>>> +dnl Parse log file >>>> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface dpdkvhostuser0 type=dpdkvhostuser], [], [stdout], [stderr]) >>>> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser1 -- set Interface dpdkvhostuser1 type=dpdkvhostuser], [], [stdout], [stderr]) >>>> +AT_CHECK([ovs-vsctl show], [], [stdout]) >>> Why execute the show? Nothing is done with the output? >>> >>>> + >>>> +dnl Parse log file >>>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >>>> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for vhost-user port dpdkvhostuser0" ovs-vswitchd.log], [], [stdout]) >>>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding succeeded" ovs-vswitchd.log], [], [stdout]) >>>> + >>>> +dnl Parse log file >>>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) >>>> +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser1 created for vhost-user port dpdkvhostuser1" ovs-vswitchd.log], [], [stdout]) >>>> +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) binding succeeded" ovs-vswitchd.log], [], [stdout]) >>>> + >>>> +dnl Configure the same QoS for both ports. >>>> +AT_CHECK([ovs-vsctl set port dpdkvhostuser0 qos=@newqos -- --id=@newqos create qos type=kpkts-policer other-config:kpkts_rate=1 other-config:kpkts_burst=1], [0], [ignore]) >>>> + >>>> +dnl add flows, only send packets from dpdkvhostuser1 to dpdkvhostuser0. >>> Start all commented with a Capital. >>> >>>> +AT_DATA([flows.txt], [dnl >>>> +priority=100,in_port=dpdkvhostuser1,ip,actions=dpdkvhostuser0 >>>> +]) >>>> + >>>> +AT_CHECK([ovs-ofctl del-flows br10]) >>>> +AT_CHECK([ovs-ofctl add-flows br10 flows.txt]) >>> If it's a single flow just do it in the command itself no need to create a flows.txt file. >>> >>>> + >>>> +dnl Execute testpmd in background >>>> +on_exit "pkill -f -x -9 'tail -f /dev/null'" >>>> +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\ >>>> + --vdev="net_virtio_user0,path=$OVS_RUNDIR/dpdkvhostuser0" \ >>>> + --vdev="net_virtio_user1,path=$OVS_RUNDIR/dpdkvhostuser1" \ >>>> + --single-file-segments -- --forward-mode=flowgen -a > $OVS_RUNDIR/testpmd-dpdkvhostuser.log 2>&1 & >>>> + >>>> +dnl sent packet 10 second. >>>> +AT_CHECK([sleep 10]) >>> Do we need 10 seconds, can we make this shorter? >>> >>>> + >>>> +dnl Clean up the testpmd now >>>> +pkill -f -x -9 'tail -f /dev/null' >>>> + >>>> +dnl ---------------------- Forward statistics for port 0 ---------------------- >>>> +dnl RX-packets: 9911 RX-dropped: 0 RX-total: 9911 >>>> +dnl TX-packets: 15937632 TX-dropped: 226661984 TX-total: 242599616 >>>> +dnl ---------------------------------------------------------------------------- >>>> +port0_rx_packets=`cat testpmd-dpdkvhostuser.log | grep "Forward statistics for port 0" -A 1 | grep "RX-packets:" | awk '{print $2}'` >>>> +echo "port0_rx_packets=$port0_rx_packets" >>>> + >>>> +AT_CHECK([test $port0_rx_packets -lt 10500]) >>>> +AT_CHECK([test $port0_rx_packets -gt 9500]) >>>> + >>>> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ >>>> +\@dpdkvhostuser ports are considered deprecated; please migrate to dpdkvhostuserclient ports.@d >>> Can we migrate to dpdkvhostuserclient ports? >>> >>>> +])") >>>> +AT_CLEANUP >>>> +dnl -------------------------------------------------------------------------- >>>> + >>>> + >>>> >>>> dnl -------------------------------------------------------------------------- >>>> dnl MTU increase phy port >>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >>>> index cfcde34ff..19fc13873 100644 >>>> --- a/vswitchd/vswitch.xml >>>> +++ b/vswitchd/vswitch.xml >>>> @@ -4874,6 +4874,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>>> created with the same <code>other_config</code> values as the >>>> physical port. >>>> </dd> >>>> + <dt><code>kpkts-policer</code></dt> >>>> + <dd> >>>> + A DPDK egress packet per second policer algorithm using the ovs >>>> + token bucket library. The token bucket library provides an >>>> + implementation which allows the policing of packets traffic. The >>>> + implementation in OVS essentially creates a single token bucket used >>>> + to police traffic. It should be noted that when the token bucket is >>>> + configured as part of QoS there will be a performance overhead as the >>>> + token bucket itself will consume CPU cycles in order to police >>>> + traffic. These CPU cycles ordinarily are used for packet proccessing. >>>> + As such the drop in performance will be noticed in terms of overall >>>> + aggregate traffic throughput. >>>> + </dd> >>>> </dl> >>>> </column> >>>> >>>> @@ -4966,6 +4979,25 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ >>>> </column> >>>> </group> >>>> >>>> + <group title="Configuration for kpkts-policer QoS"> >>>> + <p> >>>> + <ref table="QoS"/> <ref table="QoS" column="type"/> >>>> + <code>kpkts-policer</code> provides egress pkts policing for userspace >>>> + port types with DPDK. >>>> + >>>> + It has the following key-value pairs defined. >>>> + </p> >>>> + >>>> + <column name="other_config" key="kpkts_rate" type='{"type": "integer"}'> >>>> + The Kilo Packets Per Second (kpps) 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"}'> >>>> + The Packets Per Second Burst Size is measured in count and represents a >>>> + token bucket. >>>> + </column> >>>> + </group> >>>> + >>>> <group title="Configuration for linux-sfq"> >>>> <p> >>>> The <code>linux-sfq</code> QoS supports the following key-value pairs: >>>> -- >>>> 2.39.3 >> -- >> Best regards, Huang Lin.
diff --git a/Documentation/topics/dpdk/qos.rst b/Documentation/topics/dpdk/qos.rst index a98ec672f..37f482cc5 100644 --- a/Documentation/topics/dpdk/qos.rst +++ b/Documentation/topics/dpdk/qos.rst @@ -36,6 +36,9 @@ QoS (Egress Policing) Single Queue Policer ~~~~~~~~~~~~~~~~~~~~ +Bytes Per Second 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:: @@ -52,6 +55,24 @@ To clear the QoS configuration from the port and ovsdb, run:: $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos +Packets Per Second Policer +++++++++++++++++++++++++++ + +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 packets per second:: + + ovs-vsctl set port vhost-user0 qos=@newqos -- \ + --id=@newqos create qos type=kpkts-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 + +To clear the QoS configuration from the port and ovsdb, run:: + + $ ovs-vsctl destroy QoS vhost-user0 -- clear Port vhost-user0 qos Multi Queue Policer ~~~~~~~~~~~~~~~~~~~ diff --git a/NEWS b/NEWS index b582bdbbc..430c3daaf 100644 --- a/NEWS +++ b/NEWS @@ -63,6 +63,7 @@ v3.2.0 - 17 Aug 2023 * 'ovs-appctl dpif-netdev/pmd-sleep-show' command was added to get the max sleep configuration of PMD thread cores. * Removed experimental tag from PMD load based sleeping. + * Added new Qos type 'pkts-policer' to support kilo packet-per-second policing. - Linux TC offload: * Add support for offloading VXLAN tunnels with the GBP extensions. - Python diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index bc8204f7e..c6a26dc7e 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. @@ -346,6 +350,7 @@ struct dpdk_qos_ops { /* dpdk_qos_ops for each type of user space QoS implementation. */ static const struct dpdk_qos_ops egress_policer_ops; static const struct dpdk_qos_ops trtcm_policer_ops; +static const struct dpdk_qos_ops kpkts_policer_ops; /* * Array of dpdk_qos_ops, contains pointer to all supported QoS @@ -354,6 +359,7 @@ static const struct dpdk_qos_ops trtcm_policer_ops; static const struct dpdk_qos_ops *const qos_confs[] = { &egress_policer_ops, &trtcm_policer_ops, + &kpkts_policer_ops, NULL }; @@ -5571,6 +5577,159 @@ static const struct dpdk_qos_ops trtcm_policer_ops = { .qos_queue_dump_state_init = trtcm_policer_qos_queue_dump_state_init }; +/* kpkts-policer details */ +struct kpkts_policer { + struct qos_conf qos_conf; + struct token_bucket tb; + uint32_t kpkts_rate; + uint32_t kpkts_burst; +}; + +static int +kpkts_policer_run_single_packet(struct token_bucket *tb, struct rte_mbuf **pkts, + int pkt_cnt, bool should_steal) +{ + struct rte_mbuf *should_steal_batch[NETDEV_MAX_BURST]; + long long int now = time_msec(); + int i = 0, n = 0; + int cnt = 0; + + for (i = 0; i < pkt_cnt; i++) { + struct rte_mbuf *pkt = pkts[i]; + /* Handle current packet. */ + if (token_bucket_withdraw(tb, 1, now)) { + /* Count passed packets. */ + if (cnt != i) { + pkts[cnt] = pkt; + } + cnt++; + } else if (should_steal) { + /* Count dropped packets. */ + should_steal_batch[n++] = pkt; + } + } + + if (n) { + rte_pktmbuf_free_bulk(should_steal_batch, n); + } + + return cnt; +} + +static int +kpkts_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 EINVAL; + } + + return 0; +} + +static int +kpkts_policer_qos_construct(const struct smap *details, struct qos_conf **conf) +{ + uint32_t kpkts_rate, kpkts_burst; + struct kpkts_policer *policer; + int err; + + policer = xzalloc(sizeof *policer); + kpkts_rate = smap_get_uint(details, "kpkts_rate", 0); + kpkts_burst = smap_get_uint(details, "kpkts_burst", 0); + + /* + * Force to 0 if no rate specified, + * default to rate if burst is 0, + * else stick with user-specified value. + */ + kpkts_burst = (!kpkts_rate ? 0 : !kpkts_burst ? kpkts_rate : kpkts_burst); + + qos_conf_init(&policer->qos_conf, &kpkts_policer_ops); + err = kpkts_policer_profile_config(&policer->tb, kpkts_rate, kpkts_burst); + if (!err) { + policer->kpkts_rate = kpkts_rate; + policer->kpkts_burst = kpkts_burst; + + *conf = &policer->qos_conf; + } else { + VLOG_DBG("Could not create token bucket for egress policer"); + free(policer); + *conf = NULL; + } + + return err; +} + +static void +kpkts_policer_qos_destruct(struct qos_conf *conf) +{ + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, + qos_conf); + + free(policer); +} + +static int +kpkts_policer_qos_get(const struct qos_conf *conf, struct smap *details) +{ + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, + qos_conf); + + smap_add_format(details, "kpkts_rate", "%"PRIu32, policer->kpkts_rate); + smap_add_format(details, "kpkts_burst", "%"PRIu32, policer->kpkts_burst); + + return 0; +} + +static bool +kpkts_pkts_policer_qos_is_equal(const struct qos_conf *conf, + const struct smap *details) +{ + uint32_t rate, burst; + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, + qos_conf); + + rate = smap_get_uint(details, "pkts_rate", 0); + burst = smap_get_uint(details, "pkts_burst", 0); + + return (policer->tb.rate == rate && policer->tb.burst == burst); +} + +static int +kpkts_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt, + bool should_steal) +{ + int cnt; + struct kpkts_policer *policer = CONTAINER_OF(conf, struct kpkts_policer, + qos_conf); + + cnt = kpkts_policer_run_single_packet(&policer->tb, pkts, pkt_cnt, + should_steal); + + return cnt; +} + +static const struct dpdk_qos_ops kpkts_policer_ops = { + .qos_name = "kpkts-policer", + .qos_construct = kpkts_policer_qos_construct, + .qos_destruct = kpkts_policer_qos_destruct, + .qos_get = kpkts_policer_qos_get, + .qos_is_equal = kpkts_pkts_policer_qos_is_equal, + .qos_run = kpkts_policer_run +}; + static int dpdk_rx_steer_add_flow(struct netdev_dpdk *dev, const struct rte_flow_item items[], diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index 0f58e8574..8b80a31e6 100644 --- a/tests/system-dpdk.at +++ b/tests/system-dpdk.at @@ -570,6 +570,261 @@ 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=kpkts-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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 Check egress policer was deleted +QOS_UUID=`ovs-vsctl get port dpdkvhostuserclient0 qos` +AT_CHECK([ovs-vsctl set qos $QOS_UUID other_config:kpkts_rate=0]) +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 kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@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=kpkts-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 +\@Failed to set QoS type kpkts-policer on port dpdkvhostuserclient0: Invalid argument@d +])") +AT_CLEANUP +dnl -------------------------------------------------------------------------- + + +dnl -------------------------------------------------------------------------- +dnl QoS (kpkts) testpmd flowgen test +AT_SETUP([OVS-DPDK - QoS (kpkts) police]) +AT_KEYWORDS([dpdk]) + +OVS_DPDK_PRE_CHECK() +AT_SKIP_IF([! which dpdk-testpmd >/dev/null 2>/dev/null]) +OVS_DPDK_START([--no-pci]) + +dnl Find number of sockets +AT_CHECK([lscpu], [], [stdout]) +AT_CHECK([cat stdout | grep "NUMA node(s)" | awk '{c=1; while (c++<$(3)) {printf "512,"}; print "512"}' > NUMA_NODE]) + +dnl Add userspace bridge and attach it to OVS +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev]) + +dnl Parse log file +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set Interface dpdkvhostuser0 type=dpdkvhostuser], [], [stdout], [stderr]) +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser1 -- set Interface dpdkvhostuser1 type=dpdkvhostuser], [], [stdout], [stderr]) +AT_CHECK([ovs-vsctl show], [], [stdout]) + +dnl Parse log file +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for vhost-user port dpdkvhostuser0" ovs-vswitchd.log], [], [stdout]) +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding succeeded" ovs-vswitchd.log], [], [stdout]) + +dnl Parse log file +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) vhost-user server: socket created" ovs-vswitchd.log], [], [stdout]) +AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser1 created for vhost-user port dpdkvhostuser1" ovs-vswitchd.log], [], [stdout]) +AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser1) binding succeeded" ovs-vswitchd.log], [], [stdout]) + +dnl Configure the same QoS for both ports. +AT_CHECK([ovs-vsctl set port dpdkvhostuser0 qos=@newqos -- --id=@newqos create qos type=kpkts-policer other-config:kpkts_rate=1 other-config:kpkts_burst=1], [0], [ignore]) + +dnl add flows, only send packets from dpdkvhostuser1 to dpdkvhostuser0. +AT_DATA([flows.txt], [dnl +priority=100,in_port=dpdkvhostuser1,ip,actions=dpdkvhostuser0 +]) + +AT_CHECK([ovs-ofctl del-flows br10]) +AT_CHECK([ovs-ofctl add-flows br10 flows.txt]) + +dnl Execute testpmd in background +on_exit "pkill -f -x -9 'tail -f /dev/null'" +tail -f /dev/null | dpdk-testpmd --socket-mem="$(cat NUMA_NODE)" --no-pci\ + --vdev="net_virtio_user0,path=$OVS_RUNDIR/dpdkvhostuser0" \ + --vdev="net_virtio_user1,path=$OVS_RUNDIR/dpdkvhostuser1" \ + --single-file-segments -- --forward-mode=flowgen -a > $OVS_RUNDIR/testpmd-dpdkvhostuser.log 2>&1 & + +dnl sent packet 10 second. +AT_CHECK([sleep 10]) + +dnl Clean up the testpmd now +pkill -f -x -9 'tail -f /dev/null' + +dnl ---------------------- Forward statistics for port 0 ---------------------- +dnl RX-packets: 9911 RX-dropped: 0 RX-total: 9911 +dnl TX-packets: 15937632 TX-dropped: 226661984 TX-total: 242599616 +dnl ---------------------------------------------------------------------------- +port0_rx_packets=`cat testpmd-dpdkvhostuser.log | grep "Forward statistics for port 0" -A 1 | grep "RX-packets:" | awk '{print $2}'` +echo "port0_rx_packets=$port0_rx_packets" + +AT_CHECK([test $port0_rx_packets -lt 10500]) +AT_CHECK([test $port0_rx_packets -gt 9500]) + +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [ +\@dpdkvhostuser ports are considered deprecated; please migrate to dpdkvhostuserclient ports.@d +])") +AT_CLEANUP +dnl -------------------------------------------------------------------------- + + dnl -------------------------------------------------------------------------- dnl MTU increase phy port diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index cfcde34ff..19fc13873 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -4874,6 +4874,19 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ created with the same <code>other_config</code> values as the physical port. </dd> + <dt><code>kpkts-policer</code></dt> + <dd> + A DPDK egress packet per second policer algorithm using the ovs + token bucket library. The token bucket library provides an + implementation which allows the policing of packets traffic. The + implementation in OVS essentially creates a single token bucket used + to police traffic. It should be noted that when the token bucket is + configured as part of QoS there will be a performance overhead as the + token bucket itself will consume CPU cycles in order to police + traffic. These CPU cycles ordinarily are used for packet proccessing. + As such the drop in performance will be noticed in terms of overall + aggregate traffic throughput. + </dd> </dl> </column> @@ -4966,6 +4979,25 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \ </column> </group> + <group title="Configuration for kpkts-policer QoS"> + <p> + <ref table="QoS"/> <ref table="QoS" column="type"/> + <code>kpkts-policer</code> provides egress pkts policing for userspace + port types with DPDK. + + It has the following key-value pairs defined. + </p> + + <column name="other_config" key="kpkts_rate" type='{"type": "integer"}'> + The Kilo Packets Per Second (kpps) 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"}'> + The Packets Per Second Burst Size is measured in count and represents a + token bucket. + </column> + </group> + <group title="Configuration for linux-sfq"> <p> The <code>linux-sfq</code> QoS supports the following key-value pairs: