DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Viacheslav Ovsiienko <viacheslavo@mellanox.com>, dev@dpdk.org
Cc: matan@mellanox.com, rasland@mellanox.com, olivier.matz@6wind.com,
	bernard.iremonger@intel.com, thomas@monjalon.com
Subject: Re: [dpdk-dev] [PATCH v6 2/2] app/testpmd: add send scheduling test capability
Date: Fri, 10 Jul 2020 00:58:26 +0100	[thread overview]
Message-ID: <2ee95fc5-fa97-c518-fd61-400d667ace34@intel.com> (raw)
In-Reply-To: <1594298216-3746-2-git-send-email-viacheslavo@mellanox.com>

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 <viacheslavo@mellanox.com>

<...>

> +cmdline_parse_inst_t cmd_set_txtimes = {
> +	.f = cmd_set_txtimes_parsed,
> +	.data = NULL,
> +	.help_str = "set txtimes <inter_burst>,<intra_burst>",
> +	.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()'?


  reply	other threads:[~2020-07-09 23:58 UTC|newest]

Thread overview: 86+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10  6:38 [dpdk-dev] [RFC] mbuf: accurate packet Tx scheduling Viacheslav Ovsiienko
2020-06-10 13:33 ` Harman Kalra
2020-06-10 15:16   ` Slava Ovsiienko
2020-06-17 15:57     ` [dpdk-dev] [EXT] " Harman Kalra
2020-07-01 15:46       ` Slava Ovsiienko
2020-07-01 15:36 ` [dpdk-dev] [PATCH 1/2] mbuf: introduce " Viacheslav Ovsiienko
2020-07-01 15:36   ` [dpdk-dev] [PATCH 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-07 11:50   ` [dpdk-dev] [PATCH 1/2] mbuf: introduce accurate packet Tx scheduling Olivier Matz
2020-07-07 12:46     ` Slava Ovsiienko
2020-07-07 12:59 ` [dpdk-dev] [PATCH v2 " Viacheslav Ovsiienko
2020-07-07 12:59   ` [dpdk-dev] [PATCH v2 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-07 13:08 ` [dpdk-dev] [PATCH v3 1/2] mbuf: introduce accurate packet Tx scheduling Viacheslav Ovsiienko
2020-07-07 13:08   ` [dpdk-dev] [PATCH v3 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-07 14:32   ` [dpdk-dev] [PATCH v3 1/2] mbuf: introduce accurate packet Tx scheduling Olivier Matz
2020-07-07 14:57 ` [dpdk-dev] [PATCH v4 " Viacheslav Ovsiienko
2020-07-07 14:57   ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-07 15:23   ` [dpdk-dev] [PATCH v4 1/2] mbuf: introduce accurate packet Tx scheduling Olivier Matz
2020-07-08 14:16   ` [dpdk-dev] [PATCH v4 1/2] mbuf: introduce accurate packet Txscheduling Morten Brørup
2020-07-08 14:54     ` Slava Ovsiienko
2020-07-08 15:27       ` Morten Brørup
2020-07-08 15:51         ` Slava Ovsiienko
2020-07-08 15:47 ` [dpdk-dev] [PATCH v5 1/2] mbuf: introduce accurate packet Tx scheduling Viacheslav Ovsiienko
2020-07-08 15:47   ` [dpdk-dev] [PATCH v5 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-08 16:05   ` [dpdk-dev] [PATCH v5 1/2] mbuf: introduce accurate packet Tx scheduling Slava Ovsiienko
2020-07-09 12:26   ` Thomas Monjalon
2020-07-09 12:36 ` [dpdk-dev] [PATCH v6 " Viacheslav Ovsiienko
2020-07-09 12:36   ` [dpdk-dev] [PATCH v6 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-09 23:58     ` Ferruh Yigit [this message]
2020-07-10 12:41       ` Slava Ovsiienko
2020-07-09 23:47   ` [dpdk-dev] [PATCH v6 1/2] mbuf: introduce accurate packet Tx scheduling Ferruh Yigit
2020-07-10 12:32     ` Slava Ovsiienko
2020-07-10 12:39 ` [dpdk-dev] [PATCH v7 " Viacheslav Ovsiienko
2020-07-10 12:39   ` [dpdk-dev] [PATCH v7 2/2] app/testpmd: add send scheduling test capability Viacheslav Ovsiienko
2020-07-10 15:46   ` [dpdk-dev] [PATCH v7 1/2] mbuf: introduce accurate packet Tx scheduling Slava Ovsiienko
2020-07-10 22:07     ` Ferruh Yigit
2020-07-15  6:21 ` [dpdk-dev] [PATCH v2 00/17] net/mlx5: " Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 01/17] common/mlx5: update common part to support packet pacing Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 02/17] net/mlx5: introduce send scheduling devargs Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 03/17] net/mlx5: fix UAR lock sharing for multiport devices Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 04/17] net/mlx5: introduce shared UAR resource Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 05/17] net/mlx5: create clock queue for packet pacing Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 06/17] net/mlx5: create rearm " Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 07/17] net/mlx5: create Tx queues with DevX Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 08/17] net/mlx5: allocate packet pacing context Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 09/17] net/mlx5: introduce clock queue service routine Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 10/17] net/mlx5: prepare Tx queue structures to support timestamp Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 11/17] net/mlx5: convert timestamp to completion index Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 12/17] net/mlx5: prepare Tx datapath to support sheduling Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 13/17] net/mlx5: add scheduling support to send routine template Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 14/17] net/mlx5: add read device clock support Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 15/17] net/mlx5: provide the send scheduling error statistics Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 16/17] common/mlx5: add register access DevX routine Viacheslav Ovsiienko
2020-07-15  6:21   ` [dpdk-dev] [PATCH v2 17/17] net/mlx5: convert Rx timestamps in realtime format Viacheslav Ovsiienko
2020-07-16  8:23 ` [dpdk-dev] [PATCH v3 00/17] net/mlx5: introduce accurate packet Tx scheduling Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 01/17] common/mlx5: update common part to support packet pacing Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 02/17] net/mlx5: introduce send scheduling devargs Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 03/17] net/mlx5: fix UAR lock sharing for multiport devices Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 04/17] net/mlx5: introduce shared UAR resource Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 05/17] net/mlx5: create clock queue for packet pacing Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 06/17] net/mlx5: create rearm " Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 07/17] net/mlx5: create Tx queues with DevX Viacheslav Ovsiienko
2020-07-20 14:18     ` Ferruh Yigit
2020-07-20 15:25       ` Ferruh Yigit
2020-07-21 11:35         ` Slava Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 08/17] net/mlx5: allocate packet pacing context Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 09/17] net/mlx5: introduce clock queue service routine Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 10/17] net/mlx5: prepare Tx queue structures to support timestamp Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 11/17] net/mlx5: convert timestamp to completion index Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 12/17] net/mlx5: prepare Tx datapath to support sheduling Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 13/17] net/mlx5: add scheduling support to send routine template Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 14/17] net/mlx5: add read device clock support Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 15/17] net/mlx5: provide the send scheduling error statistics Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 16/17] common/mlx5: add register access DevX routine Viacheslav Ovsiienko
2020-07-16  8:23   ` [dpdk-dev] [PATCH v3 17/17] net/mlx5: convert Rx timestamps in realtime format Viacheslav Ovsiienko
2020-07-16 20:20   ` [dpdk-dev] [PATCH v3 00/17] net/mlx5: introduce accurate packet Tx scheduling Raslan Darawsheh
2020-07-17 14:28 ` [dpdk-dev] [PATCH 1/3] net/mlx5: fix compilation issue with missing DevX event Viacheslav Ovsiienko
2020-07-17 14:28   ` [dpdk-dev] [PATCH 2/3] net/mlx5: fix compilation issue with atomic128 exchange Viacheslav Ovsiienko
2020-07-17 15:08     ` Thomas Monjalon
2020-07-17 15:15       ` Slava Ovsiienko
2020-07-17 14:28   ` [dpdk-dev] [PATCH 3/3] common/mlx5: fix DevX register access opcode Viacheslav Ovsiienko
2020-07-17 15:05     ` Thomas Monjalon
2020-07-17 15:11       ` Slava Ovsiienko
2020-07-17 15:19         ` Thomas Monjalon
2020-07-17 15:23           ` Slava Ovsiienko
2020-07-17 15:59             ` Thomas Monjalon
2020-07-18 13:38   ` [dpdk-dev] [PATCH 1/3] net/mlx5: fix compilation issue with missing DevX event Raslan Darawsheh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2ee95fc5-fa97-c518-fd61-400d667ace34@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=olivier.matz@6wind.com \
    --cc=rasland@mellanox.com \
    --cc=thomas@monjalon.com \
    --cc=viacheslavo@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).