From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9229CA0526; Fri, 10 Jul 2020 01:58:35 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 210EF1DDCF; Fri, 10 Jul 2020 01:58:35 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 2C87C1DDBF for ; Fri, 10 Jul 2020 01:58:33 +0200 (CEST) IronPort-SDR: xQwOj9L5Y8O3lnKfz4T8Yl5ITxTCPObf7RC/jB0jGt2aklG2A9iXhH66jlLyRQeLB97xboFQID mc/5SEDV/yiQ== X-IronPort-AV: E=McAfee;i="6000,8403,9677"; a="148115413" X-IronPort-AV: E=Sophos;i="5.75,332,1589266800"; d="scan'208";a="148115413" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Jul 2020 16:58:32 -0700 IronPort-SDR: SdwGRBa4tJ7hda6W9Mrd0/lCa4tL3z5yHpcB+uMBRGB1TcM9gJCcWwbZ2+I7lwJfDcE+eDs7Ei ytWbNi90hQIQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,332,1589266800"; d="scan'208";a="428393573" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.213.206.164]) ([10.213.206.164]) by orsmga004.jf.intel.com with ESMTP; 09 Jul 2020 16:58:27 -0700 To: Viacheslav Ovsiienko , dev@dpdk.org Cc: matan@mellanox.com, rasland@mellanox.com, olivier.matz@6wind.com, bernard.iremonger@intel.com, thomas@monjalon.com References: <1591771085-24959-1-git-send-email-viacheslavo@mellanox.com> <1594298216-3746-1-git-send-email-viacheslavo@mellanox.com> <1594298216-3746-2-git-send-email-viacheslavo@mellanox.com> From: Ferruh Yigit Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJsBBMBCgBWAhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEABQkKqZZ8FiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl6ha3sXGHZrczovL2tl eXMub3BlbnBncC5vcmcACgkQ+TPrQ98TYR8uLA//QwltuFliUWe60xwmu9sY38c1DXvX67wk UryQ1WijVdIoj4H8cf/s2KtyIBjc89R254KMEfJDao/LrXqJ69KyGKXFhFPlF3VmFLsN4XiT PSfxkx8s6kHVaB3O183p4xAqnnl/ql8nJ5ph9HuwdL8CyO5/7dC/MjZ/mc4NGq5O9zk3YRGO lvdZAp5HW9VKW4iynvy7rl3tKyEqaAE62MbGyfJDH3C/nV/4+mPc8Av5rRH2hV+DBQourwuC ci6noiDP6GCNQqTh1FHYvXaN4GPMHD9DX6LtT8Fc5mL/V9i9kEVikPohlI0WJqhE+vQHFzR2 1q5nznE+pweYsBi3LXIMYpmha9oJh03dJOdKAEhkfBr6n8BWkWQMMiwfdzg20JX0o7a/iF8H 4dshBs+dXdIKzPfJhMjHxLDFNPNH8zRQkB02JceY9ESEah3wAbzTwz+e/9qQ5OyDTQjKkVOo cxC2U7CqeNt0JZi0tmuzIWrfxjAUulVhBmnceqyMOzGpSCQIkvalb6+eXsC9V1DZ4zsHZ2Mx Hi+7pCksdraXUhKdg5bOVCt8XFmx1MX4AoV3GWy6mZ4eMMvJN2hjXcrreQgG25BdCdcxKgqp e9cMbCtF+RZax8U6LkAWueJJ1QXrav1Jk5SnG8/5xANQoBQKGz+yFiWcgEs9Tpxth15o2v59 gXK5Ag0EV9ZMvgEQAKc0Db17xNqtSwEvmfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ES YpV8QWj0xK4YM0dLxnDU2IYxjEshSB1TqAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4Ai bPtrHuIXWQOBECcVZTTOdZYGAzaYzxiAONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxD UQljeNvKYt1lZE/gAUUxNLWsYyTT+22/vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/ 3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35piVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVj sM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQI3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdc q9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYHfVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH7 1PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZqw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFB VOQOxCvwRG2QCgcJ/UTn5vlivul+cThi6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI 8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJlRr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYC GwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNhHwUCXqFrngUJCKxSYAAKCRD5M+tD3xNhH3YWD/9b cUiWaHJasX+OpiuZ1Li5GG3m9aw4lR/k2lET0UPRer2Jy1JsL+uqzdkxGvPqzFTBXgx/6Byz EMa2mt6R9BCyR286s3lxVS5Bgr5JGB3EkpPcoJT3A7QOYMV95jBiiJTy78Qdzi5LrIu4tW6H o0MWUjpjdbR01cnj6EagKrDx9kAsqQTfvz4ff5JIFyKSKEHQMaz1YGHyCWhsTwqONhs0G7V2 0taQS1bGiaWND0dIBJ/u0pU998XZhmMzn765H+/MqXsyDXwoHv1rcaX/kcZIcN3sLUVcbdxA WHXOktGTQemQfEpCNuf2jeeJlp8sHmAQmV3dLS1R49h0q7hH4qOPEIvXjQebJGs5W7s2vxbA 5u5nLujmMkkfg1XHsds0u7Zdp2n200VC4GQf8vsUp6CSMgjedHeF9zKv1W4lYXpHp576ZV7T GgsEsvveAE1xvHnpV9d7ZehPuZfYlP4qgo2iutA1c0AXZLn5LPcDBgZ+KQZTzm05RU1gkx7n gL9CdTzVrYFy7Y5R+TrE9HFUnsaXaGsJwOB/emByGPQEKrupz8CZFi9pkqPuAPwjN6Wonokv ChAewHXPUadcJmCTj78Oeg9uXR6yjpxyFjx3vdijQIYgi5TEGpeTQBymLANOYxYWYOjXk+ae dYuOYKR9nbPv+2zK9pwwQ2NXbUBystaGyQ== Message-ID: <2ee95fc5-fa97-c518-fd61-400d667ace34@intel.com> Date: Fri, 10 Jul 2020 00:58:26 +0100 MIME-Version: 1.0 In-Reply-To: <1594298216-3746-2-git-send-email-viacheslavo@mellanox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v6 2/2] app/testpmd: add send scheduling test capability X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/9/2020 1:36 PM, Viacheslav Ovsiienko wrote: > This commit adds testpmd capability to provide timestamps on the packets > being sent in the txonly mode. This includes: > > - SEND_ON_TIMESTAMP support > new device Tx offload capability support added, example: > > testpmd> port config 0 tx_offload send_on_timestamp on > > - set txtimes, registers field and flag, example: > > testpmd> set txtimes 1000000,0 > > This command enables the packet send scheduling on timestamps if > the first parameter is not zero, generic format: > > testpmd> set txtimes (inter),(intra) > > where: > > inter - is the delay between the bursts in the device clock units. > intra - is the delay between the packets within the burst specified > in the device clock units > > As the result the bursts of packet will be transmitted with > specific delay between the packets within the burst and specific > delay between the bursts. The rte_eth_get_clock() is supposed to be > engaged to get the current device clock value and provide > the reference for the timestamps. > > - show txtimes, displays the timing settings > - txonly burst time pattern > > Signed-off-by: Viacheslav Ovsiienko <...> > +cmdline_parse_inst_t cmd_set_txtimes = { > + .f = cmd_set_txtimes_parsed, > + .data = NULL, > + .help_str = "set txtimes ,", > + .tokens = { > + (void *)&cmd_set_txtimes_keyword, > + (void *)&cmd_set_txtimes_name, > + (void *)&cmd_set_txtimes_value, > + NULL, > + }, > +}; Can you please update 'cmd_help_long_parsed()' with command updates? <...> > void > +show_tx_pkt_times(void) > +{ > + printf("Interburst gap: %u\n", tx_pkt_times[0]); > + printf("Intraburst gap: %u\n", tx_pkt_times[1]); > +} > + > +void > +set_tx_pkt_times(unsigned int *tx_times) > +{ > + int offset; > + int flag; > + > + static const struct rte_mbuf_dynfield desc_offs = { > + .name = RTE_MBUF_DYNFIELD_TIMESTAMP_NAME, > + .size = sizeof(uint64_t), > + .align = __alignof__(uint64_t), > + }; > + static const struct rte_mbuf_dynflag desc_flag = { > + .name = RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > + }; > + > + offset = rte_mbuf_dynfield_register(&desc_offs); > + if (offset < 0 && rte_errno != EEXIST) > + printf("Dynamic timestamp field registration error: %d", > + rte_errno); > + flag = rte_mbuf_dynflag_register(&desc_flag); > + if (flag < 0 && rte_errno != EEXIST) > + printf("Dynamic timestamp flag registration error: %d", > + rte_errno); You are not checking at all if the device supports 'DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP' offload or if it is configured or not, but blindly registering the dynamic fields. 'DEV_TX_OFFLOAD_SEND_ON_TIMESTAMP' seems not really used, as mentioned in prev patch I would be OK to drop the flag. > + tx_pkt_times[0] = tx_times[0]; > + tx_pkt_times[1] = tx_times[1]; I think it is better to rename 'tx_pkt_times[0]' -> 'tx_pkt_times_inter', 'tx_pkt_times[1]' --> 'tx_pkt_times_intra' to increase code readability. <...> > --- a/app/test-pmd/txonly.c > +++ b/app/test-pmd/txonly.c > @@ -53,6 +53,11 @@ > static struct rte_ipv4_hdr pkt_ip_hdr; /**< IP header of transmitted packets. */ > RTE_DEFINE_PER_LCORE(uint8_t, _ip_var); /**< IP address variation */ > static struct rte_udp_hdr pkt_udp_hdr; /**< UDP header of tx packets. */ > +RTE_DEFINE_PER_LCORE(uint64_t, ts_qskew); /**< Timestamp offset per queue */ > +static uint64_t ts_mask; /**< Timestamp dynamic flag mask */ > +static int32_t ts_off; /**< Timestamp dynamic field offset */ > +static bool ts_enable; /**< Timestamp enable */ > +static uint64_t ts_init[RTE_MAX_ETHPORTS]; What do you think renaming the 'ts_' prefix to long 'timestamp_' prefix, will variable names be too long? When you are out of this patch context and reading code 'ts_init' is not that expressive. <...> > @@ -213,6 +219,50 @@ > copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt, > sizeof(struct rte_ether_hdr) + > sizeof(struct rte_ipv4_hdr)); > + if (unlikely(ts_enable)) { > + uint64_t skew = RTE_PER_LCORE(ts_qskew); > + struct { > + rte_be32_t signature; > + rte_be16_t pkt_idx; > + rte_be16_t queue_idx; > + rte_be64_t ts; > + } ts_mark; > + > + if (unlikely(!skew)) { > + struct rte_eth_dev *dev = &rte_eth_devices[fs->tx_port]; > + unsigned int txqs_n = dev->data->nb_tx_queues; > + uint64_t phase = tx_pkt_times[0] * fs->tx_queue / > + (txqs_n ? txqs_n : 1); > + /* > + * Initialize the scheduling time phase shift > + * depending on queue index. > + */ > + skew = ts_init[fs->tx_port] + tx_pkt_times[0] + phase; > + RTE_PER_LCORE(ts_qskew) = skew; > + } > + ts_mark.pkt_idx = rte_cpu_to_be_16(idx); > + ts_mark.queue_idx = rte_cpu_to_be_16(fs->tx_queue); > + ts_mark.signature = rte_cpu_to_be_32(0xBEEFC0DE); > + if (unlikely(!idx)) { > + skew += tx_pkt_times[0]; > + pkt->ol_flags |= ts_mask; > + *RTE_MBUF_DYNFIELD(pkt, ts_off, uint64_t *) = skew; > + RTE_PER_LCORE(ts_qskew) = skew; > + ts_mark.ts = rte_cpu_to_be_64(skew); > + } else if (tx_pkt_times[1]) { > + skew += tx_pkt_times[1]; > + pkt->ol_flags |= ts_mask; > + *RTE_MBUF_DYNFIELD(pkt, ts_off, uint64_t *) = skew; > + RTE_PER_LCORE(ts_qskew) = skew; > + ts_mark.ts = rte_cpu_to_be_64(skew); > + } else { > + ts_mark.ts = RTE_BE64(0); > + } > + copy_buf_to_pkt(&ts_mark, sizeof(ts_mark), pkt, > + sizeof(struct rte_ether_hdr) + > + sizeof(struct rte_ipv4_hdr) + > + sizeof(pkt_udp_hdr)); timestamp data seems put into packet payload, I assume this is for debug, is there any intendent target app for the packets? <...> > static void > -tx_only_begin(__rte_unused portid_t pi) > +tx_only_begin(portid_t pi) > { > uint16_t pkt_data_len; > + int dynf; > > pkt_data_len = (uint16_t) (tx_pkt_length - ( > sizeof(struct rte_ether_hdr) + > sizeof(struct rte_ipv4_hdr) + > sizeof(struct rte_udp_hdr))); > setup_pkt_udp_ip_headers(&pkt_ip_hdr, &pkt_udp_hdr, pkt_data_len); > + > + ts_enable = false; > + ts_mask = 0; > + ts_off = -1; > + RTE_PER_LCORE(ts_qskew) = 0; > + dynf = rte_mbuf_dynflag_lookup > + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > + if (dynf >= 0) > + ts_mask = 1ULL << dynf; > + dynf = rte_mbuf_dynfield_lookup > + (RTE_MBUF_DYNFIELD_TIMESTAMP_NAME, NULL); > + if (dynf >= 0) > + ts_off = dynf; > + ts_enable = tx_pkt_times[0] && ts_mask && ts_off >= 0 && > + !rte_eth_read_clock(pi, &ts_init[pi]); 'rte_eth_read_clock()' support is a 'must' to have this feature. Can you please clarify this in the document/commit_log? <...> > } > > struct fwd_engine tx_only_engine = { > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index a808b6a..00413cc 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -266,7 +266,7 @@ show config > Displays the configuration of the application. > The configuration comes from the command-line, the runtime or the application defaults:: > > - testpmd> show config (rxtx|cores|fwd|txpkts) > + testpmd> show config (rxtx|cores|fwd|txpkts|txtimes) > > The available information categories are: > > @@ -278,6 +278,8 @@ The available information categories are: > > * ``txpkts``: Packets to TX configuration. > > +* ``txtimes``: Burst time pattern for Tx only mode. The description is not clear, can you pleaes give a little more detail? > + > For example: > > .. code-block:: console > @@ -725,6 +727,38 @@ Set the length of each segment of the TX-ONLY packets or length of packet for FL > > Where x[,y]* represents a CSV list of values, without white space. > > +set txtimes > +~~~~~~~~~~ WARNING: Title underline too short. > + > +Configure the timing burst pattern for Tx only mode. This command enables > +the packet send scheduling on dynamic timestamp mbuf field and configures > +timing pattern in Tx only mode. In this mode, if scheduling is enabled > +application provides timestamps in the packets being sent. It is possible > +to configure delay (in unspecified device clock units) between bursts > +and between the packets within the burst:: > + > + testpmd> set txtimes (inter),(intra) > + > +where: > + > +* ``inter`` is the delay between the bursts in the device clock units. > + If ``intra`` is zero, this is the time between the beginnings of the > + first packets in the neighbour bursts, if ``intra`` is not zero, > + ``inter`` specifies the time between the beginning of the first packet > + of the current burst and the beginning of the last packet of the > + previous burst. If ``inter`` parameter is zero the send scheduling > + on timestamps is disabled (default). > + > +* ``intra`` is the delay between the packets within the burst specified > + in the device clock units. The number of packets in the burst is defined > + by regular burst setting. If ``intra`` parameter is zero no timestamps > + provided in the packets excepting the first one in the burst. > + > +As the result the bursts of packet will be transmitted with specific > +delays between the packets within the burst and specific delay between > +the bursts. The rte_eth_get_clock() is supposed to be engaged to get the 'rte_eth_read_clock()'?