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 <viachesl...@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()'?
