Hi Vivien,
On Tue, Jun 23, 2020 at 06:10:09PM -0400, Vivien Didelot wrote:
>
> On Wed, Jun 17, 2020 at 4:16 AM Ferruh Yigit <ferruh.yi...@intel.com> wrote:
>
> > On 6/10/2020 8:39 PM, Vivien Didelot wrote:
> > > When hardware timestamping is enabled on Rx path, system time should
> > > no longer be used to calculate the timestamps when dumping packets.
> > >
> > > Instead, use the value stored by the driver in mbuf->timestamp
> > > and assume it is already converted to nanoseconds (otherwise the
> > > application may edit the packet headers itself afterwards).
> > >
> > > Signed-off-by: Vivien Didelot <vivien.dide...@gmail.com>
> > > Signed-off-by: Patrick Keroulas <patrick.kerou...@radio-canada.ca>
> > > ---
> > > doc/guides/rel_notes/release_20_08.rst | 1 +
> > > drivers/net/pcap/rte_eth_pcap.c | 30 +++++++++++++++-----------
> > > 2 files changed, 18 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > > index 7a67c960c..cedceaf9d 100644
> > > --- a/doc/guides/rel_notes/release_20_08.rst
> > > +++ b/doc/guides/rel_notes/release_20_08.rst
> > > @@ -61,6 +61,7 @@ New Features
> > > Updated PCAP driver with new features and improvements, including:
> > >
> > > * Support software Tx nanosecond timestamps precision.
> > > + * Support hardware Tx timestamps.
> > >
> > > * **Updated Mellanox mlx5 driver.**
> > >
> > > diff --git a/drivers/net/pcap/rte_eth_pcap.c
> > b/drivers/net/pcap/rte_eth_pcap.c
> > > index 13a3d0ac7..3d80b699b 100644
> > > --- a/drivers/net/pcap/rte_eth_pcap.c
> > > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > > @@ -290,19 +290,23 @@ eth_null_rx(void *queue __rte_unused,
> > > #define NSEC_PER_SEC 1000000000L
> > >
> > > static inline void
> > > -calculate_timestamp(struct timeval *ts) {
> > > - uint64_t cycles;
> > > - struct timeval cur_time;
> > > +calculate_timestamp(const struct rte_mbuf *mbuf, struct timeval *ts) {
> > > + if (mbuf->ol_flags & PKT_RX_TIMESTAMP) {
> > > + ts->tv_sec = mbuf->timestamp / NSEC_PER_SEC;
> > > + ts->tv_usec = mbuf->timestamp % NSEC_PER_SEC;
> >
> > Hi Vivien,
> >
> > No objection from pcap PMD point of view.
> >
> > But should we have a Tx mbuf flag, 'PKT_TX_TIMESTAMP', for applications to
> > request drivers to use the timestamp field on Tx path? Not sure if there
> > can be
> > any problem on using Rx flag on both direction?
> >
> > Also the metric is not defined for the 'mbuf->timestamp', it doesn't need
> > to be
> > nanoseconds, not sure if it is correct to assume it is. Or should we
> > define a
> > metric for timestamp on the Tx path?
> >
> > cc'ed Oliver, I think he can comment better on above two questions.
> >
> > Thanks,
> > ferruh
> >
> >
> Hi Oliver,
>
> Surprisingly, dumping PCAP with hardware timestamps seems to be a niche,
> but we do need this feature for our network analyzing tool.
>
> Do you guys have objections for this patch?
>
> Regards,
> Vivien
>
As said by Ferruh, the unit of timestamp in mbuf is not normalized to
nanosecs, as seen in rte_mbuf_core.h:
/** Valid if PKT_RX_TIMESTAMP is set. The unit and time reference
* are not normalized but are always the same for a given port.
* Some devices allow to query rte_eth_read_clock that will return the
* current device timestamp.
*/
uint64_t timestamp;
Using the timestamp generated from a port which is not a pmd-pcap would
require a conversion, using rte_eth_read_clock() on mbuf->port (assuming
it was not modified, which should be true except if event eth Tx adapter
is used).
Also, note that the timestamp corresponds to the Rx timestamp. I think it
could be an issue in case the mbuf is reassembled by the application: the
timestamp in reassembled mbuf would be the one from the first fragment.
So, I share Ferruh's concerns.
If the problem is about calculate_timestamp() speed, I think there is
some room for optimization: 1e6 is a float, so it will probably slow
down the function. I think the following should also work, and would
be faster:
static inline void
calculate_timestamp(struct timeval *ts)
{
uint64_t cycles;
struct timeval cur_time;
cycles = rte_get_timer_cycles() - start_cycles;
cur_time.tv_sec = cycles / hz;
cycles -= cur_time.tv_sec * hz;
cur_time.tv_usec = (cycles * 1000000) / hz;
timeradd(&start_time, &cur_time, ts);
}
I also think the call to timeradd() could be removed if an offset
is added to start_cycles.
Regards,
Olivier
