Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Gerhard Engleder <gerhard@engleder-embedded.com> writes:

>> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>> > > > >       if (shhwtstamps &&
>> > > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>> > > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
>> > > > > +             rcu_read_lock();
>> > > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
>> > > >
>> > > > __sock_recv_timestamp() is hot path.
>> > > >
>> > > > No need to call dev_get_by_napi_id() for the vast majority of cases
>> > > > using plain old MAC time stamping.
>> > >
>> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
>> >
>> > No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.
>>
>> You are right, my fault.
>>
>> > > That's the reason for the removal of a separate flag, which signals the need to
>> > > timestamp determination based on address/cookie. I thought there is no need
>> > > for that flag, as netdev is already available later in the existing code.
>> > >
>> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
>> > >
>> > > This flag tells netdev_get_tstamp() which timestamp is required. If it
>> > > is not set, then
>> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But
>> > > this normal
>> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
>> > > called.
>> >
>> > It should be this:
>> >
>> > - normal, non-vclock:   use hwtstamps->hwtstamp directly
>> > - vclock:               use slower path with lookup
>> >
>> > I don't see why you can't implement that.
>>
>> I will try to implement it that way.
>
> I'm thinking about why there should be a slow path with lookup. If the
> address/cookie
> points to a defined data structure with two timestamps, then no lookup
> for the phc or
> netdev is necessary. It should be possible for every driver to
> allocate a skbuff with enough
> space for this structure in front of the received Ethernet frame. The
> structure could be:
>
> struct skb_inline_hwtstamps {
>         ktime_t hwtstamp;
>         ktime_t hwcstamp;
> };
>
> Actually my device and igc are storing the timestamps in front of the
> received Ethernet
> frame. In my opinion it is obvious to the store metadata of received
> Ethernet frames in
> front of it, because it eliminates the need for another DMA transfer.
> What is your opinion
> Vinicius?

If I am understanding this right, the idea is providing both "cycles"
(free running cycles measurement) and PHC timestamp for all packets, for
igc, it will work fine for RX (the HW already writes the timestamps for
two timer registers in the host memory), but for TX it's going be
awkward/slow (I would have to read two extra registers), but I think
it's still possible.

But it would be best to avoid the overhead, and only providing the
"extra" (the cycles one) measurement if necessary for TX, so
SKBTX_HW_TSTAMP_USE_CYCLES would still be needed.

So, in short, I am fine with it, as long as I can get away with only
providing the cycles measurement for TX if necessary.

>
> Gerhard

Cheers,
-- 
Vinicius