Re: [RFC PATCH v2 net-next] net: Preserve skb delivery time during forward

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Dec 16, 2021 at 05:58:49PM -0500, Willem de Bruijn wrote:
> > > > @@ -530,7 +538,14 @@ struct skb_shared_info {
> > > >         /* Warning: this field is not always filled in (UFO)! */
> > > >         unsigned short  gso_segs;
> > > >         struct sk_buff  *frag_list;
> > > > -       struct skb_shared_hwtstamps hwtstamps;
> > > > +       union {
> > > > +               /* If SKBTX_DELIVERY_TSTAMP is set in tx_flags,
> > > > +                * tx_delivery_tstamp is stored instead of
> > > > +                * hwtstamps.
> > > > +                */
> > >
> > > Should we just encode the timebase and/or type { timestamp,
> > > delivery_time } in th lower bits of the timestamp field? Its
> > > resolution is higher than actual clock precision.
> > In skb->tstamp ?
> 
> Yes. Arguably a hack, but those bits are in the noise now, and it
> avoids the clone issue with skb_shinfo (and scarcity of flag bits
> there).
> 
> > >
> > > is non-zero skb->tstamp test not sufficient, instead of
> > > SKBTX_DELIVERY_TSTAMP_ALLOW_FWD.
> > >
> > > It is if only called on the egress path. Is bpf on ingress the only
> > > reason for this?
> > Ah. ic.  meaning testing non-zero skb->tstamp and then call
> > skb_save_delivery_time() only during the veth-egress-path:
> > somewhere in veth_xmit() => veth_forward_skb() but before
> > skb->tstamp was reset to 0 in __dev_forward_skb().
> 
> Right. If delivery_time is the only use of skb->tstamp on egress, and
> timestamp is the only use on ingress, then the only time the
> delivery_time needs to be cached if when looping from egress to
> ingress and this field is non-zero.
> 
> >
> > Keep *_forward() and bpf_out_*() unchanged (i.e. keep skb->tstamp = 0)
> > because the skb->tstamp could be stamped by net_timestamp_check().
> >
> > Then SKBTX_DELIVERY_TSTAMP_ALLOW_FWD is not needed.
> >
> > Did I understand your suggestion correctly?
> 
> I think so.
> 
> But the reality is complicated if something may be setting a delivery
> time on ingress (a BPF filter?)
If bpf@ingress needs to set a delivery_time, the only reasonable
usecase is to finally egress it out by calling bpf_redirect_neigh().
One option is to have a new bpf_redirect_*() helper to take the fifth
'delivery_time' argument and have the skb_do_redirect() to set
the delivery_time in skb.  An extra helper is not ideal but probably
acceptable considering other tricky constraints we are working with.

Another potential issue is,
after looping from egress to ingress, the skb->tstamp has the delivery_time.
If it is passing up to the stack, it needs to be reset back to
timestamp (skb->tstamp = ktime_get_real()) before it is used.
Not sure what is the best place to do it (?) ....hmm 

> >
> > However, we still need a bit to distinguish tx_delivery_tstamp
> > from hwtstamps.
> >
> > >
> > > > +{
> > > > +       if (skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP_ALLOW_FWD) {
> > > > +               skb_shinfo(skb)->tx_delivery_tstamp = skb->tstamp;
> > > > +               skb_shinfo(skb)->tx_flags |= SKBTX_DELIVERY_TSTAMP;
> > > > +               skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP_ALLOW_FWD;
> > > > +       }
> > >
> > > Is this only called when there are no clones/shares?
> > No, I don't think so.  TCP clone it.  I also started thinking about
> > this after noticing a mistake in the change in  __tcp_transmit_skb().
> >
> > There are other places that change tx_flags, e.g. tcp_offload.c.
> > It is not shared at those places or there is some specific points
> > in the stack that is safe to change ?
> 
> The packet probably is not yet shared. Until the TCP stack gives a
> packet to the IP layer, it can treat it as exclusive.
> 
> Though it does seem that these fields are accessed in a possibly racy
> manner. Drivers with hardware tx timestamp offload may set
> skb_shinfo(orig_skb)->tx_flags & SKBTX_IN_PROGRESS without checking
> whether the skb may be cloned.