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, [email protected] wrote: >> >>> From: Lin Huang <[email protected]> >>> >>> OvS has supported packet-per-second policer which can be set at ingress >>> and egress side in kernel datapath. But the userspace datapath doesn't >>> support for ingress and egress packet-per-second policing now. >>> >>> So, this patch add support for userspace egress pps policing by using >>> native ovs token bucket library. Token bucket is accumulated by 'rate' >>> tokens per millisecond and store maximum tokens at 'burst' bucket size. >>> One token in the bucket means one packet (1 kpkts * millisecond) which >>> will drop or pass by policer. >>> >>> This patch add 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 <[email protected]> >>> --- >>> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
