[2/2] net/pcap: add TODO for writing Tx HW timestamp
diff mbox series

Message ID 20200523172130.2285380-2-vivien.didelot@gmail.com
State Rejected
Delegated to: Ferruh Yigit
Headers show
Series
  • [1/2] net/pcap: support software Tx nanosecond timestamp
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Vivien Didelot May 23, 2020, 5:21 p.m. UTC
In order to write a packet with hardware timestamp enabled into a
PCAP file, we need to convert its device specific raw timestamp first.

This might not be trivial since querying the raw clock value from
the device is still experimental, and derivating the frequency would
ideally require an additional alarm thread.

As a first step, pass the mbuf to the timestamp calculation function
since mbuf->port and mbuf->timestamp would be needed, and add a TODO
note. No functional changes.

Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger May 24, 2020, 1:38 a.m. UTC | #1
On Sat, 23 May 2020 13:21:30 -0400
Vivien Didelot <vivien.didelot@gmail.com> wrote:

> In order to write a packet with hardware timestamp enabled into a
> PCAP file, we need to convert its device specific raw timestamp first.
> 
> This might not be trivial since querying the raw clock value from
> the device is still experimental, and derivating the frequency would
> ideally require an additional alarm thread.
> 
> As a first step, pass the mbuf to the timestamp calculation function
> since mbuf->port and mbuf->timestamp would be needed, and add a TODO
> note. No functional changes.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> ---
>  drivers/net/pcap/rte_eth_pcap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 68588c3d7..f205a28e0 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -290,10 +290,16 @@ eth_null_rx(void *queue __rte_unused,
>  #define NSEC_PER_SEC	1e9
>  
>  static inline void
> -calculate_timestamp(struct timeval *ts) {
> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
>  	uint64_t cycles;
>  	struct timeval cur_time;
>  
> +	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> +		/* TODO: convert mbuf->timestamp into nanoseconds instead.
> +                 * See rte_eth_read_clock().
> +                 */
> +	}
> +
>  	cycles = rte_get_timer_cycles() - start_cycles;
>  	cur_time.tv_sec = cycles / hz;
>  	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
> @@ -339,7 +345,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			caplen = sizeof(temp_data);
>  		}
>  
> -		calculate_timestamp(&header.ts);
> +		calculate_timestamp(mbuf, &header.ts);
>  		header.len = len;
>  		header.caplen = caplen;
>  		/* rte_pktmbuf_read() returns a pointer to the data directly
> -- 

NAK
What is the point of this patch.
Most projects do not accept speculative patches. Instead incorporate this
patch in when you have code that uses it.
Ferruh Yigit June 3, 2020, 5:31 p.m. UTC | #2
On 5/24/2020 2:38 AM, Stephen Hemminger wrote:
> On Sat, 23 May 2020 13:21:30 -0400
> Vivien Didelot <vivien.didelot@gmail.com> wrote:
> 
>> In order to write a packet with hardware timestamp enabled into a
>> PCAP file, we need to convert its device specific raw timestamp first.
>>
>> This might not be trivial since querying the raw clock value from
>> the device is still experimental, and derivating the frequency would
>> ideally require an additional alarm thread.
>>
>> As a first step, pass the mbuf to the timestamp calculation function
>> since mbuf->port and mbuf->timestamp would be needed, and add a TODO
>> note. No functional changes.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>> ---
>>  drivers/net/pcap/rte_eth_pcap.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
>> index 68588c3d7..f205a28e0 100644
>> --- a/drivers/net/pcap/rte_eth_pcap.c
>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>> @@ -290,10 +290,16 @@ eth_null_rx(void *queue __rte_unused,
>>  #define NSEC_PER_SEC	1e9
>>  
>>  static inline void
>> -calculate_timestamp(struct timeval *ts) {
>> +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
>>  	uint64_t cycles;
>>  	struct timeval cur_time;
>>  
>> +	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
>> +		/* TODO: convert mbuf->timestamp into nanoseconds instead.
>> +                 * See rte_eth_read_clock().
>> +                 */
>> +	}
>> +
>>  	cycles = rte_get_timer_cycles() - start_cycles;
>>  	cur_time.tv_sec = cycles / hz;
>>  	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
>> @@ -339,7 +345,7 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>>  			caplen = sizeof(temp_data);
>>  		}
>>  
>> -		calculate_timestamp(&header.ts);
>> +		calculate_timestamp(mbuf, &header.ts);
>>  		header.len = len;
>>  		header.caplen = caplen;
>>  		/* rte_pktmbuf_read() returns a pointer to the data directly
>> -- 
> 
> NAK
> What is the point of this patch.
> Most projects do not accept speculative patches. Instead incorporate this
> patch in when you have code that uses it.
> 

+1

Patch
diff mbox series

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 68588c3d7..f205a28e0 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -290,10 +290,16 @@  eth_null_rx(void *queue __rte_unused,
 #define NSEC_PER_SEC	1e9
 
 static inline void
-calculate_timestamp(struct timeval *ts) {
+calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
 	uint64_t cycles;
 	struct timeval cur_time;
 
+	if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
+		/* TODO: convert mbuf->timestamp into nanoseconds instead.
+                 * See rte_eth_read_clock().
+                 */
+	}
+
 	cycles = rte_get_timer_cycles() - start_cycles;
 	cur_time.tv_sec = cycles / hz;
 	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
@@ -339,7 +345,7 @@  eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			caplen = sizeof(temp_data);
 		}
 
-		calculate_timestamp(&header.ts);
+		calculate_timestamp(mbuf, &header.ts);
 		header.len = len;
 		header.caplen = caplen;
 		/* rte_pktmbuf_read() returns a pointer to the data directly