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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Apr 2022 22:28:10 +0200
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     Richard Cochran <richardcochran@...il.com>, yangbo.lu@....com,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, mlichvar@...hat.com,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

> >> > > > > @@ -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
On Tue, Apr 12, 2022 at 11:05 PM Vinicius Costa Gomes
<vinicius.gomes@...el.com> wrote:
>
> Gerhard Engleder <gerhard@...leder-embedded.com> writes:
> 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.

Of course for TX only cycles measurement shall be provided and
SKBTX_HW_TSTAMP_USE_CYCLES is used for that.

Thanks!

Gerhard