Re: [dpdk-dev] [PATCH v6 2/2] app/testpmd: add send scheduling test capability

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi Ferruh,

Thanks a lot for the comments, addressed all of them.

With best regards, Slava

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, July 10, 2020 2:58
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <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
> 
> 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()'?