Re: [PATCH net v1 1/3] net: introduce helper to resolve hardware timestamps from skb

Kohei Enju
2026-04-30T04:50:59Z

On 04/29 17:04, Willem de Bruijn wrote:
> Kohei Enju wrote:
> > Move the logic that resolves a hardware timestamp from an skb, including
> > late timestamp resolution via netdev_get_tstamp(), from net/socket.c to
> > a common helper.
> >
> > Let's allow other networking code to reuse the same resolution path.
> >
> > Signed-off-by: Kohei Enju <kohei@enjuk.jp>
>
> Thanks for the fix series.
>
> Fixes require a Fixes tag. See also https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html.
>
> I suggest merging this and the second patch, as this is not a
> standalone fix, and the other is a one-line change.

Thank you for the suggestion, and it looks good to me.
I'll do so in v2.

>
> > ---
> > include/linux/skbuff.h | 11 +++++++++++
> > net/core/skbuff.c | 27 +++++++++++++++++++++++++++
> > net/socket.c | 27 +++------------------------
> > 3 files changed, 41 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 2bcf78a4de7b..651a5ae8b11c 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -4731,6 +4731,17 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb, const struct sk_buff *ack_skb,
> > void skb_tstamp_tx(struct sk_buff *orig_skb,
> > struct skb_shared_hwtstamps *hwtstamps);
> >
> > +/**
> > + * skb_get_hwtstamp - resolve a hardware timestamp from an skb
> > + * @skb: skb carrying the timestamp
> > + * @cycles: true to request the free-running cycle-based timestamp
> > + * @if_index: optional return pointer for the originating netdev ifindex
> > + *
> > + * Return: resolved hardware timestamp, or the stored skb hwtstamp when no
> > + * device-specific late timestamp resolution is needed.
> > + */
> > +ktime_t skb_get_hwtstamp(struct sk_buff *skb, bool cycles, int *if_index);
> > +
> > /**
> > * skb_tx_timestamp() - Driver hook for transmit timestamping
> > *
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 7dad68e3b518..d11f4e2e9391 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5729,6 +5729,33 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
> > }
> > EXPORT_SYMBOL_GPL(skb_tstamp_tx);
> >
> > +ktime_t skb_get_hwtstamp(struct sk_buff *skb, bool cycles, int *if_index)
> > +{
> > + struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> > + struct net_device *orig_dev;
> > + ktime_t hwtstamp;
> > +
> > + if (if_index)
> > + *if_index = 0;
> > +
> > + if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV))
> > + return shhwtstamps->hwtstamp;
> > +
> > + rcu_read_lock();
> > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > + if (orig_dev) {
> > + if (if_index)
> > + *if_index = orig_dev->ifindex;
> > + hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, cycles);
> > + } else {
> > + hwtstamp = shhwtstamps->hwtstamp;
> > + }
> > + rcu_read_unlock();
> > +
> > + return hwtstamp;
> > +}
> > +EXPORT_SYMBOL_GPL(skb_get_hwtstamp);
> > +
> > #ifdef CONFIG_WIRELESS
> > void skb_complete_wifi_ack(struct sk_buff *skb, bool acked)
> > {
> > diff --git a/net/socket.c b/net/socket.c
> > index 22a412fdec07..95b21b16a0fc 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -876,21 +876,7 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
> > static ktime_t get_timestamp(struct sock *sk, struct sk_buff *skb, int *if_index)
> > {
> > bool cycles = READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC;
> > - struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> > - struct net_device *orig_dev;
> > - ktime_t hwtstamp;
> > -
> > - rcu_read_lock();
> > - orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > - if (orig_dev) {
> > - *if_index = orig_dev->ifindex;
> > - hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, cycles);
> > - } else {
> > - hwtstamp = shhwtstamps->hwtstamp;
> > - }
> > - rcu_read_unlock();
> > -
> > - return hwtstamp;
> > + return skb_get_hwtstamp(skb, cycles, if_index);
> > }
>
> At this point simpler to remove get_timestamp entirely. It's an
> unnecessary layer of indirection.
>
> Perhaps pass sk_tsflags rather than bool to skb_get_hwtstamp.

Good points. I'll rework this in v2.

Thanks,
Kohei

>
> > static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
> > @@ -940,7 +926,6 @@ int skb_get_tx_timestamp(struct sk_buff *skb, struct sock *sk,
> > {
> > u32 tsflags = READ_ONCE(sk->sk_tsflags);
> > ktime_t hwtstamp;
> > - int if_index = 0;
> >
> > if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> > ktime_to_timespec64_cond(skb->tstamp, ts))
> > @@ -950,10 +935,7 @@ int skb_get_tx_timestamp(struct sk_buff *skb, struct sock *sk,
> > skb_is_swtx_tstamp(skb, false))
> > return -ENOENT;
> >
> > - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> > - hwtstamp = get_timestamp(sk, skb, &if_index);
> > - else
> > - hwtstamp = skb_hwtstamps(skb)->hwtstamp;
> > + hwtstamp = get_timestamp(sk, skb, NULL);
> >
> > if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
> > hwtstamp = ptp_convert_timestamp(&hwtstamp,
> > @@ -1033,10 +1015,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
> > !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > if_index = 0;
> > - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> > - hwtstamp = get_timestamp(sk, skb, &if_index);
> > - else
> > - hwtstamp = shhwtstamps->hwtstamp;
> > + hwtstamp = get_timestamp(sk, skb, &if_index);
> >
> > if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
> > hwtstamp = ptp_convert_timestamp(&hwtstamp,
> > --
> > 2.53.0
> >
>
>