[PATCH v2] Added DPDK driver HW timestamp Mbuf extention support. #156
Conversation
odpbuild
changed the title
odpbuild
changed the title
|
@MatiasElo Could you take a look when would have a minute ? |
| /**Apply hardware timestamp on pkt input. Should be supported | ||
| * by NIC driver. */ | ||
| uint64_t ts_hw : 1; | ||
|
|
There was a problem hiding this comment.
All ODP API changes are done through the linux-generic project (https://github.com/OpenDataPlane/odp). At a glance this addition seems unnecessary. The existing ts_all field could be used instead and the implementation can decide to use SW/HW based on the HW capabilities.
There was a problem hiding this comment.
That of corse may be hidden under ts_all. In that case user is not able to select which one to use if there are two options. And some priorities on selection needs to be added. Which would not handle all the cases right - only behaving by some algorithm. We loosing flexibility. I think it's possible to add the same API to linux-generic withSOF_TIMESTAMPING_RX_HARDWARE support.
There was a problem hiding this comment.
The same flexibility could also be achieved by using the config file (config/odp-linux-dpdk.conf) without having to modify the API. Anyway, as API modifications are done through the linux-generic repo, this change should be removed from this PR.
There was a problem hiding this comment.
Just to clarify the logic I'm suggesting:
- Use HW by default if available
- Use SW if HW not available
- Default can be overridden using the config file
| @@ -136,6 +136,14 @@ const pktio_if_ops_t * const _odp_pktio_if_ops[] = { | |||
| NULL | |||
| }; | |||
|
|
|||
| /* Timestamp offset in rte mbuf.*/ | |||
| static int rte_mbuf_hw_ts_dynfield_offset = -1; | |||
There was a problem hiding this comment.
This value should be stored inside pkt_dpdk_t to enable process mode operation.
| if (pktio_entry->s.config.pktin.bit.ts_hw) { | ||
| rx_offloads |= DEV_RX_OFFLOAD_TIMESTAMP; | ||
|
|
||
| rte_mbuf_dyn_rx_timestamp_register(&rte_mbuf_hw_ts_dynfield_offset, NULL); | ||
| if (rte_mbuf_hw_ts_dynfield_offset < 0) { | ||
| //Disable HW mbuf timestamping since we would not be able to read it. | ||
| pktio_entry->s.config.pktin.bit.ts_hw = 0; | ||
| ODP_ERR("rte_mbuf_dyn_rx_timestamp_register failed. ts_hw disabled!\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
As I mentioned in the API change comment, perhaps a better solution would be to check if HW timestamping is supported, and if not, fall back to using SW timestamps.
There was a problem hiding this comment.
In general that would work for "my" case. But in this variant we loosing flexibility. And on processing stage not really sure from where TS is going from .
| /*Moved this into pkt processing cycle since we want to have unique TS for each ptk, rather than one for group*/ | ||
| if (pktio_entry->s.config.pktin.bit.ts_all || | ||
| pktio_entry->s.config.pktin.bit.ts_ptp) { | ||
| ts_val = odp_time_global(); | ||
| ts = &ts_val; | ||
| } |
There was a problem hiding this comment.
Taking a separate SW timestamp for each packet causes major performance overhead. And anyway, all packets processed in this loop have been received in the same rte_eth_rx_burst() burst.
There was a problem hiding this comment.
I understood it's from the same burst... just same ts may cause issues in some cases. Would remove this change
| if (pktio_entry->s.config.pktin.bit.ts_hw) { | ||
| rx_offloads |= DEV_RX_OFFLOAD_TIMESTAMP; | ||
|
|
||
| rte_mbuf_dyn_rx_timestamp_register(&rte_mbuf_hw_ts_dynfield_offset, NULL); |
There was a problem hiding this comment.
ODP-DPDK supports both current DPDK LTS releases (v19.11, v20.11) and this API is not yet available in DPDK v19.11.
There was a problem hiding this comment.
What is also problematic with this API is that it doesn't specify anything about the resolution or units of timestamps, which would be required to implement odp_pktio_ts_res().
There was a problem hiding this comment.
It's HW timestamp do the odp_pktio_ts_res() still nedded ?
There was a problem hiding this comment.
Yes, odp_pktio_ts_res(), odp_pktio_ts_from_ns(), and odp_pktio_time() are required. To implement these functions you likely need to use rte_eth_read_clock() and measure the HW clock frequency during odp_pktio_open(), since there is no DPDK API to read this information.
You also need to ifdef the code to fix build with DPDK 19.11.
#131