[{"id":1760278,"web_url":"http://patchwork.ozlabs.org/comment/1760278/","msgid":"","list_archive_url":null,"date":"2017-08-30T15:18:18","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":67333,"url":"http://patchwork.ozlabs.org/api/people/67333/","name":"Ian Stokes","email":"ian.stokes@intel.com"},"content":"> In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\n> QoS checking may drop some of them.\n> Move the QoS checking in front of copying data to mbuf, it helps to reduce\n> useless copy.\n\nHi Zhenyu, thanks for the patch, I agree with the objective of the patch but have some comments below I'd like to see addressed/tested before signing off.\n\n> \n> Signed-off-by: Zhenyu Gao \n> ---\n> lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++--------------\n> -----\n> 1 file changed, 36 insertions(+), 19 deletions(-)\n> \n> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\n> 100644\n> --- a/lib/netdev-dpdk.c\n> +++ b/lib/netdev-dpdk.c\n> @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> * For all QoS implementations it should always be non-null.\n> */\n> int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> - int pkt_cnt);\n> + int pkt_cnt, bool may_steal);\n> };\n> \n> /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\n> 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> *meter,\n> \n> static int\n> netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> - struct rte_mbuf **pkts, int pkt_cnt)\n> + struct rte_mbuf **pkts, int pkt_cnt,\n> + bool may_steal)\n> {\n> int i = 0;\n> int cnt = 0;\n> @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> *meter,\n> }\n> cnt++;\n> } else {\n> - rte_pktmbuf_free(pkt);\n> + if (may_steal) {\n> + rte_pktmbuf_free(pkt);\n> + }\n> }\n> }\n> \n> @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> *meter,\n> \n> static int\n> ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> **pkts,\n> - int pkt_cnt)\n> + int pkt_cnt, bool may_steal)\n> {\n> int cnt = 0;\n> \n> rte_spinlock_lock(&policer->policer_lock);\n> - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n> + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> + pkt_cnt, may_steal);\n> rte_spinlock_unlock(&policer->policer_lock);\n> \n> return cnt;\n> @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n> dropped = nb_rx;\n> nb_rx = ingress_policer_run(policer,\n> (struct rte_mbuf **) batch->packets,\n> - nb_rx);\n> + nb_rx, true);\n> dropped -= nb_rx;\n> }\n> \n> @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\n> dp_packet_batch *batch)\n> dropped = nb_rx;\n> nb_rx = ingress_policer_run(policer,\n> (struct rte_mbuf **) batch->packets,\n> - nb_rx);\n> + nb_rx, true);\n> dropped -= nb_rx;\n> }\n> \n> @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> struct dp_packet_batch *batch)\n> \n> static inline int\n> netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> - int cnt)\n> + int cnt, bool may_steal)\n> {\n> struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\n> >qos_conf);\n> \n> if (qos_conf) {\n> rte_spinlock_lock(&qos_conf->lock);\n> - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> rte_spinlock_unlock(&qos_conf->lock);\n> }\n> \n> @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int\n> qid,\n> \n> cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> /* Check has QoS has been configured for the netdev */\n> - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> dropped = total_pkts - cnt;\n> \n> do {\n> @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> struct dp_packet_batch *batch) #endif\n> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> + int txcnt_eth = batch->count;\n> int dropped = 0;\n> int newcnt = 0;\n> int i;\n> \n> + if (dev->type != DPDK_DEV_VHOST) {\n> + /* Check if QoS has been configured for this netdev. */\n> + txcnt_eth = netdev_dpdk_qos_run(dev,\n> + (struct rte_mbuf **)batch-\n> >packets,\n> + txcnt_eth, false);\n\nSo dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its triggered by the following conditions\n\n if (OVS_UNLIKELY(!may_steal ||\n batch->packets[0]->source != DPBUF_DPDK)\n\nWhat will happen in above if the source of the packet is not DPBUF_DPDK?\n\nFor the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()' is called later on.\n\nThis has specific DPDK calls that expect an mbuf source for the packet. \n\nPreviously QoS took place after the packet had been copied so this was not an issue but now we have DPDK rte functions being called on a non mbuf source packet. I'm not sure of the behavior that could occur in this case. Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt) in 'netdev_dpdk_policer_pkt_handle()'?\n\nIt could be the case then that non DPDK specific call maybe required to attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()' before calling the meter itself.\n\nHave you tested this use case?\n\nIan\n\n> + if (txcnt_eth == 0) {\n> + dropped = batch->count;\n> + goto out;\n> + }\n> + }\n> +\n> dp_packet_batch_apply_cutlen(batch);\n> \n> for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\n> dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\n> *batch)\n> rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> \n> newcnt++;\n> + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> + dropped += batch->count - i - 1;\n> + break;\n> + }\n> }\n> \n> if (dev->type == DPDK_DEV_VHOST) {\n> __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n> newcnt);\n> } else {\n> - unsigned int qos_pkts = newcnt;\n> -\n> - /* Check if QoS has been configured for this netdev. */\n> - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> -\n> - dropped += qos_pkts - newcnt;\n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> }\n> \n> +out:\n> if (OVS_UNLIKELY(dropped)) {\n> rte_spinlock_lock(&dev->stats_lock);\n> dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\n> netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n> dp_packet_batch_apply_cutlen(batch);\n> \n> cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> dropped = batch->count - cnt;\n> \n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\n> 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\n> *conf, }\n> \n> static int\n> -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> pkt_cnt)\n> +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> pkt_cnt,\n> + bool may_steal)\n> {\n> int cnt = 0;\n> struct egress_policer *policer =\n> CONTAINER_OF(conf, struct egress_policer, qos_conf);\n> \n> - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\n> + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> + pkt_cnt, may_steal);\n> \n> return cnt;\n> }\n> --\n> 1.8.3.1","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xj8PY2Kyxz9s8w\n\tfor ;\n\tThu, 31 Aug 2017 01:22:09 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 6AA2EB75;\n\tWed, 30 Aug 2017 15:18:26 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 7C42CB69\n\tfor ; Wed, 30 Aug 2017 15:18:25 +0000 (UTC)","from mga03.intel.com (mga03.intel.com [134.134.136.65])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 93B541B3\n\tfor ; Wed, 30 Aug 2017 15:18:23 +0000 (UTC)","from fmsmga006.fm.intel.com ([10.253.24.20])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t30 Aug 2017 08:18:22 -0700","from irsmsx153.ger.corp.intel.com ([163.33.192.75])\n\tby fmsmga006.fm.intel.com with ESMTP; 30 Aug 2017 08:18:22 -0700","from irsmsx101.ger.corp.intel.com ([169.254.1.22]) by\n\tIRSMSX153.ger.corp.intel.com ([169.254.9.34]) with mapi id\n\t14.03.0319.002; Wed, 30 Aug 2017 16:18:19 +0100"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,449,1498546800\"; d=\"scan'208\";a=\"146324742\"","From":"\"Stokes, Ian\" ","To":"Zhenyu Gao , \"dev@openvswitch.org\"\n\t","Thread-Topic":"[PATCH v2] netdev-dpdk: Execute QoS Checking before copying to\n\tmbuf","Thread-Index":"AQHTILubOGIxIMkmR0SSj5OWMAF0eKKc/nLw","Date":"Wed, 30 Aug 2017 15:18:18 +0000","Message-ID":"","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>","In-Reply-To":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>","Accept-Language":"en-IE, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-titus-metadata-40":"eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWRhN2U4ZTYtNWI3Yi00YWE5LWE5ZTktZGE5MmUxZThkNDViIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ijd3VzBCN1ZjOG9vakh3OVYrMkRiVUM5Y0xSc1lcL3BJQlhNVjh4cUlIWnZFPSJ9","x-ctpclassification":"CTP_IC","dlp-product":"dlpe-windows","dlp-version":"11.0.0.116","dlp-reaction":"no-action","x-originating-ip":"[163.33.239.182]","MIME-Version":"1.0","X-Spam-Status":"No, score=-5.0 required=5.0 tests=RCVD_IN_DNSWL_HI,\n\tRP_MATCHES_RCVD autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1760599,"web_url":"http://patchwork.ozlabs.org/comment/1760599/","msgid":"","list_archive_url":null,"date":"2017-08-31T01:56:21","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":70513,"url":"http://patchwork.ozlabs.org/api/people/70513/","name":"Gao Zhenyu","email":"sysugaozhenyu@gmail.com"},"content":"Here is the the testing I just did:\n\n1. Add log in function netdev_dpdk_policer_pkt_handle\nstatic inline\nbool\nnetdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n*meter,\n struct rte_mbuf *pkt, uint64_t\ntime)\n{\n uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct\nether_hdr);\n VLOG_WARN(\"GGGG, payload_len:%u,\nrte_pkt_len:%u\",\n pkt_len, rte_pktmbuf_pkt_len(pkt));\n<------------------------print out pkt_len\n\n\n return rte_meter_srtcm_color_blind_check(meter, time, pkt_len)\n==\n\ne_RTE_METER_GREEN;\n}\n\n2. rebuild a rpm, install in machine, enable DPDK.\n3. Create a container on the bridge(userspace bridge). (The container use\nveth device)\n4. Config qos on eth dpdk eth by using : ovs-vsctl set port\ndpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos\ntype=egress-policer other-config:cir=1250000 other-config:cbs=2048\n5. execute ping on the container and monitor ovs-vswitchd.log. Then I can\nsee:\n\n2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88,\nrte_pkt_len:102\n2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88,\nrte_pkt_len:102\n2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88,\nrte_pkt_len:102\n2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88,\nrte_pkt_len:102\n\nSo the mbuf->pkt_len has a correct length.\n\nOn the code side, we have function dp_packet_set_size(), if ovs compiled\nwith dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many\nfunctions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in\nNetdev-linux.c\n\n#ifdef DPDK_NETDEV\n......\n\nstatic inline void\ndp_packet_set_size(struct dp_packet *b, uint32_t v)\n{\n.....\n b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\n b->mbuf.pkt_len = v; /* Total length of all segments linked\nto\n * this segment. */\n}\n\nPlease let me know if you have any concern on it.\n\nThanks\nZhenyu Gao\n\n2017-08-30 23:18 GMT+08:00 Stokes, Ian :\n\n> > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\n> > QoS checking may drop some of them.\n> > Move the QoS checking in front of copying data to mbuf, it helps to\n> reduce\n> > useless copy.\n>\n> Hi Zhenyu, thanks for the patch, I agree with the objective of the patch\n> but have some comments below I'd like to see addressed/tested before\n> signing off.\n>\n> >\n> > Signed-off-by: Zhenyu Gao \n> > ---\n> > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++\n> ++++++--------------\n> > -----\n> > 1 file changed, 36 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\n> > 100644\n> > --- a/lib/netdev-dpdk.c\n> > +++ b/lib/netdev-dpdk.c\n> > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> > * For all QoS implementations it should always be non-null.\n> > */\n> > int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> > - int pkt_cnt);\n> > + int pkt_cnt, bool may_steal);\n> > };\n> >\n> > /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\n> > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> > - struct rte_mbuf **pkts, int pkt_cnt)\n> > + struct rte_mbuf **pkts, int pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int i = 0;\n> > int cnt = 0;\n> > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> > }\n> > cnt++;\n> > } else {\n> > - rte_pktmbuf_free(pkt);\n> > + if (may_steal) {\n> > + rte_pktmbuf_free(pkt);\n> > + }\n> > }\n> > }\n> >\n> > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> > **pkts,\n> > - int pkt_cnt)\n> > + int pkt_cnt, bool may_steal)\n> > {\n> > int cnt = 0;\n> >\n> > rte_spinlock_lock(&policer->policer_lock);\n> > - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> > + pkt_cnt, may_steal);\n> > rte_spinlock_unlock(&policer->policer_lock);\n> >\n> > return cnt;\n> > @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\n> > dp_packet_batch *batch)\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> > struct dp_packet_batch *batch)\n> >\n> > static inline int\n> > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> > - int cnt)\n> > + int cnt, bool may_steal)\n> > {\n> > struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\n> > >qos_conf);\n> >\n> > if (qos_conf) {\n> > rte_spinlock_lock(&qos_conf->lock);\n> > - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> > + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> > rte_spinlock_unlock(&qos_conf->lock);\n> > }\n> >\n> > @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,\n> int\n> > qid,\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> > /* Check has QoS has been configured for the netdev */\n> > - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> > dropped = total_pkts - cnt;\n> >\n> > do {\n> > @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> > struct dp_packet_batch *batch) #endif\n> > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> > struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> > + int txcnt_eth = batch->count;\n> > int dropped = 0;\n> > int newcnt = 0;\n> > int i;\n> >\n> > + if (dev->type != DPDK_DEV_VHOST) {\n> > + /* Check if QoS has been configured for this netdev. */\n> > + txcnt_eth = netdev_dpdk_qos_run(dev,\n> > + (struct rte_mbuf **)batch-\n> > >packets,\n> > + txcnt_eth, false);\n>\n> So dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its\n> triggered by the following conditions\n>\n> if (OVS_UNLIKELY(!may_steal ||\n> batch->packets[0]->source != DPBUF_DPDK)\n>\n> What will happen in above if the source of the packet is not DPBUF_DPDK?\n>\n> For the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()'\n> is called later on.\n>\n> This has specific DPDK calls that expect an mbuf source for the packet.\n>\n> Previously QoS took place after the packet had been copied so this was not\n> an issue but now we have DPDK rte functions being called on a non mbuf\n> source packet. I'm not sure of the behavior that could occur in this case.\n> Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt)\n> in 'netdev_dpdk_policer_pkt_handle()'?\n>\n> It could be the case then that non DPDK specific call maybe required to\n> attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()'\n> before calling the meter itself.\n>\n> Have you tested this use case?\n>\n> Ian\n>\n> > + if (txcnt_eth == 0) {\n> > + dropped = batch->count;\n> > + goto out;\n> > + }\n> > + }\n> > +\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\n> > dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\n> > *batch)\n> > rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> >\n> > newcnt++;\n> > + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> > + dropped += batch->count - i - 1;\n> > + break;\n> > + }\n> > }\n> >\n> > if (dev->type == DPDK_DEV_VHOST) {\n> > __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)\n> pkts,\n> > newcnt);\n> > } else {\n> > - unsigned int qos_pkts = newcnt;\n> > -\n> > - /* Check if QoS has been configured for this netdev. */\n> > - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> > -\n> > - dropped += qos_pkts - newcnt;\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> > }\n> >\n> > +out:\n> > if (OVS_UNLIKELY(dropped)) {\n> > rte_spinlock_lock(&dev->stats_lock);\n> > dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\n> > netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> > - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> > dropped = batch->count - cnt;\n> >\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\n> > 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\n> > *conf, }\n> >\n> > static int\n> > -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt)\n> > +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int cnt = 0;\n> > struct egress_policer *policer =\n> > CONTAINER_OF(conf, struct egress_policer, qos_conf);\n> >\n> > - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> > + pkt_cnt, may_steal);\n> >\n> > return cnt;\n> > }\n> > --\n> > 1.8.3.1\n>\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EOdi2YP+\"; dkim-atps=neutral"],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xjQTV2z1Hz9s7c\n\tfor ;\n\tThu, 31 Aug 2017 11:56:27 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 64888A7B;\n\tThu, 31 Aug 2017 01:56:24 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 952D3416\n\tfor ; Thu, 31 Aug 2017 01:56:23 +0000 (UTC)","from mail-pg0-f46.google.com (mail-pg0-f46.google.com\n\t[74.125.83.46])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 6CD103F5\n\tfor ; Thu, 31 Aug 2017 01:56:22 +0000 (UTC)","by mail-pg0-f46.google.com with SMTP id b8so24789020pgn.5\n\tfor ; Wed, 30 Aug 2017 18:56:22 -0700 (PDT)","by 10.100.151.235 with HTTP; Wed, 30 Aug 2017 18:56:21 -0700 (PDT)"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=GjRxjamHEkz4Z2z+cSxolDgDU2zB+tBl4cff9I6ylc0=;\n\tb=EOdi2YP+pZxvAYOW0PIOBexTmJcflvXgEh0PgNnAW9qcnYkaHFiGh0vPEgPsm08Q1k\n\tp6JP5/x7gv7aKPtbKFDCZYK73MLtzti+knsPF7vwbGlixQ/cNewnCJ/JXdpeyydK1/iI\n\t3JHCAvsfZrZkvEbObPdOlO4ZL3PkgCE6ZehPhsQ3bkmajjMPp7nVc/xVcmimb3yGuXdf\n\tgUFG6LbjwTwtE9GE59/+mfsfGJvFnxeNFvqA7PcNfhdSMdBLGpBprNtA1nwRNyM7FR7z\n\te3sREUgWRT6cwnAeWXDv9almuiC3u3dSOCoAAXX+oRLE9QcyS8vfCLuQuodwGgaukwey\n\ttr7g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=GjRxjamHEkz4Z2z+cSxolDgDU2zB+tBl4cff9I6ylc0=;\n\tb=UtcxMMfvBAFpI0ztjnat5baOzhiKoMle2eFa8YMpIdEtB9MFWuImYUKB/tSOzrYHOB\n\t10AdjH+yb5Bh0w4XDu083tLS8KrZ09XHVy8f+kZg+h0n+BRdEL53nAjp1cWmG/qajsp6\n\tym127VTLcSVYM0VHnLkrrJwaVfhVW3ggNLbMLahOLJva6WFpdK6wuA7weFl5V7M9XlUl\n\tvSu3ljIalYUn25H8obVOuvqR+97gpSfDjy7b9jEi/im5cFO5NI9n+y5Aj3VW0C16Cg70\n\tLFpwembcHwSOFLVLYl2wXKZ0J5o/t+TmfZOpNmhXdq51zD32g/NZiFGlJRAo0u/1GIOp\n\t9FNA==","X-Gm-Message-State":"AHYfb5g3Sgmv0WVPWnmFibumJ92KLlrKQEq/aahT6yoSrhWNLL7c3jm4\n\tXKrldJTkHqB5ELOBmoWTEabFbsK1AQUj","X-Received":"by 10.98.200.154 with SMTP id i26mr606257pfk.256.1504144581901; \n\tWed, 30 Aug 2017 18:56:21 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t","From":"Gao Zhenyu ","Date":"Thu, 31 Aug 2017 09:56:21 +0800","Message-ID":"","To":"\"Stokes, Ian\" ","X-Spam-Status":"No, score=-0.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tDKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_NONE\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1762241,"web_url":"http://patchwork.ozlabs.org/comment/1762241/","msgid":"","list_archive_url":null,"date":"2017-09-03T10:46:24","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":67333,"url":"http://patchwork.ozlabs.org/api/people/67333/","name":"Ian Stokes","email":"ian.stokes@intel.com"},"content":"OK,\r\n\r\nIn my own testing I’m seeing the same behavior with a tap interface pinging to a bridge with a DPDK device. As such I think this should be ok.\r\n\r\nAcked-by: ian.stokes@intel.com\r\n\r\nIan\r\n\r\nFrom: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]\r\nSent: Thursday, August 31, 2017 2:56 AM\r\nTo: Stokes, Ian \r\nCc: dev@openvswitch.org\r\nSubject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf\r\n\r\nHere is the the testing I just did:\r\n1. Add log in function netdev_dpdk_policer_pkt_handle\r\nstatic inline bool\r\nnetdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\r\n struct rte_mbuf *pkt, uint64_t time)\r\n{\r\n uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr);\r\n VLOG_WARN(\"GGGG, payload_len:%u, rte_pkt_len:%u\",\r\n pkt_len, rte_pktmbuf_pkt_len(pkt)); <------------------------print out pkt_len\r\n\r\n return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==\r\n e_RTE_METER_GREEN;\r\n}\r\n2. rebuild a rpm, install in machine, enable DPDK.\r\n3. Create a container on the bridge(userspace bridge). (The container use veth device)\r\n4. Config qos on eth dpdk eth by using : ovs-vsctl set port dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048\r\n5. execute ping on the container and monitor ovs-vswitchd.log. Then I can see:\r\n\r\n2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\nSo the mbuf->pkt_len has a correct length.\r\nOn the code side, we have function dp_packet_set_size(), if ovs compiled with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c\r\n\r\n#ifdef DPDK_NETDEV\r\n......\r\n\r\nstatic inline void\r\ndp_packet_set_size(struct dp_packet *b, uint32_t v)\r\n{\r\n.....\r\n b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\r\n b->mbuf.pkt_len = v; /* Total length of all segments linked to\r\n * this segment. */\r\n}\r\nPlease let me know if you have any concern on it.\r\n\r\nThanks\r\nZhenyu Gao\r\n\r\n2017-08-30 23:18 GMT+08:00 Stokes, Ian >:\r\n> In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\r\n> QoS checking may drop some of them.\r\n> Move the QoS checking in front of copying data to mbuf, it helps to reduce\r\n> useless copy.\r\n\r\nHi Zhenyu, thanks for the patch, I agree with the objective of the patch but have some comments below I'd like to see addressed/tested before signing off.\r\n\r\n>\r\n> Signed-off-by: Zhenyu Gao >\r\n> ---\r\n> lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++--------------\r\n> -----\r\n> 1 file changed, 36 insertions(+), 19 deletions(-)\r\n>\r\n> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\r\n> 100644\r\n> --- a/lib/netdev-dpdk.c\r\n> +++ b/lib/netdev-dpdk.c\r\n> @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\r\n> * For all QoS implementations it should always be non-null.\r\n> */\r\n> int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\r\n> - int pkt_cnt);\r\n> + int pkt_cnt, bool may_steal);\r\n> };\r\n>\r\n> /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\r\n> 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\r\n> *meter,\r\n>\r\n> static int\r\n> netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\r\n> - struct rte_mbuf **pkts, int pkt_cnt)\r\n> + struct rte_mbuf **pkts, int pkt_cnt,\r\n> + bool may_steal)\r\n> {\r\n> int i = 0;\r\n> int cnt = 0;\r\n> @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\r\n> *meter,\r\n> }\r\n> cnt++;\r\n> } else {\r\n> - rte_pktmbuf_free(pkt);\r\n> + if (may_steal) {\r\n> + rte_pktmbuf_free(pkt);\r\n> + }\r\n> }\r\n> }\r\n>\r\n> @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\r\n> *meter,\r\n>\r\n> static int\r\n> ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\r\n> **pkts,\r\n> - int pkt_cnt)\r\n> + int pkt_cnt, bool may_steal)\r\n> {\r\n> int cnt = 0;\r\n>\r\n> rte_spinlock_lock(&policer->policer_lock);\r\n> - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\r\n> + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\r\n> + pkt_cnt, may_steal);\r\n> rte_spinlock_unlock(&policer->policer_lock);\r\n>\r\n> return cnt;\r\n> @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\r\n> dropped = nb_rx;\r\n> nb_rx = ingress_policer_run(policer,\r\n> (struct rte_mbuf **) batch->packets,\r\n> - nb_rx);\r\n> + nb_rx, true);\r\n> dropped -= nb_rx;\r\n> }\r\n>\r\n> @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\r\n> dp_packet_batch *batch)\r\n> dropped = nb_rx;\r\n> nb_rx = ingress_policer_run(policer,\r\n> (struct rte_mbuf **) batch->packets,\r\n> - nb_rx);\r\n> + nb_rx, true);\r\n> dropped -= nb_rx;\r\n> }\r\n>\r\n> @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\r\n> struct dp_packet_batch *batch)\r\n>\r\n> static inline int\r\n> netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\r\n> - int cnt)\r\n> + int cnt, bool may_steal)\r\n> {\r\n> struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\r\n> >qos_conf);\r\n>\r\n> if (qos_conf) {\r\n> rte_spinlock_lock(&qos_conf->lock);\r\n> - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\r\n> + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\r\n> rte_spinlock_unlock(&qos_conf->lock);\r\n> }\r\n>\r\n> @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int\r\n> qid,\r\n>\r\n> cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\r\n> /* Check has QoS has been configured for the netdev */\r\n> - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\r\n> + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\r\n> dropped = total_pkts - cnt;\r\n>\r\n> do {\r\n> @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\r\n> struct dp_packet_batch *batch) #endif\r\n> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\r\n> struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\r\n> + int txcnt_eth = batch->count;\r\n> int dropped = 0;\r\n> int newcnt = 0;\r\n> int i;\r\n>\r\n> + if (dev->type != DPDK_DEV_VHOST) {\r\n> + /* Check if QoS has been configured for this netdev. */\r\n> + txcnt_eth = netdev_dpdk_qos_run(dev,\r\n> + (struct rte_mbuf **)batch-\r\n> >packets,\r\n> + txcnt_eth, false);\r\nSo dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its triggered by the following conditions\r\n\r\n if (OVS_UNLIKELY(!may_steal ||\r\n batch->packets[0]->source != DPBUF_DPDK)\r\n\r\nWhat will happen in above if the source of the packet is not DPBUF_DPDK?\r\n\r\nFor the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()' is called later on.\r\n\r\nThis has specific DPDK calls that expect an mbuf source for the packet.\r\n\r\nPreviously QoS took place after the packet had been copied so this was not an issue but now we have DPDK rte functions being called on a non mbuf source packet. I'm not sure of the behavior that could occur in this case. Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt) in 'netdev_dpdk_policer_pkt_handle()'?\r\n\r\nIt could be the case then that non DPDK specific call maybe required to attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()' before calling the meter itself.\r\n\r\nHave you tested this use case?\r\n\r\nIan\r\n\r\n> + if (txcnt_eth == 0) {\r\n> + dropped = batch->count;\r\n> + goto out;\r\n> + }\r\n> + }\r\n> +\r\n> dp_packet_batch_apply_cutlen(batch);\r\n>\r\n> for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\r\n> dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\r\n> *batch)\r\n> rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\r\n>\r\n> newcnt++;\r\n> + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\r\n> + dropped += batch->count - i - 1;\r\n> + break;\r\n> + }\r\n> }\r\n>\r\n> if (dev->type == DPDK_DEV_VHOST) {\r\n> __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\r\n> newcnt);\r\n> } else {\r\n> - unsigned int qos_pkts = newcnt;\r\n> -\r\n> - /* Check if QoS has been configured for this netdev. */\r\n> - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\r\n> -\r\n> - dropped += qos_pkts - newcnt;\r\n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\r\n> }\r\n>\r\n> +out:\r\n> if (OVS_UNLIKELY(dropped)) {\r\n> rte_spinlock_lock(&dev->stats_lock);\r\n> dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\r\n> netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\r\n> dp_packet_batch_apply_cutlen(batch);\r\n>\r\n> cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\r\n> - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\r\n> + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\r\n> dropped = batch->count - cnt;\r\n>\r\n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\r\n> 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\r\n> *conf, }\r\n>\r\n> static int\r\n> -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\r\n> pkt_cnt)\r\n> +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\r\n> pkt_cnt,\r\n> + bool may_steal)\r\n> {\r\n> int cnt = 0;\r\n> struct egress_policer *policer =\r\n> CONTAINER_OF(conf, struct egress_policer, qos_conf);\r\n>\r\n> - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\r\n> + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\r\n> + pkt_cnt, may_steal);\r\n>\r\n> return cnt;\r\n> }\r\n> --\r\n> 1.8.3.1","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xlV5n0S9Sz9sRW\n\tfor ;\n\tSun, 3 Sep 2017 20:46:35 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 14FEA899;\n\tSun, 3 Sep 2017 10:46:32 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 073C25AA\n\tfor ; Sun, 3 Sep 2017 10:46:30 +0000 (UTC)","from mga06.intel.com (mga06.intel.com [134.134.136.31])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id ABF74F6\n\tfor ; Sun, 3 Sep 2017 10:46:28 +0000 (UTC)","from orsmga001.jf.intel.com ([10.7.209.18])\n\tby orsmga104.jf.intel.com with ESMTP; 03 Sep 2017 03:46:27 -0700","from irsmsx107.ger.corp.intel.com ([163.33.3.99])\n\tby orsmga001.jf.intel.com with ESMTP; 03 Sep 2017 03:46:26 -0700","from irsmsx101.ger.corp.intel.com ([169.254.1.22]) by\n\tIRSMSX107.ger.corp.intel.com ([169.254.10.65]) with mapi id\n\t14.03.0319.002; Sun, 3 Sep 2017 11:46:25 +0100"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,468,1498546800\"; \n\td=\"scan'208,217\";a=\"1168683113\"","From":"\"Stokes, Ian\" ","To":"Gao Zhenyu ","Thread-Topic":"[PATCH v2] netdev-dpdk: Execute QoS Checking before copying to\n\tmbuf","Thread-Index":"AQHTILubOGIxIMkmR0SSj5OWMAF0eKKc/nLwgACoAoCABVt3IA==","Date":"Sun, 3 Sep 2017 10:46:24 +0000","Message-ID":"","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t\n\t","In-Reply-To":"","Accept-Language":"en-IE, en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-titus-metadata-40":"eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDQxNzk3ZTctYmIzMC00Zjk5LWExYjMtY2M4MDdiMmY2MDFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjZsZVBTckxjWXRtNmhHM0cwVElcL0xhU1hEM0pmWkYxZ3B2WW1MN29wcFVJPSJ9","x-ctpclassification":"CTP_IC","dlp-product":"dlpe-windows","dlp-version":"11.0.0.116","dlp-reaction":"no-action","x-originating-ip":"[163.33.239.181]","MIME-Version":"1.0","X-Spam-Status":"No, score=-2.3 required=5.0 tests=AC_DIV_BONANZA, HTML_MESSAGE,\n\tRCVD_IN_DNSWL_MED,\n\tRP_MATCHES_RCVD autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1762563,"web_url":"http://patchwork.ozlabs.org/comment/1762563/","msgid":"","list_archive_url":null,"date":"2017-09-04T10:27:53","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":70513,"url":"http://patchwork.ozlabs.org/api/people/70513/","name":"Gao Zhenyu","email":"sysugaozhenyu@gmail.com"},"content":"Thanks for your testing!\n\nThanks\nZhenyu Gao\n\n2017-09-03 18:46 GMT+08:00 Stokes, Ian :\n\n> OK,\n>\n>\n>\n> In my own testing I’m seeing the same behavior with a tap interface\n> pinging to a bridge with a DPDK device. As such I think this should be ok.\n>\n>\n>\n> Acked-by: ian.stokes@intel.com\n>\n>\n>\n> Ian\n>\n>\n>\n> *From:* Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]\n> *Sent:* Thursday, August 31, 2017 2:56 AM\n> *To:* Stokes, Ian \n> *Cc:* dev@openvswitch.org\n> *Subject:* Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before\n> copying to mbuf\n>\n>\n>\n> Here is the the testing I just did:\n>\n> 1. Add log in function netdev_dpdk_policer_pkt_handle\n> static inline bool\n>\n> netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> *meter,\n> struct rte_mbuf *pkt, uint64_t\n> time)\n> {\n> uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct\n> ether_hdr);\n> VLOG_WARN(\"GGGG, payload_len:%u, rte_pkt_len:%u\",\n>\n> pkt_len, rte_pktmbuf_pkt_len(pkt));\n> <------------------------print out pkt_len\n>\n>\n> return rte_meter_srtcm_color_blind_check(meter, time, pkt_len)\n> ==\n>\n> e_RTE_METER_GREEN;\n> }\n>\n> 2. rebuild a rpm, install in machine, enable DPDK.\n>\n> 3. Create a container on the bridge(userspace bridge). (The container use\n> veth device)\n>\n> 4. Config qos on eth dpdk eth by using : ovs-vsctl set port\n> dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos\n> type=egress-policer other-config:cir=1250000 other-config:cbs=2048\n>\n> 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can\n> see:\n>\n> 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88,\n> rte_pkt_len:102\n> 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88,\n> rte_pkt_len:102\n> 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88,\n> rte_pkt_len:102\n> 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88,\n> rte_pkt_len:102\n>\n> So the mbuf->pkt_len has a correct length.\n>\n> On the code side, we have function dp_packet_set_size(), if ovs compiled\n> with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many\n> functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in\n> Netdev-linux.c\n>\n> #ifdef DPDK_NETDEV\n> ......\n>\n> static inline void\n> dp_packet_set_size(struct dp_packet *b, uint32_t v)\n> {\n> .....\n> b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\n> b->mbuf.pkt_len = v; /* Total length of all segments\n> linked to\n> * this segment. */\n> }\n>\n> Please let me know if you have any concern on it.\n>\n>\n>\n> Thanks\n>\n> Zhenyu Gao\n>\n>\n>\n> 2017-08-30 23:18 GMT+08:00 Stokes, Ian :\n>\n> > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\n> > QoS checking may drop some of them.\n> > Move the QoS checking in front of copying data to mbuf, it helps to\n> reduce\n> > useless copy.\n>\n> Hi Zhenyu, thanks for the patch, I agree with the objective of the patch\n> but have some comments below I'd like to see addressed/tested before\n> signing off.\n>\n>\n> >\n> > Signed-off-by: Zhenyu Gao \n> > ---\n> > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++\n> ++++++--------------\n> > -----\n> > 1 file changed, 36 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\n> > 100644\n> > --- a/lib/netdev-dpdk.c\n> > +++ b/lib/netdev-dpdk.c\n> > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> > * For all QoS implementations it should always be non-null.\n> > */\n> > int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> > - int pkt_cnt);\n> > + int pkt_cnt, bool may_steal);\n> > };\n> >\n> > /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\n> > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> > - struct rte_mbuf **pkts, int pkt_cnt)\n> > + struct rte_mbuf **pkts, int pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int i = 0;\n> > int cnt = 0;\n> > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> > }\n> > cnt++;\n> > } else {\n> > - rte_pktmbuf_free(pkt);\n> > + if (may_steal) {\n> > + rte_pktmbuf_free(pkt);\n> > + }\n> > }\n> > }\n> >\n> > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> > **pkts,\n> > - int pkt_cnt)\n> > + int pkt_cnt, bool may_steal)\n> > {\n> > int cnt = 0;\n> >\n> > rte_spinlock_lock(&policer->policer_lock);\n> > - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> > + pkt_cnt, may_steal);\n> > rte_spinlock_unlock(&policer->policer_lock);\n> >\n> > return cnt;\n> > @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\n> > dp_packet_batch *batch)\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> > struct dp_packet_batch *batch)\n> >\n> > static inline int\n> > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> > - int cnt)\n> > + int cnt, bool may_steal)\n> > {\n> > struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\n> > >qos_conf);\n> >\n> > if (qos_conf) {\n> > rte_spinlock_lock(&qos_conf->lock);\n> > - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> > + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> > rte_spinlock_unlock(&qos_conf->lock);\n> > }\n> >\n> > @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,\n> int\n> > qid,\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> > /* Check has QoS has been configured for the netdev */\n> > - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> > dropped = total_pkts - cnt;\n> >\n> > do {\n> > @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> > struct dp_packet_batch *batch) #endif\n> > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> > struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> > + int txcnt_eth = batch->count;\n> > int dropped = 0;\n> > int newcnt = 0;\n> > int i;\n> >\n> > + if (dev->type != DPDK_DEV_VHOST) {\n> > + /* Check if QoS has been configured for this netdev. */\n> > + txcnt_eth = netdev_dpdk_qos_run(dev,\n> > + (struct rte_mbuf **)batch-\n> > >packets,\n> > + txcnt_eth, false);\n>\n> So dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its\n> triggered by the following conditions\n>\n> if (OVS_UNLIKELY(!may_steal ||\n> batch->packets[0]->source != DPBUF_DPDK)\n>\n> What will happen in above if the source of the packet is not DPBUF_DPDK?\n>\n> For the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()'\n> is called later on.\n>\n> This has specific DPDK calls that expect an mbuf source for the packet.\n>\n> Previously QoS took place after the packet had been copied so this was not\n> an issue but now we have DPDK rte functions being called on a non mbuf\n> source packet. I'm not sure of the behavior that could occur in this case.\n> Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt)\n> in 'netdev_dpdk_policer_pkt_handle()'?\n>\n> It could be the case then that non DPDK specific call maybe required to\n> attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()'\n> before calling the meter itself.\n>\n> Have you tested this use case?\n>\n> Ian\n>\n>\n> > + if (txcnt_eth == 0) {\n> > + dropped = batch->count;\n> > + goto out;\n> > + }\n> > + }\n> > +\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\n> > dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\n> > *batch)\n> > rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> >\n> > newcnt++;\n> > + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> > + dropped += batch->count - i - 1;\n> > + break;\n> > + }\n> > }\n> >\n> > if (dev->type == DPDK_DEV_VHOST) {\n> > __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)\n> pkts,\n> > newcnt);\n> > } else {\n> > - unsigned int qos_pkts = newcnt;\n> > -\n> > - /* Check if QoS has been configured for this netdev. */\n> > - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> > -\n> > - dropped += qos_pkts - newcnt;\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> > }\n> >\n> > +out:\n> > if (OVS_UNLIKELY(dropped)) {\n> > rte_spinlock_lock(&dev->stats_lock);\n> > dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\n> > netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> > - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> > dropped = batch->count - cnt;\n> >\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\n> > 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\n> > *conf, }\n> >\n> > static int\n> > -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt)\n> > +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int cnt = 0;\n> > struct egress_policer *policer =\n> > CONTAINER_OF(conf, struct egress_policer, qos_conf);\n> >\n> > - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> > + pkt_cnt, may_steal);\n> >\n> > return cnt;\n> > }\n> > --\n> > 1.8.3.1\n>\n>\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"gsOzOOGt\"; dkim-atps=neutral"],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xm5dq5Sv7z9s06\n\tfor ;\n\tMon, 4 Sep 2017 20:27:59 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 668B68E3;\n\tMon, 4 Sep 2017 10:27:57 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 497708DC\n\tfor ; Mon, 4 Sep 2017 10:27:56 +0000 (UTC)","from mail-pf0-f174.google.com (mail-pf0-f174.google.com\n\t[209.85.192.174])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id E63E9127\n\tfor ; Mon, 4 Sep 2017 10:27:54 +0000 (UTC)","by mail-pf0-f174.google.com with SMTP id m1so222894pfk.1\n\tfor ; Mon, 04 Sep 2017 03:27:54 -0700 (PDT)","by 10.100.151.235 with HTTP; Mon, 4 Sep 2017 03:27:53 -0700 (PDT)"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=IkVO6Ym12wEHQgCZydOpedgRAMhpre3FQkw+iz8vT0A=;\n\tb=gsOzOOGtANDqv2NJOIiZ4/jZq5a46YCoFFbU75We1uHbDiJ0UEQDX4IsfzxwhB7o2w\n\t9SkQQoWJ3WMb8V0ULRZCb3WY12yclZUO/X+1aG5K/bhT0DwjKb53xekWMDTRi5Y4bt+B\n\tLtHzbdlM3CQSokU5Fhbc7Fzd6noOBJ/16Wi5W808m2OGmLXD+pId28+xZio0jkuvPCVK\n\t3OLxhH22GjJ6JpC5gEWqECjGpSrUQHBe77v3p7+N5Njz6kJMRp0wARCE7LtmAO97lU5b\n\tJFVtwOxOdnzXmTUu60nn6TO8nRZFKiHJfc9/784h1AXky2aoeU74APraWJqChPf8U0l4\n\tY5dA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=IkVO6Ym12wEHQgCZydOpedgRAMhpre3FQkw+iz8vT0A=;\n\tb=kTTugcbBHMc+Nx62JPM/S6mIx6N7wZ0P6fZyUtoWBSivT5/98rDunoTjjaqXdtBdgn\n\t/Vgoe4DSyVPluF4brcGMN7FkP0pOHKOP6x82BmrabmqeyJ7j70k6bLsQcr9YG1YC6LNC\n\tJsgxo9ZuyPd1K0hddfG4i4h6cFcvdoh3afg1UVhzUjpruX9w0JL3pT4gtmc6nTVGkY8e\n\t6nRtymW3rPTIgV6TsKA4Xr8I4s4hGverTVPKBqtVtUIUfhXHYC1fatJK16/F5KNWTvJB\n\tgGQ400J/oKzwLTx5HPtO7XHnincq18ebnJpvFi5Foolh3ZxqKVUDzICYFj0pPdZS7Dx4\n\tDc9w==","X-Gm-Message-State":"AHPjjUiB+s52J6IzGtHGAkyp5uur655cDoxCHkKsairNTn4FZcIwuVuS\n\taejG4HXgzsEljOkJclAJno5UNhit7g==","X-Google-Smtp-Source":"ADKCNb47rDNA/dTHLvV+o+hYlV3PumUoO6BRQeZmFtBpkLhITzucpPnYS+WDaIkcD3B8KKbIj8OmeHGbehbTFNemdrg=","X-Received":"by 10.84.211.78 with SMTP id b72mr86454pli.361.1504520874546;\n\tMon, 04 Sep 2017 03:27:54 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t\n\t\n\t","From":"Gao Zhenyu ","Date":"Mon, 4 Sep 2017 18:27:53 +0800","Message-ID":"","To":"\"Stokes, Ian\" ","X-Spam-Status":"No, score=0.4 required=5.0 tests=AC_DIV_BONANZA, DKIM_SIGNED, \n\tDKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, HTML_MESSAGE,\n\tRCVD_IN_DNSWL_NONE, \n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1763480,"web_url":"http://patchwork.ozlabs.org/comment/1763480/","msgid":"<508BD058-F68B-4344-B347-B1CBE8D2684A@vmware.com>","list_archive_url":null,"date":"2017-09-05T16:24:08","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":68212,"url":"http://patchwork.ozlabs.org/api/people/68212/","name":"Darrell Ball","email":"dball@vmware.com"},"content":"Hi Gao\n\nHow about the following incremental ?\n\nThanks Darrell\n\n\n\ndiff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\nindex 4808d38..648d719 100644\n--- a/lib/netdev-dpdk.c\n+++ b/lib/netdev-dpdk.c\n@@ -1843,64 +1843,58 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n #endif\n struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n- int txcnt_eth = batch->count;\n- int dropped = 0;\n- int newcnt = 0;\n- int i;\n+ uint32_t cnt = batch->count;\n+ uint32_t dropped = 0;\n \n if (dev->type != DPDK_DEV_VHOST) {\n /* Check if QoS has been configured for this netdev. */\n- txcnt_eth = netdev_dpdk_qos_run(dev,\n- (struct rte_mbuf **)batch->packets,\n- txcnt_eth, false);\n- if (txcnt_eth == 0) {\n- dropped = batch->count;\n- goto out;\n- }\n+ cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **) batch->packets,\n+ cnt, false);\n+ dropped += batch->count - cnt;\n }\n \n dp_packet_batch_apply_cutlen(batch);\n \n- for (i = 0; i < batch->count; i++) {\n- int size = dp_packet_size(batch->packets[i]);\n+ uint32_t txcnt = 0;\n+\n+ for (uint32_t i = 0; i < cnt; i++) {\n+\n+ uint32_t size = dp_packet_size(batch->packets[i]);\n \n if (OVS_UNLIKELY(size > dev->max_packet_len)) {\n- VLOG_WARN_RL(&rl, \"Too big size %d max_packet_len %d\",\n- (int) size, dev->max_packet_len);\n+ VLOG_WARN_RL(&rl, \"Too big size %u max_packet_len %d\",\n+ size, dev->max_packet_len);\n \n dropped++;\n continue;\n }\n \n- pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n+ pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n \n- if (!pkts[newcnt]) {\n- dropped += batch->count - i;\n+ if (!pkts[txcnt]) {\n+ dropped += cnt - i;\n break;\n }\n \n /* We have to do a copy for now */\n- memcpy(rte_pktmbuf_mtod(pkts[newcnt], void *),\n+ memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),\n dp_packet_data(batch->packets[i]), size);\n \n- rte_pktmbuf_data_len(pkts[newcnt]) = size;\n- rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n+ rte_pktmbuf_data_len(pkts[txcnt]) = size;\n+ rte_pktmbuf_pkt_len(pkts[txcnt]) = size;\n \n- newcnt++;\n- if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n- dropped += batch->count - i - 1;\n- break;\n- }\n+ txcnt++;\n }\n \n- if (dev->type == DPDK_DEV_VHOST) {\n- __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n- newcnt);\n- } else {\n- dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n+ if (OVS_LIKELY(txcnt)) {\n+ if (dev->type == DPDK_DEV_VHOST) {\n+ __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n+ txcnt);\n+ } else {\n+ dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, txcnt);\n+ }\n }\n \n-out:\n if (OVS_UNLIKELY(dropped)) {\n rte_spinlock_lock(&dev->stats_lock);\n dev->stats.tx_dropped += dropped;\n\n\n//////////////////////////////////////////////////////////////\n\n\nOn 8/29/17, 4:39 AM, \"ovs-dev-bounces@openvswitch.org on behalf of Zhenyu Gao\" wrote:\n\n In dpdk_do_tx_copy function, all packets were copied to mbuf first,\n but QoS checking may drop some of them.\n Move the QoS checking in front of copying data to mbuf, it helps to\n reduce useless copy.\n \n Signed-off-by: Zhenyu Gao \n ---\n lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 36 insertions(+), 19 deletions(-)\n \n diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\n index 1aaf6f7..50874b8 100644\n --- a/lib/netdev-dpdk.c\n +++ b/lib/netdev-dpdk.c\n @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n * For all QoS implementations it should always be non-null.\n */\n int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n - int pkt_cnt);\n + int pkt_cnt, bool may_steal);\n };\n \n /* dpdk_qos_ops for each type of user space QoS implementation */\n @@ -1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\n \n static int\n netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n - struct rte_mbuf **pkts, int pkt_cnt)\n + struct rte_mbuf **pkts, int pkt_cnt,\n + bool may_steal)\n {\n int i = 0;\n int cnt = 0;\n @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n }\n cnt++;\n } else {\n - rte_pktmbuf_free(pkt);\n + if (may_steal) {\n + rte_pktmbuf_free(pkt);\n + }\n }\n }\n \n @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n \n static int\n ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf **pkts,\n - int pkt_cnt)\n + int pkt_cnt, bool may_steal)\n {\n int cnt = 0;\n \n rte_spinlock_lock(&policer->policer_lock);\n - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n + pkt_cnt, may_steal);\n rte_spinlock_unlock(&policer->policer_lock);\n \n return cnt;\n @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n dropped = nb_rx;\n nb_rx = ingress_policer_run(policer,\n (struct rte_mbuf **) batch->packets,\n - nb_rx);\n + nb_rx, true);\n dropped -= nb_rx;\n }\n \n @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)\n dropped = nb_rx;\n nb_rx = ingress_policer_run(policer,\n (struct rte_mbuf **) batch->packets,\n - nb_rx);\n + nb_rx, true);\n dropped -= nb_rx;\n }\n \n @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)\n \n static inline int\n netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n - int cnt)\n + int cnt, bool may_steal)\n {\n struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev->qos_conf);\n \n if (qos_conf) {\n rte_spinlock_lock(&qos_conf->lock);\n - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n rte_spinlock_unlock(&qos_conf->lock);\n }\n \n @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,\n \n cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n /* Check has QoS has been configured for the netdev */\n - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n dropped = total_pkts - cnt;\n \n do {\n @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n #endif\n struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n + int txcnt_eth = batch->count;\n int dropped = 0;\n int newcnt = 0;\n int i;\n \n + if (dev->type != DPDK_DEV_VHOST) {\n + /* Check if QoS has been configured for this netdev. */\n + txcnt_eth = netdev_dpdk_qos_run(dev,\n + (struct rte_mbuf **)batch->packets,\n + txcnt_eth, false);\n + if (txcnt_eth == 0) {\n + dropped = batch->count;\n + goto out;\n + }\n + }\n +\n dp_packet_batch_apply_cutlen(batch);\n \n for (i = 0; i < batch->count; i++) {\n @@ -1848,21 +1864,20 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n \n newcnt++;\n + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n + dropped += batch->count - i - 1;\n + break;\n + }\n }\n \n if (dev->type == DPDK_DEV_VHOST) {\n __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n newcnt);\n } else {\n - unsigned int qos_pkts = newcnt;\n -\n - /* Check if QoS has been configured for this netdev. */\n - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n -\n - dropped += qos_pkts - newcnt;\n dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n }\n \n +out:\n if (OVS_UNLIKELY(dropped)) {\n rte_spinlock_lock(&dev->stats_lock);\n dev->stats.tx_dropped += dropped;\n @@ -1915,7 +1930,7 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n dp_packet_batch_apply_cutlen(batch);\n \n cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n dropped = batch->count - cnt;\n \n dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt);\n @@ -3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf *conf,\n }\n \n static int\n -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt)\n +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt,\n + bool may_steal)\n {\n int cnt = 0;\n struct egress_policer *policer =\n CONTAINER_OF(conf, struct egress_policer, qos_conf);\n \n - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\n + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n + pkt_cnt, may_steal);\n \n return cnt;\n }\n -- \n 1.8.3.1\n \n _______________________________________________\n dev mailing list\n dev@openvswitch.org\n https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EZzkW98aZhxGnYEEmB3u5e3vaFA90qSr3aIM1t-ggFY&s=fHRBBnkb29ujBAr27aGj1MCWkVzjfKqmJz3R9wEg99o&e=","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"hN0tcnEw\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmsVT3fP8z9s7g\n\tfor ;\n\tWed, 6 Sep 2017 02:24:17 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 3FCC9A86;\n\tTue, 5 Sep 2017 16:24:14 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 7A07A904\n\tfor ; Tue, 5 Sep 2017 16:24:13 +0000 (UTC)","from NAM02-CY1-obe.outbound.protection.outlook.com\n\t(mail-cys01nam02on0053.outbound.protection.outlook.com\n\t[104.47.37.53])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 6FE17481\n\tfor ; Tue, 5 Sep 2017 16:24:11 +0000 (UTC)","from BLUPR05MB611.namprd05.prod.outlook.com (10.141.204.27) by\n\tBLUPR05MB564.namprd05.prod.outlook.com (10.141.202.150) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id\n\t15.20.35.3; Tue, 5 Sep 2017 16:24:09 +0000","from BLUPR05MB611.namprd05.prod.outlook.com ([10.141.204.27]) by\n\tBLUPR05MB611.namprd05.prod.outlook.com ([10.141.204.27]) with mapi id\n\t15.20.0035.010; Tue, 5 Sep 2017 16:24:08 +0000"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=onevmw.onmicrosoft.com; s=selector1-vmware-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=xbynhYCLOEhN7OTDmp576DDptnpoE1VZLLxPFd/l7n0=;\n\tb=hN0tcnEwaRBKTIqDmOjZc9u9Yc5bOjFGZuB4YryIx+Lm+tLj9ukF95Qhxk0HSyGy6sppI+1ngye9qR/jPAqp7S9yeYU6uht8KDcWW0bCSQAbAYT6kvu8HJpXE3K0N2iK25FCt2ha24pEMlc/KNTyCoI0c2r769GGlyXoKEshifk=","From":"Darrell Ball ","To":"Zhenyu Gao , \"ian.stokes@intel.com\"\n\t, \"dev@openvswitch.org\" ","Thread-Topic":"[ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","Thread-Index":"AQHTJmNm5xRXfwzkwES+df7A1GLf5A==","Date":"Tue, 5 Sep 2017 16:24:08 +0000","Message-ID":"<508BD058-F68B-4344-B347-B1CBE8D2684A@vmware.com>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>","In-Reply-To":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","user-agent":"Microsoft-MacOutlook/f.23.0.170610","x-originating-ip":"[73.162.236.45]","x-ms-publictraffictype":"Email","x-microsoft-exchange-diagnostics":"1; BLUPR05MB564;\n\t20:MpXl9IIJJsg7qOqLyUXYLw2HmC1HuCiC10Scua5lIoWAy7deox5xyu1u00pC2x+c451SPuAMKarMc8gdLIqYroNWXeWyU+17dl+4KjSHGAHZMYq3eWLDWBLGM1kOS5PiWxknVjGvNK4lo9C/dZm2fVpGjT3XLEjREIF/0gmMW4w=","x-ms-exchange-antispam-srfa-diagnostics":"SSOS;","x-ms-office365-filtering-correlation-id":"e5d511fc-d293-419e-506c-08d4f47a891b","x-microsoft-antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(300000500095)(300135000095)(300000501095)(300135300095)(300000502095)(300135100095)(22001)(2017030254152)(300000503095)(300135400095)(2017052603199)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095);\n\tSRVR:BLUPR05MB564; ","x-ms-traffictypediagnostic":"BLUPR05MB564:","authentication-results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"hN0tcnEw\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"x-exchange-antispam-report-test":"UriScan:(10436049006162)(216315784871565);","x-microsoft-antispam-prvs":"","x-exchange-antispam-report-cfa-test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(93006095)(93001095)(3002001)(10201501046)(100000703101)(100105400095)(6041248)(20161123558100)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(20161123564025)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:BLUPR05MB564; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:BLUPR05MB564; ","x-forefront-prvs":"0421BF7135","x-forefront-antispam-report":"SFV:NSPM;\n\tSFS:(10009020)(6009001)(199003)(377454003)(189002)(24454002)(50986999)(76176999)(54356999)(39060400002)(3280700002)(2201001)(14454004)(2950100002)(478600001)(2906002)(101416001)(81166006)(83506001)(966005)(8676002)(81156014)(66066001)(83716003)(189998001)(106356001)(105586002)(5660300001)(82746002)(8936002)(36756003)(97736004)(53546010)(4001350100001)(3660700001)(2900100001)(33656002)(6506006)(6306002)(6512007)(99286003)(68736007)(6436002)(6116002)(2501003)(53936002)(86362001)(25786009)(3846002)(102836003)(305945005)(229853002)(77096006)(6486002)(7736002)(6246003);\n\tDIR:OUT; SFP:1101; SCL:1; SRVR:BLUPR05MB564;\n\tH:BLUPR05MB611.namprd05.prod.outlook.com; FPR:; SPF:None;\n\tPTR:InfoNoRecords; A:1; MX:1; LANG:en; ","received-spf":"None (protection.outlook.com: vmware.com does not designate\n\tpermitted sender hosts)","spamdiagnosticoutput":"1:99","spamdiagnosticmetadata":"NSPM","Content-ID":"","MIME-Version":"1.0","X-OriginatorOrg":"vmware.com","X-MS-Exchange-CrossTenant-originalarrivaltime":"05 Sep 2017 16:24:08.8913\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"BLUPR05MB564","X-Spam-Status":"No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tRCVD_IN_DNSWL_NONE autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1763835,"web_url":"http://patchwork.ozlabs.org/comment/1763835/","msgid":"","list_archive_url":null,"date":"2017-09-06T06:39:35","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":70513,"url":"http://patchwork.ozlabs.org/api/people/70513/","name":"Gao Zhenyu","email":"sysugaozhenyu@gmail.com"},"content":"Thanks Derrell, it makes sence to me and I think the code path is more\nclear.\n\nThanks\nZhenyu Gao\n\n2017-09-06 0:24 GMT+08:00 Darrell Ball :\n\n> Hi Gao\n>\n> How about the following incremental ?\n>\n> Thanks Darrell\n>\n>\n>\n> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\n> index 4808d38..648d719 100644\n> --- a/lib/netdev-dpdk.c\n> +++ b/lib/netdev-dpdk.c\n> @@ -1843,64 +1843,58 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> struct dp_packet_batch *batch)\n> #endif\n> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> - int txcnt_eth = batch->count;\n> - int dropped = 0;\n> - int newcnt = 0;\n> - int i;\n> + uint32_t cnt = batch->count;\n> + uint32_t dropped = 0;\n>\n> if (dev->type != DPDK_DEV_VHOST) {\n> /* Check if QoS has been configured for this netdev. */\n> - txcnt_eth = netdev_dpdk_qos_run(dev,\n> - (struct rte_mbuf\n> **)batch->packets,\n> - txcnt_eth, false);\n> - if (txcnt_eth == 0) {\n> - dropped = batch->count;\n> - goto out;\n> - }\n> + cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **)\n> batch->packets,\n> + cnt, false);\n> + dropped += batch->count - cnt;\n> }\n>\n> dp_packet_batch_apply_cutlen(batch);\n>\n> - for (i = 0; i < batch->count; i++) {\n> - int size = dp_packet_size(batch->packets[i]);\n> + uint32_t txcnt = 0;\n> +\n> + for (uint32_t i = 0; i < cnt; i++) {\n> +\n> + uint32_t size = dp_packet_size(batch->packets[i]);\n>\n> if (OVS_UNLIKELY(size > dev->max_packet_len)) {\n> - VLOG_WARN_RL(&rl, \"Too big size %d max_packet_len %d\",\n> - (int) size, dev->max_packet_len);\n> + VLOG_WARN_RL(&rl, \"Too big size %u max_packet_len %d\",\n> + size, dev->max_packet_len);\n>\n> dropped++;\n> continue;\n> }\n>\n> - pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n> + pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n>\n> - if (!pkts[newcnt]) {\n> - dropped += batch->count - i;\n> + if (!pkts[txcnt]) {\n> + dropped += cnt - i;\n> break;\n> }\n>\n> /* We have to do a copy for now */\n> - memcpy(rte_pktmbuf_mtod(pkts[newcnt], void *),\n> + memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),\n> dp_packet_data(batch->packets[i]), size);\n>\n> - rte_pktmbuf_data_len(pkts[newcnt]) = size;\n> - rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> + rte_pktmbuf_data_len(pkts[txcnt]) = size;\n> + rte_pktmbuf_pkt_len(pkts[txcnt]) = size;\n>\n> - newcnt++;\n> - if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> - dropped += batch->count - i - 1;\n> - break;\n> - }\n> + txcnt++;\n> }\n>\n> - if (dev->type == DPDK_DEV_VHOST) {\n> - __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n> - newcnt);\n> - } else {\n> - dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> + if (OVS_LIKELY(txcnt)) {\n> + if (dev->type == DPDK_DEV_VHOST) {\n> + __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)\n> pkts,\n> + txcnt);\n> + } else {\n> + dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, txcnt);\n> + }\n> }\n>\n> -out:\n> if (OVS_UNLIKELY(dropped)) {\n> rte_spinlock_lock(&dev->stats_lock);\n> dev->stats.tx_dropped += dropped;\n>\n>\n> //////////////////////////////////////////////////////////////\n>\n>\n> On 8/29/17, 4:39 AM, \"ovs-dev-bounces@openvswitch.org on behalf of Zhenyu\n> Gao\" \n> wrote:\n>\n> In dpdk_do_tx_copy function, all packets were copied to mbuf first,\n> but QoS checking may drop some of them.\n> Move the QoS checking in front of copying data to mbuf, it helps to\n> reduce useless copy.\n>\n> Signed-off-by: Zhenyu Gao \n> ---\n> lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++\n> ++++++-------------------\n> 1 file changed, 36 insertions(+), 19 deletions(-)\n>\n> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\n> index 1aaf6f7..50874b8 100644\n> --- a/lib/netdev-dpdk.c\n> +++ b/lib/netdev-dpdk.c\n> @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> * For all QoS implementations it should always be non-null.\n> */\n> int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> - int pkt_cnt);\n> + int pkt_cnt, bool may_steal);\n> };\n>\n> /* dpdk_qos_ops for each type of user space QoS implementation */\n> @@ -1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct\n> rte_meter_srtcm *meter,\n>\n> static int\n> netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> - struct rte_mbuf **pkts, int pkt_cnt)\n> + struct rte_mbuf **pkts, int pkt_cnt,\n> + bool may_steal)\n> {\n> int i = 0;\n> int cnt = 0;\n> @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> *meter,\n> }\n> cnt++;\n> } else {\n> - rte_pktmbuf_free(pkt);\n> + if (may_steal) {\n> + rte_pktmbuf_free(pkt);\n> + }\n> }\n> }\n>\n> @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> *meter,\n>\n> static int\n> ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> **pkts,\n> - int pkt_cnt)\n> + int pkt_cnt, bool may_steal)\n> {\n> int cnt = 0;\n>\n> rte_spinlock_lock(&policer->policer_lock);\n> - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> pkt_cnt);\n> + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> + pkt_cnt, may_steal);\n> rte_spinlock_unlock(&policer->policer_lock);\n>\n> return cnt;\n> @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq\n> *rxq,\n> dropped = nb_rx;\n> nb_rx = ingress_policer_run(policer,\n> (struct rte_mbuf **)\n> batch->packets,\n> - nb_rx);\n> + nb_rx, true);\n> dropped -= nb_rx;\n> }\n>\n> @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> struct dp_packet_batch *batch)\n> dropped = nb_rx;\n> nb_rx = ingress_policer_run(policer,\n> (struct rte_mbuf **)\n> batch->packets,\n> - nb_rx);\n> + nb_rx, true);\n> dropped -= nb_rx;\n> }\n>\n> @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> struct dp_packet_batch *batch)\n>\n> static inline int\n> netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> - int cnt)\n> + int cnt, bool may_steal)\n> {\n> struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *,\n> &dev->qos_conf);\n>\n> if (qos_conf) {\n> rte_spinlock_lock(&qos_conf->lock);\n> - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> rte_spinlock_unlock(&qos_conf->lock);\n> }\n>\n> @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,\n> int qid,\n>\n> cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> /* Check has QoS has been configured for the netdev */\n> - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> dropped = total_pkts - cnt;\n>\n> do {\n> @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int\n> qid, struct dp_packet_batch *batch)\n> #endif\n> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> + int txcnt_eth = batch->count;\n> int dropped = 0;\n> int newcnt = 0;\n> int i;\n>\n> + if (dev->type != DPDK_DEV_VHOST) {\n> + /* Check if QoS has been configured for this netdev. */\n> + txcnt_eth = netdev_dpdk_qos_run(dev,\n> + (struct rte_mbuf\n> **)batch->packets,\n> + txcnt_eth, false);\n> + if (txcnt_eth == 0) {\n> + dropped = batch->count;\n> + goto out;\n> + }\n> + }\n> +\n> dp_packet_batch_apply_cutlen(batch);\n>\n> for (i = 0; i < batch->count; i++) {\n> @@ -1848,21 +1864,20 @@ dpdk_do_tx_copy(struct netdev *netdev, int\n> qid, struct dp_packet_batch *batch)\n> rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n>\n> newcnt++;\n> + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> + dropped += batch->count - i - 1;\n> + break;\n> + }\n> }\n>\n> if (dev->type == DPDK_DEV_VHOST) {\n> __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **)\n> pkts,\n> newcnt);\n> } else {\n> - unsigned int qos_pkts = newcnt;\n> -\n> - /* Check if QoS has been configured for this netdev. */\n> - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> -\n> - dropped += qos_pkts - newcnt;\n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> }\n>\n> +out:\n> if (OVS_UNLIKELY(dropped)) {\n> rte_spinlock_lock(&dev->stats_lock);\n> dev->stats.tx_dropped += dropped;\n> @@ -1915,7 +1930,7 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int\n> qid,\n> dp_packet_batch_apply_cutlen(batch);\n>\n> cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> dropped = batch->count - cnt;\n>\n> dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt);\n> @@ -3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct\n> qos_conf *conf,\n> }\n>\n> static int\n> -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> pkt_cnt)\n> +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> pkt_cnt,\n> + bool may_steal)\n> {\n> int cnt = 0;\n> struct egress_policer *policer =\n> CONTAINER_OF(conf, struct egress_policer, qos_conf);\n>\n> - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> pkt_cnt);\n> + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> + pkt_cnt, may_steal);\n>\n> return cnt;\n> }\n> --\n> 1.8.3.1\n>\n> _______________________________________________\n> dev mailing list\n> dev@openvswitch.org\n> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.\n> openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=\n> uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=\n> EZzkW98aZhxGnYEEmB3u5e3vaFA90qSr3aIM1t-ggFY&s=\n> fHRBBnkb29ujBAr27aGj1MCWkVzjfKqmJz3R9wEg99o&e=\n>\n>\n>\n>\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"J/FBMQyQ\"; dkim-atps=neutral"],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnDTV1ZcGz9sRV\n\tfor ;\n\tWed, 6 Sep 2017 16:39:42 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id F1ACFA7A;\n\tWed, 6 Sep 2017 06:39:38 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 44194722\n\tfor ; Wed, 6 Sep 2017 06:39:38 +0000 (UTC)","from mail-pg0-f68.google.com (mail-pg0-f68.google.com\n\t[74.125.83.68])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 18F2A20C\n\tfor ; Wed, 6 Sep 2017 06:39:37 +0000 (UTC)","by mail-pg0-f68.google.com with SMTP id q68so3058195pgq.2\n\tfor ; Tue, 05 Sep 2017 23:39:37 -0700 (PDT)","by 10.100.151.235 with HTTP; Tue, 5 Sep 2017 23:39:35 -0700 (PDT)"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=maVw/p9VEUTHkRRLN2WDtIuGUWGf9K4AsO93MbTVFVI=;\n\tb=J/FBMQyQnjX3SLJfzjPItKJip/24ghpjoOwpMGNXaQTD0cqwUakCuQo9aVmekrDCnq\n\tdLhhfIeeKLToWm2iwxslB/1sjDVf5H1DAReKEmAYZ2gmG3Z5SbidDlAkKRjn/Jz3Go8D\n\trpeNOhwU+asQkTQ7sIxDZ5owsJ4/2x5zzkKo4gokpYwoflSz62ldDNx+6nHIW4zZQzoo\n\tzwWIaRs2GJwmgUenQnTQ0PkytCK0ysDMk+l6aytdvLguw89+0iQKVD6dPKrXpMyyQqtr\n\t90CgyKbdPN7CIUki2nZ9U4/hfLzoYo8mqcy3qtIdwQnFzH2YrAaHHoz2haZF/9FzFs95\n\tbt0A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=maVw/p9VEUTHkRRLN2WDtIuGUWGf9K4AsO93MbTVFVI=;\n\tb=DmzvqEDO97qYRxXn7WSmWIYqr3g6ZgxUX5ygZP/4uUEwEfvkTPnKLeQ8UbNT4FXc+Z\n\tndqOGp3gfOMlY1JTRuMWBfN8YZ7GSrvBEe4KmNnyE+0ZViMBZFTOx6r0uLl8Oo4xE2qe\n\tg/LUT5ZqXP6XcrP8CdPgf1Ik+ZNWJbLnvocyD9KCFUK1gm0y19NKzkqrVp1Lv3viBej8\n\tVNu+vrpG0MF8aWO3EktzBLeqL/ej+traN63FjUmKe1aXIS3j55OCQbElO/qhJQV6Oymg\n\twqvEMSAjFlM4mCHLqxyA4/iBZPXaUDTAUHrg8oB+WwkPyN0pDBJQOX+uhhkX3/t7xUCg\n\twvIw==","X-Gm-Message-State":"AHPjjUgDxqCMQIJzypdmlw34RXsX/4LzxU3Cb03Vmv299V05jXqZSyzY\n\tMwoYYPLSs+csdCHsjJ32GRPGKuKLZg==","X-Google-Smtp-Source":"ADKCNb707QaxXW5JuNhPQlDHF4I8yqBuupPh5M+3PeqP8Z79UKqm/FjL/saqxkHc34oyAkKfGUCDJaxiaZiRrF1olVc=","X-Received":"by 10.98.100.206 with SMTP id y197mr6305116pfb.53.1504679976553; \n\tTue, 05 Sep 2017 23:39:36 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<508BD058-F68B-4344-B347-B1CBE8D2684A@vmware.com>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t<508BD058-F68B-4344-B347-B1CBE8D2684A@vmware.com>","From":"Gao Zhenyu ","Date":"Wed, 6 Sep 2017 14:39:35 +0800","Message-ID":"","To":"Darrell Ball ","X-Spam-Status":"No, score=0.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tDKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1764529,"web_url":"http://patchwork.ozlabs.org/comment/1764529/","msgid":"<7263C55A-BDA6-450E-909B-4D159B250FBD@vmware.com>","list_archive_url":null,"date":"2017-09-07T06:14:54","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":68212,"url":"http://patchwork.ozlabs.org/api/people/68212/","name":"Darrell Ball","email":"dball@vmware.com"},"content":"Hi Gao\n\nI had applied the patch to dpdk_merge here\n\nhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_darball_ovs_commits_dpdk-5Fmerge&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=A2_FCacqbp2moAo3HGFlTuxsjONUGhlN42OBcAuQQ6w&s=b6btPKhgvOFr2GOUYvktND6kaC6jc3fXI-mXfvNgXOU&e=\n\nPlease let me know if I missed anything.\n\nThanks Darrell\n\n\nFrom: Gao Zhenyu \nDate: Tuesday, September 5, 2017 at 11:39 PM\nTo: Darrel Ball \nCc: \"ian.stokes@intel.com\" , \"dev@openvswitch.org\" \nSubject: Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf\n\nThanks Derrell, it makes sence to me and I think the code path is more clear.\nThanks\nZhenyu Gao\n\n2017-09-06 0:24 GMT+08:00 Darrell Ball >:\nHi Gao\n\nHow about the following incremental ?\n\nThanks Darrell\n\n\n\ndiff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\nindex 4808d38..648d719 100644\n--- a/lib/netdev-dpdk.c\n+++ b/lib/netdev-dpdk.c\n@@ -1843,64 +1843,58 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n #endif\n struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n- int txcnt_eth = batch->count;\n- int dropped = 0;\n- int newcnt = 0;\n- int i;\n+ uint32_t cnt = batch->count;\n+ uint32_t dropped = 0;\n\n if (dev->type != DPDK_DEV_VHOST) {\n /* Check if QoS has been configured for this netdev. */\n- txcnt_eth = netdev_dpdk_qos_run(dev,\n- (struct rte_mbuf **)batch->packets,\n- txcnt_eth, false);\n- if (txcnt_eth == 0) {\n- dropped = batch->count;\n- goto out;\n- }\n+ cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **) batch->packets,\n+ cnt, false);\n+ dropped += batch->count - cnt;\n }\n\n dp_packet_batch_apply_cutlen(batch);\n\n- for (i = 0; i < batch->count; i++) {\n- int size = dp_packet_size(batch->packets[i]);\n+ uint32_t txcnt = 0;\n+\n+ for (uint32_t i = 0; i < cnt; i++) {\n+\n+ uint32_t size = dp_packet_size(batch->packets[i]);\n\n if (OVS_UNLIKELY(size > dev->max_packet_len)) {\n- VLOG_WARN_RL(&rl, \"Too big size %d max_packet_len %d\",\n- (int) size, dev->max_packet_len);\n+ VLOG_WARN_RL(&rl, \"Too big size %u max_packet_len %d\",\n+ size, dev->max_packet_len);\n\n dropped++;\n continue;\n }\n\n- pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n+ pkts[txcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);\n\n- if (!pkts[newcnt]) {\n- dropped += batch->count - i;\n+ if (!pkts[txcnt]) {\n+ dropped += cnt - i;\n break;\n }\n\n /* We have to do a copy for now */\n- memcpy(rte_pktmbuf_mtod(pkts[newcnt], void *),\n+ memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),\n dp_packet_data(batch->packets[i]), size);\n\n- rte_pktmbuf_data_len(pkts[newcnt]) = size;\n- rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n+ rte_pktmbuf_data_len(pkts[txcnt]) = size;\n+ rte_pktmbuf_pkt_len(pkts[txcnt]) = size;\n\n- newcnt++;\n- if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n- dropped += batch->count - i - 1;\n- break;\n- }\n+ txcnt++;\n }\n\n- if (dev->type == DPDK_DEV_VHOST) {\n- __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n- newcnt);\n- } else {\n- dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n+ if (OVS_LIKELY(txcnt)) {\n+ if (dev->type == DPDK_DEV_VHOST) {\n+ __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n+ txcnt);\n+ } else {\n+ dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, txcnt);\n+ }\n }\n\n-out:\n if (OVS_UNLIKELY(dropped)) {\n rte_spinlock_lock(&dev->stats_lock);\n dev->stats.tx_dropped += dropped;\n\n\n//////////////////////////////////////////////////////////////\n\n\nOn 8/29/17, 4:39 AM, \"ovs-dev-bounces@openvswitch.org on behalf of Zhenyu Gao\" on behalf of sysugaozhenyu@gmail.com> wrote:\n\n In dpdk_do_tx_copy function, all packets were copied to mbuf first,\n but QoS checking may drop some of them.\n Move the QoS checking in front of copying data to mbuf, it helps to\n reduce useless copy.\n\n Signed-off-by: Zhenyu Gao >\n ---\n lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 36 insertions(+), 19 deletions(-)\n\n diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c\n index 1aaf6f7..50874b8 100644\n --- a/lib/netdev-dpdk.c\n +++ b/lib/netdev-dpdk.c\n @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n * For all QoS implementations it should always be non-null.\n */\n int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n - int pkt_cnt);\n + int pkt_cnt, bool may_steal);\n };\n\n /* dpdk_qos_ops for each type of user space QoS implementation */\n @@ -1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\n\n static int\n netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n - struct rte_mbuf **pkts, int pkt_cnt)\n + struct rte_mbuf **pkts, int pkt_cnt,\n + bool may_steal)\n {\n int i = 0;\n int cnt = 0;\n @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n }\n cnt++;\n } else {\n - rte_pktmbuf_free(pkt);\n + if (may_steal) {\n + rte_pktmbuf_free(pkt);\n + }\n }\n }\n\n @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n\n static int\n ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf **pkts,\n - int pkt_cnt)\n + int pkt_cnt, bool may_steal)\n {\n int cnt = 0;\n\n rte_spinlock_lock(&policer->policer_lock);\n - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n + pkt_cnt, may_steal);\n rte_spinlock_unlock(&policer->policer_lock);\n\n return cnt;\n @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n dropped = nb_rx;\n nb_rx = ingress_policer_run(policer,\n (struct rte_mbuf **) batch->packets,\n - nb_rx);\n + nb_rx, true);\n dropped -= nb_rx;\n }\n\n @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)\n dropped = nb_rx;\n nb_rx = ingress_policer_run(policer,\n (struct rte_mbuf **) batch->packets,\n - nb_rx);\n + nb_rx, true);\n dropped -= nb_rx;\n }\n\n @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct dp_packet_batch *batch)\n\n static inline int\n netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n - int cnt)\n + int cnt, bool may_steal)\n {\n struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev->qos_conf);\n\n if (qos_conf) {\n rte_spinlock_lock(&qos_conf->lock);\n - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n rte_spinlock_unlock(&qos_conf->lock);\n }\n\n @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,\n\n cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n /* Check has QoS has been configured for the netdev */\n - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n dropped = total_pkts - cnt;\n\n do {\n @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n #endif\n struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n + int txcnt_eth = batch->count;\n int dropped = 0;\n int newcnt = 0;\n int i;\n\n + if (dev->type != DPDK_DEV_VHOST) {\n + /* Check if QoS has been configured for this netdev. */\n + txcnt_eth = netdev_dpdk_qos_run(dev,\n + (struct rte_mbuf **)batch->packets,\n + txcnt_eth, false);\n + if (txcnt_eth == 0) {\n + dropped = batch->count;\n + goto out;\n + }\n + }\n +\n dp_packet_batch_apply_cutlen(batch);\n\n for (i = 0; i < batch->count; i++) {\n @@ -1848,21 +1864,20 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)\n rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n\n newcnt++;\n + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n + dropped += batch->count - i - 1;\n + break;\n + }\n }\n\n if (dev->type == DPDK_DEV_VHOST) {\n __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n newcnt);\n } else {\n - unsigned int qos_pkts = newcnt;\n -\n - /* Check if QoS has been configured for this netdev. */\n - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n -\n - dropped += qos_pkts - newcnt;\n dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n }\n\n +out:\n if (OVS_UNLIKELY(dropped)) {\n rte_spinlock_lock(&dev->stats_lock);\n dev->stats.tx_dropped += dropped;\n @@ -1915,7 +1930,7 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n dp_packet_batch_apply_cutlen(batch);\n\n cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n dropped = batch->count - cnt;\n\n dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt);\n @@ -3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf *conf,\n }\n\n static int\n -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt)\n +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int pkt_cnt,\n + bool may_steal)\n {\n int cnt = 0;\n struct egress_policer *policer =\n CONTAINER_OF(conf, struct egress_policer, qos_conf);\n\n - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\n + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n + pkt_cnt, may_steal);\n\n return cnt;\n }\n --\n 1.8.3.1\n _______________________________________________\n dev mailing list\n dev@openvswitch.org\n https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EZzkW98aZhxGnYEEmB3u5e3vaFA90qSr3aIM1t-ggFY&s=fHRBBnkb29ujBAr27aGj1MCWkVzjfKqmJz3R9wEg99o&e=","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"F6V04idH\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnqtf6wflz9sRY\n\tfor ;\n\tThu, 7 Sep 2017 16:15:06 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 013AF5A7;\n\tThu, 7 Sep 2017 06:15:02 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 9C97E5A7\n\tfor ; Thu, 7 Sep 2017 06:15:00 +0000 (UTC)","from NAM02-CY1-obe.outbound.protection.outlook.com\n\t(mail-cys01nam02on0057.outbound.protection.outlook.com\n\t[104.47.37.57])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id C68DE1B3\n\tfor ; Thu, 7 Sep 2017 06:14:58 +0000 (UTC)","from BLUPR05MB611.namprd05.prod.outlook.com (10.141.204.27) by\n\tBLUPR05MB754.namprd05.prod.outlook.com (10.141.208.142) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id\n\t15.20.56.4; Thu, 7 Sep 2017 06:14:55 +0000","from BLUPR05MB611.namprd05.prod.outlook.com ([10.141.204.27]) by\n\tBLUPR05MB611.namprd05.prod.outlook.com ([10.141.204.27]) with mapi id\n\t15.20.0056.003; Thu, 7 Sep 2017 06:14:55 +0000"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=onevmw.onmicrosoft.com; s=selector1-vmware-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=2jnKBvlibD7DIXgcNy/Mk4eDy3DhM3eeJEx/78crLnI=;\n\tb=F6V04idHAaO3aLM3iraR645l3VvWtaKHd6PKXi4j+9XZ33BuKIRUV+jvrBcvKYkaoAJlp+Sn6tkO3J/e17rgOpxGUyKew2YZSZ5J+5djve6akaHDQG89EiEpmRErHAmsmGxhWUQcFQCK5clbow28jrwSRa9p9IlpXLjpi8WZJ10=","From":"Darrell Ball ","To":"Gao Zhenyu ","Thread-Topic":"[ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","Thread-Index":"AQHTJmNm5xRXfwzkwES+df7A1GLf5KKnaQeAgAEWFoA=","Date":"Thu, 7 Sep 2017 06:14:54 +0000","Message-ID":"<7263C55A-BDA6-450E-909B-4D159B250FBD@vmware.com>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t<508BD058-F68B-4344-B347-B1CBE8D2684A@vmware.com>\n\t","In-Reply-To":"","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","user-agent":"Microsoft-MacOutlook/f.23.0.170610","x-originating-ip":"[73.162.236.45]","x-ms-publictraffictype":"Email","x-microsoft-exchange-diagnostics":"1; BLUPR05MB754;\n\t20:OFsSOwYB1muqZ+HTzCEgwFRVz+F0KhHtjrIrWI34zmsHSKOk7MaFiVcZ4iL6l+Z1IWADGQmGC91q90X9dAb7yTzdEpHqoAhZLE6Jzkmwv5qZSOo/CyOPq258I41RvpeIhfTLgBJB9agUOeWf5R9uTKYIgmQIOu+TC/e3YG18ubk=","x-ms-exchange-antispam-srfa-diagnostics":"SSOS;","x-ms-office365-filtering-correlation-id":"8d19734e-bff5-4d36-941f-08d4f5b7c23b","x-microsoft-antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(300000503095)(300135400095)(2017052603199)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095);\n\tSRVR:BLUPR05MB754; ","x-ms-traffictypediagnostic":"BLUPR05MB754:","authentication-results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"F6V04idH\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"x-exchange-antispam-report-test":"UriScan:(61668805478150)(10436049006162)(216315784871565)(21748063052155)(228905959029699);","x-microsoft-antispam-prvs":"","x-exchange-antispam-report-cfa-test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(100000703101)(100105400095)(6041248)(20161123558100)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123562025)(20161123560025)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:BLUPR05MB754; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:BLUPR05MB754; ","x-forefront-prvs":"04238CD941","x-forefront-antispam-report":"SFV:NSPM;\n\tSFS:(10009020)(377424004)(377454003)(24454002)(199003)(189002)(83716003)(4326008)(76176999)(106356001)(2950100002)(101416001)(105586002)(6436002)(110136004)(6506006)(39060400002)(6916009)(8676002)(6486002)(68736007)(81156014)(81166006)(7736002)(8936002)(50986999)(54896002)(6306002)(5660300001)(53936002)(33656002)(6512007)(54356999)(236005)(77096006)(229853002)(86362001)(97736004)(606006)(82746002)(4001350100001)(25786009)(53546010)(66066001)(2900100001)(3846002)(54906002)(102836003)(6116002)(14454004)(966005)(2906002)(6246003)(36756003)(189998001)(1411001)(3280700002)(3660700001)(478600001)(83506001)(99286003);\n\tDIR:OUT; SFP:1101; SCL:1; SRVR:BLUPR05MB754;\n\tH:BLUPR05MB611.namprd05.prod.outlook.com; FPR:; SPF:None;\n\tPTR:InfoNoRecords; A:1; MX:1; LANG:en; ","received-spf":"None (protection.outlook.com: vmware.com does not designate\n\tpermitted sender hosts)","spamdiagnosticoutput":"1:99","spamdiagnosticmetadata":"NSPM","MIME-Version":"1.0","X-OriginatorOrg":"vmware.com","X-MS-Exchange-CrossTenant-originalarrivaltime":"07 Sep 2017 06:14:55.0243\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"BLUPR05MB754","X-Spam-Status":"No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tHTML_MESSAGE,RCVD_IN_DNSWL_NONE autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1796278,"web_url":"http://patchwork.ozlabs.org/comment/1796278/","msgid":"<20171030213749.GC27530@ovn.org>","list_archive_url":null,"date":"2017-10-30T21:37:49","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"Hi Ian and Gao, it looks like this patch never got applied. Gao, is it\nstill relevant and, if so, Ian, will you add it to your next batch of\npatches?\n\nThanks,\n\nBen.\n\nOn Sun, Sep 03, 2017 at 10:46:24AM +0000, Stokes, Ian wrote:\n> OK,\n> \n> In my own testing I’m seeing the same behavior with a tap interface pinging to a bridge with a DPDK device. As such I think this should be ok.\n> \n> Acked-by: ian.stokes@intel.com\n> \n> Ian\n> \n> From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]\n> Sent: Thursday, August 31, 2017 2:56 AM\n> To: Stokes, Ian \n> Cc: dev@openvswitch.org\n> Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf\n> \n> Here is the the testing I just did:\n> 1. Add log in function netdev_dpdk_policer_pkt_handle\n> static inline bool\n> netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\n> struct rte_mbuf *pkt, uint64_t time)\n> {\n> uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr);\n> VLOG_WARN(\"GGGG, payload_len:%u, rte_pkt_len:%u\",\n> pkt_len, rte_pktmbuf_pkt_len(pkt)); <------------------------print out pkt_len\n> \n> return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==\n> e_RTE_METER_GREEN;\n> }\n> 2. rebuild a rpm, install in machine, enable DPDK.\n> 3. Create a container on the bridge(userspace bridge). (The container use veth device)\n> 4. Config qos on eth dpdk eth by using : ovs-vsctl set port dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048\n> 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can see:\n> \n> 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> So the mbuf->pkt_len has a correct length.\n> On the code side, we have function dp_packet_set_size(), if ovs compiled with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c\n> \n> #ifdef DPDK_NETDEV\n> ......\n> \n> static inline void\n> dp_packet_set_size(struct dp_packet *b, uint32_t v)\n> {\n> .....\n> b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\n> b->mbuf.pkt_len = v; /* Total length of all segments linked to\n> * this segment. */\n> }\n> Please let me know if you have any concern on it.\n> \n> Thanks\n> Zhenyu Gao\n> \n> 2017-08-30 23:18 GMT+08:00 Stokes, Ian >:\n> > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\n> > QoS checking may drop some of them.\n> > Move the QoS checking in front of copying data to mbuf, it helps to reduce\n> > useless copy.\n> \n> Hi Zhenyu, thanks for the patch, I agree with the objective of the patch but have some comments below I'd like to see addressed/tested before signing off.\n> \n> >\n> > Signed-off-by: Zhenyu Gao >\n> > ---\n> > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++--------------\n> > -----\n> > 1 file changed, 36 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\n> > 100644\n> > --- a/lib/netdev-dpdk.c\n> > +++ b/lib/netdev-dpdk.c\n> > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> > * For all QoS implementations it should always be non-null.\n> > */\n> > int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> > - int pkt_cnt);\n> > + int pkt_cnt, bool may_steal);\n> > };\n> >\n> > /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\n> > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> > - struct rte_mbuf **pkts, int pkt_cnt)\n> > + struct rte_mbuf **pkts, int pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int i = 0;\n> > int cnt = 0;\n> > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> > }\n> > cnt++;\n> > } else {\n> > - rte_pktmbuf_free(pkt);\n> > + if (may_steal) {\n> > + rte_pktmbuf_free(pkt);\n> > + }\n> > }\n> > }\n> >\n> > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > *meter,\n> >\n> > static int\n> > ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> > **pkts,\n> > - int pkt_cnt)\n> > + int pkt_cnt, bool may_steal)\n> > {\n> > int cnt = 0;\n> >\n> > rte_spinlock_lock(&policer->policer_lock);\n> > - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> > + pkt_cnt, may_steal);\n> > rte_spinlock_unlock(&policer->policer_lock);\n> >\n> > return cnt;\n> > @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\n> > dp_packet_batch *batch)\n> > dropped = nb_rx;\n> > nb_rx = ingress_policer_run(policer,\n> > (struct rte_mbuf **) batch->packets,\n> > - nb_rx);\n> > + nb_rx, true);\n> > dropped -= nb_rx;\n> > }\n> >\n> > @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> > struct dp_packet_batch *batch)\n> >\n> > static inline int\n> > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> > - int cnt)\n> > + int cnt, bool may_steal)\n> > {\n> > struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\n> > >qos_conf);\n> >\n> > if (qos_conf) {\n> > rte_spinlock_lock(&qos_conf->lock);\n> > - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> > + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> > rte_spinlock_unlock(&qos_conf->lock);\n> > }\n> >\n> > @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int\n> > qid,\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> > /* Check has QoS has been configured for the netdev */\n> > - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> > dropped = total_pkts - cnt;\n> >\n> > do {\n> > @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> > struct dp_packet_batch *batch) #endif\n> > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> > struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> > + int txcnt_eth = batch->count;\n> > int dropped = 0;\n> > int newcnt = 0;\n> > int i;\n> >\n> > + if (dev->type != DPDK_DEV_VHOST) {\n> > + /* Check if QoS has been configured for this netdev. */\n> > + txcnt_eth = netdev_dpdk_qos_run(dev,\n> > + (struct rte_mbuf **)batch-\n> > >packets,\n> > + txcnt_eth, false);\n> So dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its triggered by the following conditions\n> \n> if (OVS_UNLIKELY(!may_steal ||\n> batch->packets[0]->source != DPBUF_DPDK)\n> \n> What will happen in above if the source of the packet is not DPBUF_DPDK?\n> \n> For the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()' is called later on.\n> \n> This has specific DPDK calls that expect an mbuf source for the packet.\n> \n> Previously QoS took place after the packet had been copied so this was not an issue but now we have DPDK rte functions being called on a non mbuf source packet. I'm not sure of the behavior that could occur in this case. Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt) in 'netdev_dpdk_policer_pkt_handle()'?\n> \n> It could be the case then that non DPDK specific call maybe required to attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()' before calling the meter itself.\n> \n> Have you tested this use case?\n> \n> Ian\n> \n> > + if (txcnt_eth == 0) {\n> > + dropped = batch->count;\n> > + goto out;\n> > + }\n> > + }\n> > +\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\n> > dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\n> > *batch)\n> > rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> >\n> > newcnt++;\n> > + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> > + dropped += batch->count - i - 1;\n> > + break;\n> > + }\n> > }\n> >\n> > if (dev->type == DPDK_DEV_VHOST) {\n> > __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n> > newcnt);\n> > } else {\n> > - unsigned int qos_pkts = newcnt;\n> > -\n> > - /* Check if QoS has been configured for this netdev. */\n> > - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> > -\n> > - dropped += qos_pkts - newcnt;\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> > }\n> >\n> > +out:\n> > if (OVS_UNLIKELY(dropped)) {\n> > rte_spinlock_lock(&dev->stats_lock);\n> > dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\n> > netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n> > dp_packet_batch_apply_cutlen(batch);\n> >\n> > cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> > - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> > + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> > dropped = batch->count - cnt;\n> >\n> > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\n> > 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\n> > *conf, }\n> >\n> > static int\n> > -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt)\n> > +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > pkt_cnt,\n> > + bool may_steal)\n> > {\n> > int cnt = 0;\n> > struct egress_policer *policer =\n> > CONTAINER_OF(conf, struct egress_policer, qos_conf);\n> >\n> > - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\n> > + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> > + pkt_cnt, may_steal);\n> >\n> > return cnt;\n> > }\n> > --\n> > 1.8.3.1\n> \n> _______________________________________________\n> dev mailing list\n> dev@openvswitch.org\n> https://mail.openvswitch.org/mailman/listinfo/ovs-dev","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQns33t50z9t3J\n\tfor ;\n\tTue, 31 Oct 2017 08:37:59 +1100 (AEDT)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 8AF0CD31;\n\tMon, 30 Oct 2017 21:37:57 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 13332BE1\n\tfor ; Mon, 30 Oct 2017 21:37:57 +0000 (UTC)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 26A3A4CE\n\tfor ; Mon, 30 Oct 2017 21:37:56 +0000 (UTC)","from ovn.org (unknown [208.91.3.26])\n\t(Authenticated sender: blp@ovn.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 2A79AA80CB;\n\tMon, 30 Oct 2017 22:37:52 +0100 (CET)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"208.91.3.26","Date":"Mon, 30 Oct 2017 14:37:49 -0700","From":"Ben Pfaff ","To":"\"Stokes, Ian\" ","Message-ID":"<20171030213749.GC27530@ovn.org>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1796286,"web_url":"http://patchwork.ozlabs.org/comment/1796286/","msgid":"<00F17C60-19C6-4A04-B111-BD19CE96FAD0@vmware.com>","list_archive_url":null,"date":"2017-10-30T21:53:01","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":68212,"url":"http://patchwork.ozlabs.org/api/people/68212/","name":"Darrell Ball","email":"dball@vmware.com"},"content":"This patch was merged to dpdk_merge on Sept 6 and later merged to ovs.\r\n\r\nhttps://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338413.html\r\n\r\n\r\nOn 10/30/17, 2:38 PM, \"ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff\" wrote:\r\n\r\n Hi Ian and Gao, it looks like this patch never got applied. Gao, is it\r\n still relevant and, if so, Ian, will you add it to your next batch of\r\n patches?\r\n \r\n Thanks,\r\n \r\n Ben.\r\n \r\n On Sun, Sep 03, 2017 at 10:46:24AM +0000, Stokes, Ian wrote:\r\n > OK,\r\n > \r\n > In my own testing I’m seeing the same behavior with a tap interface pinging to a bridge with a DPDK device. As such I think this should be ok.\r\n > \r\n > Acked-by: ian.stokes@intel.com\r\n > \r\n > Ian\r\n > \r\n > From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]\r\n > Sent: Thursday, August 31, 2017 2:56 AM\r\n > To: Stokes, Ian \r\n > Cc: dev@openvswitch.org\r\n > Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf\r\n > \r\n > Here is the the testing I just did:\r\n > 1. Add log in function netdev_dpdk_policer_pkt_handle\r\n > static inline bool\r\n > netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\r\n > struct rte_mbuf *pkt, uint64_t time)\r\n > {\r\n > uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr);\r\n > VLOG_WARN(\"GGGG, payload_len:%u, rte_pkt_len:%u\",\r\n > pkt_len, rte_pktmbuf_pkt_len(pkt)); <------------------------print out pkt_len\r\n > \r\n > return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==\r\n > e_RTE_METER_GREEN;\r\n > }\r\n > 2. rebuild a rpm, install in machine, enable DPDK.\r\n > 3. Create a container on the bridge(userspace bridge). (The container use veth device)\r\n > 4. Config qos on eth dpdk eth by using : ovs-vsctl set port dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048\r\n > 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can see:\r\n > \r\n > 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n > 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n > 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n > 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\r\n > So the mbuf->pkt_len has a correct length.\r\n > On the code side, we have function dp_packet_set_size(), if ovs compiled with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c\r\n > \r\n > #ifdef DPDK_NETDEV\r\n > ......\r\n > \r\n > static inline void\r\n > dp_packet_set_size(struct dp_packet *b, uint32_t v)\r\n > {\r\n > .....\r\n > b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\r\n > b->mbuf.pkt_len = v; /* Total length of all segments linked to\r\n > * this segment. */\r\n > }\r\n > Please let me know if you have any concern on it.\r\n > \r\n > Thanks\r\n > Zhenyu Gao\r\n > \r\n > 2017-08-30 23:18 GMT+08:00 Stokes, Ian >:\r\n > > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\r\n > > QoS checking may drop some of them.\r\n > > Move the QoS checking in front of copying data to mbuf, it helps to reduce\r\n > > useless copy.\r\n > \r\n > Hi Zhenyu, thanks for the patch, I agree with the objective of the patch but have some comments below I'd like to see addressed/tested before signing off.\r\n > \r\n > >\r\n > > Signed-off-by: Zhenyu Gao >\r\n > > ---\r\n > > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++--------------\r\n > > -----\r\n > > 1 file changed, 36 insertions(+), 19 deletions(-)\r\n > >\r\n > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\r\n > > 100644\r\n > > --- a/lib/netdev-dpdk.c\r\n > > +++ b/lib/netdev-dpdk.c\r\n > > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\r\n > > * For all QoS implementations it should always be non-null.\r\n > > */\r\n > > int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\r\n > > - int pkt_cnt);\r\n > > + int pkt_cnt, bool may_steal);\r\n > > };\r\n > >\r\n > > /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\r\n > > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\r\n > > *meter,\r\n > >\r\n > > static int\r\n > > netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\r\n > > - struct rte_mbuf **pkts, int pkt_cnt)\r\n > > + struct rte_mbuf **pkts, int pkt_cnt,\r\n > > + bool may_steal)\r\n > > {\r\n > > int i = 0;\r\n > > int cnt = 0;\r\n > > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\r\n > > *meter,\r\n > > }\r\n > > cnt++;\r\n > > } else {\r\n > > - rte_pktmbuf_free(pkt);\r\n > > + if (may_steal) {\r\n > > + rte_pktmbuf_free(pkt);\r\n > > + }\r\n > > }\r\n > > }\r\n > >\r\n > > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\r\n > > *meter,\r\n > >\r\n > > static int\r\n > > ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\r\n > > **pkts,\r\n > > - int pkt_cnt)\r\n > > + int pkt_cnt, bool may_steal)\r\n > > {\r\n > > int cnt = 0;\r\n > >\r\n > > rte_spinlock_lock(&policer->policer_lock);\r\n > > - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\r\n > > + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\r\n > > + pkt_cnt, may_steal);\r\n > > rte_spinlock_unlock(&policer->policer_lock);\r\n > >\r\n > > return cnt;\r\n > > @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\r\n > > dropped = nb_rx;\r\n > > nb_rx = ingress_policer_run(policer,\r\n > > (struct rte_mbuf **) batch->packets,\r\n > > - nb_rx);\r\n > > + nb_rx, true);\r\n > > dropped -= nb_rx;\r\n > > }\r\n > >\r\n > > @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\r\n > > dp_packet_batch *batch)\r\n > > dropped = nb_rx;\r\n > > nb_rx = ingress_policer_run(policer,\r\n > > (struct rte_mbuf **) batch->packets,\r\n > > - nb_rx);\r\n > > + nb_rx, true);\r\n > > dropped -= nb_rx;\r\n > > }\r\n > >\r\n > > @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\r\n > > struct dp_packet_batch *batch)\r\n > >\r\n > > static inline int\r\n > > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\r\n > > - int cnt)\r\n > > + int cnt, bool may_steal)\r\n > > {\r\n > > struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\r\n > > >qos_conf);\r\n > >\r\n > > if (qos_conf) {\r\n > > rte_spinlock_lock(&qos_conf->lock);\r\n > > - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\r\n > > + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\r\n > > rte_spinlock_unlock(&qos_conf->lock);\r\n > > }\r\n > >\r\n > > @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int\r\n > > qid,\r\n > >\r\n > > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\r\n > > /* Check has QoS has been configured for the netdev */\r\n > > - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\r\n > > + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\r\n > > dropped = total_pkts - cnt;\r\n > >\r\n > > do {\r\n > > @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\r\n > > struct dp_packet_batch *batch) #endif\r\n > > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\r\n > > struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\r\n > > + int txcnt_eth = batch->count;\r\n > > int dropped = 0;\r\n > > int newcnt = 0;\r\n > > int i;\r\n > >\r\n > > + if (dev->type != DPDK_DEV_VHOST) {\r\n > > + /* Check if QoS has been configured for this netdev. */\r\n > > + txcnt_eth = netdev_dpdk_qos_run(dev,\r\n > > + (struct rte_mbuf **)batch-\r\n > > >packets,\r\n > > + txcnt_eth, false);\r\n > So dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its triggered by the following conditions\r\n > \r\n > if (OVS_UNLIKELY(!may_steal ||\r\n > batch->packets[0]->source != DPBUF_DPDK)\r\n > \r\n > What will happen in above if the source of the packet is not DPBUF_DPDK?\r\n > \r\n > For the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()' is called later on.\r\n > \r\n > This has specific DPDK calls that expect an mbuf source for the packet.\r\n > \r\n > Previously QoS took place after the packet had been copied so this was not an issue but now we have DPDK rte functions being called on a non mbuf source packet. I'm not sure of the behavior that could occur in this case. Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt) in 'netdev_dpdk_policer_pkt_handle()'?\r\n > \r\n > It could be the case then that non DPDK specific call maybe required to attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()' before calling the meter itself.\r\n > \r\n > Have you tested this use case?\r\n > \r\n > Ian\r\n > \r\n > > + if (txcnt_eth == 0) {\r\n > > + dropped = batch->count;\r\n > > + goto out;\r\n > > + }\r\n > > + }\r\n > > +\r\n > > dp_packet_batch_apply_cutlen(batch);\r\n > >\r\n > > for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\r\n > > dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\r\n > > *batch)\r\n > > rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\r\n > >\r\n > > newcnt++;\r\n > > + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\r\n > > + dropped += batch->count - i - 1;\r\n > > + break;\r\n > > + }\r\n > > }\r\n > >\r\n > > if (dev->type == DPDK_DEV_VHOST) {\r\n > > __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\r\n > > newcnt);\r\n > > } else {\r\n > > - unsigned int qos_pkts = newcnt;\r\n > > -\r\n > > - /* Check if QoS has been configured for this netdev. */\r\n > > - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\r\n > > -\r\n > > - dropped += qos_pkts - newcnt;\r\n > > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\r\n > > }\r\n > >\r\n > > +out:\r\n > > if (OVS_UNLIKELY(dropped)) {\r\n > > rte_spinlock_lock(&dev->stats_lock);\r\n > > dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\r\n > > netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\r\n > > dp_packet_batch_apply_cutlen(batch);\r\n > >\r\n > > cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\r\n > > - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\r\n > > + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\r\n > > dropped = batch->count - cnt;\r\n > >\r\n > > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\r\n > > 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\r\n > > *conf, }\r\n > >\r\n > > static int\r\n > > -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\r\n > > pkt_cnt)\r\n > > +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\r\n > > pkt_cnt,\r\n > > + bool may_steal)\r\n > > {\r\n > > int cnt = 0;\r\n > > struct egress_policer *policer =\r\n > > CONTAINER_OF(conf, struct egress_policer, qos_conf);\r\n > >\r\n > > - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\r\n > > + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\r\n > > + pkt_cnt, may_steal);\r\n > >\r\n > > return cnt;\r\n > > }\r\n > > --\r\n > > 1.8.3.1\r\n > \r\n > _______________________________________________\r\n > dev mailing list\r\n > dev@openvswitch.org\r\n > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=dcQaJCTTWP1qu7IpAUJgDFmLk-ybQ66X_Jc0QNWWRSY&s=UjOv2s3fuqSLjg89oMkiwGeE3Nu-7FX0hG-aR3aJ-3s&e=\r\n _______________________________________________\r\n dev mailing list\r\n dev@openvswitch.org\r\n https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=dcQaJCTTWP1qu7IpAUJgDFmLk-ybQ66X_Jc0QNWWRSY&s=UjOv2s3fuqSLjg89oMkiwGeE3Nu-7FX0hG-aR3aJ-3s&e=","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"cQrTVft4\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQpBZ48zFz9t3x\n\tfor ;\n\tTue, 31 Oct 2017 08:53:10 +1100 (AEDT)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 97148C33;\n\tMon, 30 Oct 2017 21:53:06 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 0A90BAF5\n\tfor ; Mon, 30 Oct 2017 21:53:05 +0000 (UTC)","from NAM03-BY2-obe.outbound.protection.outlook.com\n\t(mail-by2nam03on0053.outbound.protection.outlook.com [104.47.42.53])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 977B5483\n\tfor ; Mon, 30 Oct 2017 21:53:03 +0000 (UTC)","from MWHPR05MB3406.namprd05.prod.outlook.com (10.174.175.155) by\n\tMWHPR05MB3408.namprd05.prod.outlook.com (10.174.175.157) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id\n\t15.20.197.4; Mon, 30 Oct 2017 21:53:01 +0000","from MWHPR05MB3406.namprd05.prod.outlook.com ([10.174.175.155]) by\n\tMWHPR05MB3406.namprd05.prod.outlook.com ([10.174.175.155]) with\n\tmapi id 15.20.0197.011; Mon, 30 Oct 2017 21:53:01 +0000"],"X-Greylist":"whitelisted by SQLgrey-1.7.6","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=onevmw.onmicrosoft.com; s=selector1-vmware-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=2pr2fpX6VC6LS77W6ZNaatCtzjyQD06UEjIZNcTG1pM=;\n\tb=cQrTVft4FE35KgY2aCyzrIPMMaWbhh2C+WYrai8ig9menr+WAxOqpIJ6GBhxxn8s1vxZ58FoCqyaWg8ide+914LK/iWEtQXbDlZUYhY1SiaU3jmfqhGh8Xhn6wpWsb+ZCmAyasLq0cCfZoVf8eRQ02pkXwlxtKqP+bRLS86FYYA=","From":"Darrell Ball ","To":"Ben Pfaff , \"Stokes, Ian\" ","Thread-Topic":"[ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","Thread-Index":"AQHTJKH767rWG4m2PEOQuzOMpyUs1aL9RWaAgAAEPgA=","Date":"Mon, 30 Oct 2017 21:53:01 +0000","Message-ID":"<00F17C60-19C6-4A04-B111-BD19CE96FAD0@vmware.com>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t\n\t\n\t\n\t<20171030213749.GC27530@ovn.org>","In-Reply-To":"<20171030213749.GC27530@ovn.org>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","user-agent":"Microsoft-MacOutlook/f.26.0.170902","x-originating-ip":"[208.91.2.1]","x-ms-publictraffictype":"Email","x-microsoft-exchange-diagnostics":"1; MWHPR05MB3408;\n\t20:pUJrnnAbmEnbkqBUgtiKxbbo3i0JD5JgB2MyZdvxSMurjC2Xh8Lw42YgDKuqVlGOccnJRT5DKMzXB+KxB77GV6KV6aMAmUNjxgFrHHPeIC2bIcSy9UnloEDwXU4xQeKHG+6K+iZ/Kw8N3/+rdDx/r2wrHiLqvrDsLW7yzXaclJo=","x-ms-exchange-antispam-srfa-diagnostics":"SSOS;","x-ms-office365-filtering-correlation-id":"403c2f80-77f3-4b35-8126-08d51fe0973b","x-microsoft-antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(22001)(4534020)(4602075)(2017052603238);\n\tSRVR:MWHPR05MB3408; ","x-ms-traffictypediagnostic":"MWHPR05MB3408:","authentication-results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=onevmw.onmicrosoft.com\n\theader.i=@onevmw.onmicrosoft.com header.b=\"cQrTVft4\"; \n\tdkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=dball@vmware.com; "],"x-exchange-antispam-report-test":"UriScan:(10436049006162)(216315784871565)(228905959029699); ","x-microsoft-antispam-prvs":"","x-exchange-antispam-report-cfa-test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(3002001)(93006095)(93001095)(10201501046)(3231020)(100000703101)(100105400095)(6041248)(20161123555025)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123558100)(20161123562025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:MWHPR05MB3408; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:MWHPR05MB3408; ","x-forefront-prvs":"0476D4AB88","x-forefront-antispam-report":"SFV:NSPM;\n\tSFS:(10009020)(979002)(6009001)(346002)(376002)(51914003)(377424004)(189002)(199003)(24454002)(93886005)(33656002)(4326008)(106356001)(2900100001)(8676002)(101416001)(305945005)(81166006)(81156014)(76176999)(7736002)(50986999)(8936002)(478600001)(25786009)(966005)(105586002)(14454004)(97736004)(66066001)(54356999)(3660700001)(86362001)(77096006)(5660300001)(6506006)(189998001)(316002)(229853002)(575784001)(6486002)(99286003)(6512007)(6306002)(53546010)(2950100002)(36756003)(53936002)(68736007)(3846002)(6436002)(6246003)(83506002)(110136005)(6116002)(102836003)(3280700002)(82746002)(58126008)(2906002)(83716003)(969003)(989001)(999001)(1009001)(1019001);\n\tDIR:OUT; SFP:1101; SCL:1; SRVR:MWHPR05MB3408;\n\tH:MWHPR05MB3406.namprd05.prod.outlook.com; FPR:; SPF:None;\n\tPTR:InfoNoRecords; MX:1; A:1; LANG:en; ","received-spf":"None (protection.outlook.com: vmware.com does not designate\n\tpermitted sender hosts)","spamdiagnosticoutput":"1:99","spamdiagnosticmetadata":"NSPM","Content-ID":"<14B8FA2EFEBC7E47ADE65B3286E1B4C5@namprd05.prod.outlook.com>","MIME-Version":"1.0","X-OriginatorOrg":"vmware.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"403c2f80-77f3-4b35-8126-08d51fe0973b","X-MS-Exchange-CrossTenant-originalarrivaltime":"30 Oct 2017 21:53:01.0687\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"MWHPR05MB3408","X-Spam-Status":"No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,\n\tRCVD_IN_DNSWL_NONE autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1796292,"web_url":"http://patchwork.ozlabs.org/comment/1796292/","msgid":"<20171030215552.GE27530@ovn.org>","list_archive_url":null,"date":"2017-10-30T21:55:52","subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"OK, great, thanks.\n\nOn Mon, Oct 30, 2017 at 09:53:01PM +0000, Darrell Ball wrote:\n> This patch was merged to dpdk_merge on Sept 6 and later merged to ovs.\n> \n> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338413.html\n> \n> \n> On 10/30/17, 2:38 PM, \"ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff\" wrote:\n> \n> Hi Ian and Gao, it looks like this patch never got applied. Gao, is it\n> still relevant and, if so, Ian, will you add it to your next batch of\n> patches?\n> \n> Thanks,\n> \n> Ben.\n> \n> On Sun, Sep 03, 2017 at 10:46:24AM +0000, Stokes, Ian wrote:\n> > OK,\n> > \n> > In my own testing I’m seeing the same behavior with a tap interface pinging to a bridge with a DPDK device. As such I think this should be ok.\n> > \n> > Acked-by: ian.stokes@intel.com\n> > \n> > Ian\n> > \n> > From: Gao Zhenyu [mailto:sysugaozhenyu@gmail.com]\n> > Sent: Thursday, August 31, 2017 2:56 AM\n> > To: Stokes, Ian \n> > Cc: dev@openvswitch.org\n> > Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf\n> > \n> > Here is the the testing I just did:\n> > 1. Add log in function netdev_dpdk_policer_pkt_handle\n> > static inline bool\n> > netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,\n> > struct rte_mbuf *pkt, uint64_t time)\n> > {\n> > uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr);\n> > VLOG_WARN(\"GGGG, payload_len:%u, rte_pkt_len:%u\",\n> > pkt_len, rte_pktmbuf_pkt_len(pkt)); <------------------------print out pkt_len\n> > \n> > return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==\n> > e_RTE_METER_GREEN;\n> > }\n> > 2. rebuild a rpm, install in machine, enable DPDK.\n> > 3. Create a container on the bridge(userspace bridge). (The container use veth device)\n> > 4. Config qos on eth dpdk eth by using : ovs-vsctl set port dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos type=egress-policer other-config:cir=1250000 other-config:cbs=2048\n> > 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can see:\n> > \n> > 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> > 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> > 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> > 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|GGGG, payload_len:88, rte_pkt_len:102\n> > So the mbuf->pkt_len has a correct length.\n> > On the code side, we have function dp_packet_set_size(), if ovs compiled with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c\n> > \n> > #ifdef DPDK_NETDEV\n> > ......\n> > \n> > static inline void\n> > dp_packet_set_size(struct dp_packet *b, uint32_t v)\n> > {\n> > .....\n> > b->mbuf.data_len = (uint16_t)v; /* Current seg length. */\n> > b->mbuf.pkt_len = v; /* Total length of all segments linked to\n> > * this segment. */\n> > }\n> > Please let me know if you have any concern on it.\n> > \n> > Thanks\n> > Zhenyu Gao\n> > \n> > 2017-08-30 23:18 GMT+08:00 Stokes, Ian >:\n> > > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but\n> > > QoS checking may drop some of them.\n> > > Move the QoS checking in front of copying data to mbuf, it helps to reduce\n> > > useless copy.\n> > \n> > Hi Zhenyu, thanks for the patch, I agree with the objective of the patch but have some comments below I'd like to see addressed/tested before signing off.\n> > \n> > >\n> > > Signed-off-by: Zhenyu Gao >\n> > > ---\n> > > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++++++++--------------\n> > > -----\n> > > 1 file changed, 36 insertions(+), 19 deletions(-)\n> > >\n> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8\n> > > 100644\n> > > --- a/lib/netdev-dpdk.c\n> > > +++ b/lib/netdev-dpdk.c\n> > > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {\n> > > * For all QoS implementations it should always be non-null.\n> > > */\n> > > int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,\n> > > - int pkt_cnt);\n> > > + int pkt_cnt, bool may_steal);\n> > > };\n> > >\n> > > /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -\n> > > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm\n> > > *meter,\n> > >\n> > > static int\n> > > netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,\n> > > - struct rte_mbuf **pkts, int pkt_cnt)\n> > > + struct rte_mbuf **pkts, int pkt_cnt,\n> > > + bool may_steal)\n> > > {\n> > > int i = 0;\n> > > int cnt = 0;\n> > > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > > *meter,\n> > > }\n> > > cnt++;\n> > > } else {\n> > > - rte_pktmbuf_free(pkt);\n> > > + if (may_steal) {\n> > > + rte_pktmbuf_free(pkt);\n> > > + }\n> > > }\n> > > }\n> > >\n> > > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm\n> > > *meter,\n> > >\n> > > static int\n> > > ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf\n> > > **pkts,\n> > > - int pkt_cnt)\n> > > + int pkt_cnt, bool may_steal)\n> > > {\n> > > int cnt = 0;\n> > >\n> > > rte_spinlock_lock(&policer->policer_lock);\n> > > - cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts, pkt_cnt);\n> > > + cnt = netdev_dpdk_policer_run(&policer->in_policer, pkts,\n> > > + pkt_cnt, may_steal);\n> > > rte_spinlock_unlock(&policer->policer_lock);\n> > >\n> > > return cnt;\n> > > @@ -1635,7 +1639,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,\n> > > dropped = nb_rx;\n> > > nb_rx = ingress_policer_run(policer,\n> > > (struct rte_mbuf **) batch->packets,\n> > > - nb_rx);\n> > > + nb_rx, true);\n> > > dropped -= nb_rx;\n> > > }\n> > >\n> > > @@ -1672,7 +1676,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct\n> > > dp_packet_batch *batch)\n> > > dropped = nb_rx;\n> > > nb_rx = ingress_policer_run(policer,\n> > > (struct rte_mbuf **) batch->packets,\n> > > - nb_rx);\n> > > + nb_rx, true);\n> > > dropped -= nb_rx;\n> > > }\n> > >\n> > > @@ -1690,13 +1694,13 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,\n> > > struct dp_packet_batch *batch)\n> > >\n> > > static inline int\n> > > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts,\n> > > - int cnt)\n> > > + int cnt, bool may_steal)\n> > > {\n> > > struct qos_conf *qos_conf = ovsrcu_get(struct qos_conf *, &dev-\n> > > >qos_conf);\n> > >\n> > > if (qos_conf) {\n> > > rte_spinlock_lock(&qos_conf->lock);\n> > > - cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt);\n> > > + cnt = qos_conf->ops->qos_run(qos_conf, pkts, cnt, may_steal);\n> > > rte_spinlock_unlock(&qos_conf->lock);\n> > > }\n> > >\n> > > @@ -1770,7 +1774,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int\n> > > qid,\n> > >\n> > > cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);\n> > > /* Check has QoS has been configured for the netdev */\n> > > - cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt);\n> > > + cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);\n> > > dropped = total_pkts - cnt;\n> > >\n> > > do {\n> > > @@ -1816,10 +1820,22 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid,\n> > > struct dp_packet_batch *batch) #endif\n> > > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);\n> > > struct rte_mbuf *pkts[PKT_ARRAY_SIZE];\n> > > + int txcnt_eth = batch->count;\n> > > int dropped = 0;\n> > > int newcnt = 0;\n> > > int i;\n> > >\n> > > + if (dev->type != DPDK_DEV_VHOST) {\n> > > + /* Check if QoS has been configured for this netdev. */\n> > > + txcnt_eth = netdev_dpdk_qos_run(dev,\n> > > + (struct rte_mbuf **)batch-\n> > > >packets,\n> > > + txcnt_eth, false);\n> > So dpdk_do_tx_copy will be called as part of netdev_dpdk_eth_send__ if its triggered by the following conditions\n> > \n> > if (OVS_UNLIKELY(!may_steal ||\n> > batch->packets[0]->source != DPBUF_DPDK)\n> > \n> > What will happen in above if the source of the packet is not DPBUF_DPDK?\n> > \n> > For the most part it will be ok until 'netdev_dpdk_policer_pkt_handle()' is called later on.\n> > \n> > This has specific DPDK calls that expect an mbuf source for the packet.\n> > \n> > Previously QoS took place after the packet had been copied so this was not an issue but now we have DPDK rte functions being called on a non mbuf source packet. I'm not sure of the behavior that could occur in this case. Could an incorrect length be returned for the call rte_pktmbuf_pkt_len(pkt) in 'netdev_dpdk_policer_pkt_handle()'?\n> > \n> > It could be the case then that non DPDK specific call maybe required to attain the length of the packet in 'netdev_dpdk_policer_pkt_handle()' before calling the meter itself.\n> > \n> > Have you tested this use case?\n> > \n> > Ian\n> > \n> > > + if (txcnt_eth == 0) {\n> > > + dropped = batch->count;\n> > > + goto out;\n> > > + }\n> > > + }\n> > > +\n> > > dp_packet_batch_apply_cutlen(batch);\n> > >\n> > > for (i = 0; i < batch->count; i++) { @@ -1848,21 +1864,20 @@\n> > > dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch\n> > > *batch)\n> > > rte_pktmbuf_pkt_len(pkts[newcnt]) = size;\n> > >\n> > > newcnt++;\n> > > + if (dev->type != DPDK_DEV_VHOST && newcnt >= txcnt_eth) {\n> > > + dropped += batch->count - i - 1;\n> > > + break;\n> > > + }\n> > > }\n> > >\n> > > if (dev->type == DPDK_DEV_VHOST) {\n> > > __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) pkts,\n> > > newcnt);\n> > > } else {\n> > > - unsigned int qos_pkts = newcnt;\n> > > -\n> > > - /* Check if QoS has been configured for this netdev. */\n> > > - newcnt = netdev_dpdk_qos_run(dev, pkts, newcnt);\n> > > -\n> > > - dropped += qos_pkts - newcnt;\n> > > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, newcnt);\n> > > }\n> > >\n> > > +out:\n> > > if (OVS_UNLIKELY(dropped)) {\n> > > rte_spinlock_lock(&dev->stats_lock);\n> > > dev->stats.tx_dropped += dropped; @@ -1915,7 +1930,7 @@\n> > > netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,\n> > > dp_packet_batch_apply_cutlen(batch);\n> > >\n> > > cnt = netdev_dpdk_filter_packet_len(dev, pkts, cnt);\n> > > - cnt = netdev_dpdk_qos_run(dev, pkts, cnt);\n> > > + cnt = netdev_dpdk_qos_run(dev, pkts, cnt, true);\n> > > dropped = batch->count - cnt;\n> > >\n> > > dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, cnt); @@ -\n> > > 3132,13 +3147,15 @@ egress_policer_qos_is_equal(const struct qos_conf\n> > > *conf, }\n> > >\n> > > static int\n> > > -egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > > pkt_cnt)\n> > > +egress_policer_run(struct qos_conf *conf, struct rte_mbuf **pkts, int\n> > > pkt_cnt,\n> > > + bool may_steal)\n> > > {\n> > > int cnt = 0;\n> > > struct egress_policer *policer =\n> > > CONTAINER_OF(conf, struct egress_policer, qos_conf);\n> > >\n> > > - cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts, pkt_cnt);\n> > > + cnt = netdev_dpdk_policer_run(&policer->egress_meter, pkts,\n> > > + pkt_cnt, may_steal);\n> > >\n> > > return cnt;\n> > > }\n> > > --\n> > > 1.8.3.1\n> > \n> > _______________________________________________\n> > dev mailing list\n> > dev@openvswitch.org\n> > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=dcQaJCTTWP1qu7IpAUJgDFmLk-ybQ66X_Jc0QNWWRSY&s=UjOv2s3fuqSLjg89oMkiwGeE3Nu-7FX0hG-aR3aJ-3s&e=\n> _______________________________________________\n> dev mailing list\n> dev@openvswitch.org\n> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=dcQaJCTTWP1qu7IpAUJgDFmLk-ybQ66X_Jc0QNWWRSY&s=UjOv2s3fuqSLjg89oMkiwGeE3Nu-7FX0hG-aR3aJ-3s&e=\n> \n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQpFt36snz9sRg\n\tfor ;\n\tTue, 31 Oct 2017 08:56:02 +1100 (AEDT)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id E559BC19;\n\tMon, 30 Oct 2017 21:56:00 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id C6240C12\n\tfor ; Mon, 30 Oct 2017 21:55:59 +0000 (UTC)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id B11C3483\n\tfor ; Mon, 30 Oct 2017 21:55:58 +0000 (UTC)","from ovn.org (unknown [208.91.3.26])\n\t(Authenticated sender: blp@ovn.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D161BC5A53;\n\tMon, 30 Oct 2017 22:55:55 +0100 (CET)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"208.91.3.26","Date":"Mon, 30 Oct 2017 14:55:52 -0700","From":"Ben Pfaff ","To":"Darrell Ball ","Message-ID":"<20171030215552.GE27530@ovn.org>","References":"<20170829113944.4073-1-sysugaozhenyu@gmail.com>\n\t\n\t\n\t\n\t<20171030213749.GC27530@ovn.org>\n\t<00F17C60-19C6-4A04-B111-BD19CE96FAD0@vmware.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<00F17C60-19C6-4A04-B111-BD19CE96FAD0@vmware.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.2 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"dev@openvswitch.org\" ","Subject":"Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before\n\tcopying to mbuf","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}}]