| Message ID | 20180703224300.25300-2-jesus.sanchez-palencia@intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Scheduled packet Transmission: ETF | expand |
On 07/03/2018 03:42 PM, Jesus Sanchez-Palencia wrote: > This is done in preparation for the upcoming time based transmission > patchset. Now that skb->tstamp will be used to hold packet's txtime, > we must ensure that it is being cleared when traversing namespaces. > Also, doing that from skb_scrub_packet() before the early return would > break our feature when tunnels are used. > > Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> > --- > net/core/skbuff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 1357f36c8a5e..c4e24ac27464 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); > */ > void skb_scrub_packet(struct sk_buff *skb, bool xnet) > { > - skb->tstamp = 0; > skb->pkt_type = PACKET_HOST; > skb->skb_iif = 0; > skb->ignore_df = 0; > @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) > > ipvs_reset(skb); > skb->mark = 0; > + skb->tstamp = 0; > } > EXPORT_SYMBOL_GPL(skb_scrub_packet); > > I believe we had some misunderstanding here. What I meant by forwarding is the following case : - We receive a packet. - netstamp_wanted is >0 (because at least one packet capture is active) - __net_timestamp() is called and does : skb->tstamp = ktime_get_real(); Then this skb is forwarded into an interface where EDT is taken into consideration by either a qdisc or a device. Since CLOCK_TAI is a different base than CLOCK_REALTIME, we might have a problem. Solutions for this problem : 1) Convert all our skb->tstamp usages to CLOCK_TAI base. or 2) clear skb->tstamp in forwarding paths, including the ones not scrubbing the packet. My preference is 1), even if it is a bit more work.
Hi Eric, On 07/13/2018 10:35 AM, Eric Dumazet wrote: > > > On 07/03/2018 03:42 PM, Jesus Sanchez-Palencia wrote: >> This is done in preparation for the upcoming time based transmission >> patchset. Now that skb->tstamp will be used to hold packet's txtime, >> we must ensure that it is being cleared when traversing namespaces. >> Also, doing that from skb_scrub_packet() before the early return would >> break our feature when tunnels are used. >> >> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> >> --- >> net/core/skbuff.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index 1357f36c8a5e..c4e24ac27464 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); >> */ >> void skb_scrub_packet(struct sk_buff *skb, bool xnet) >> { >> - skb->tstamp = 0; >> skb->pkt_type = PACKET_HOST; >> skb->skb_iif = 0; >> skb->ignore_df = 0; >> @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) >> >> ipvs_reset(skb); >> skb->mark = 0; >> + skb->tstamp = 0; >> } >> EXPORT_SYMBOL_GPL(skb_scrub_packet); >> >> > > > > I believe we had some misunderstanding here. > > What I meant by forwarding is the following case : > > - We receive a packet. > - netstamp_wanted is >0 (because at least one packet capture is active) > - __net_timestamp() is called and does : > skb->tstamp = ktime_get_real(); > > Then this skb is forwarded into an interface where EDT is taken into > consideration by either a qdisc or a device. > > Since CLOCK_TAI is a different base than CLOCK_REALTIME, we might have a problem. I'm not sure we have a problem here. For the Tx path I only see net_timestamp_set() being called from dev_queue_xmit_nit(). And even there, it's a clone of the skb that gets timestamped. I believe the original skb, which had the valid txtime copied into skb->tstamp, is not modified anywhere along that path. What am I missing, please? Thanks, Jesus > > > Solutions for this problem : > > 1) Convert all our skb->tstamp usages to CLOCK_TAI base. > > or > > 2) clear skb->tstamp in forwarding paths, including the ones not scrubbing the packet. > > My preference is 1), even if it is a bit more work. >
On 07/16/2018 02:52 PM, Jesus Sanchez-Palencia wrote: > Hi Eric, > > > > On 07/13/2018 10:35 AM, Eric Dumazet wrote: >> >> >> On 07/03/2018 03:42 PM, Jesus Sanchez-Palencia wrote: >>> This is done in preparation for the upcoming time based transmission >>> patchset. Now that skb->tstamp will be used to hold packet's txtime, >>> we must ensure that it is being cleared when traversing namespaces. >>> Also, doing that from skb_scrub_packet() before the early return would >>> break our feature when tunnels are used. >>> >>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> >>> --- >>> net/core/skbuff.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index 1357f36c8a5e..c4e24ac27464 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); >>> */ >>> void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>> { >>> - skb->tstamp = 0; >>> skb->pkt_type = PACKET_HOST; >>> skb->skb_iif = 0; >>> skb->ignore_df = 0; >>> @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>> >>> ipvs_reset(skb); >>> skb->mark = 0; >>> + skb->tstamp = 0; >>> } >>> EXPORT_SYMBOL_GPL(skb_scrub_packet); >>> >>> >> >> >> >> I believe we had some misunderstanding here. >> >> What I meant by forwarding is the following case : >> >> - We receive a packet. >> - netstamp_wanted is >0 (because at least one packet capture is active) >> - __net_timestamp() is called and does : >> skb->tstamp = ktime_get_real(); >> >> Then this skb is forwarded into an interface where EDT is taken into >> consideration by either a qdisc or a device. >> >> Since CLOCK_TAI is a different base than CLOCK_REALTIME, we might have a problem. > > > I'm not sure we have a problem here. For the Tx path I only see > net_timestamp_set() being called from dev_queue_xmit_nit(). And even there, it's > a clone of the skb that gets timestamped. > > I believe the original skb, which had the valid txtime copied into skb->tstamp, > is not modified anywhere along that path. > > What am I missing, please? > > Thanks, > Jesus > I am simply stating that a linux router, receiving packet on ethX and forwarding them on ethY, could have a problem if ethY has a qdisc looking at skb->tstamp assuming a timestamp in CLOCK_TAI base. In this case, skb->tstamp would have been set at ingress (not using CLOCK_TAI but CLOCK_REALTIME), and would be read at egress (assuming CLOCK_TAI) Normal IPV4 routing path would be in net/ipv4/ip_forward.c, no scrubbing ever happens, and no cloning either. Your patch (Clear skb->tstamp only on the forwarding path) is not handling the typical forward path, only the cases where 'scrubbing' is used. > > >> >> >> Solutions for this problem : >> >> 1) Convert all our skb->tstamp usages to CLOCK_TAI base. >> >> or >> >> 2) clear skb->tstamp in forwarding paths, including the ones not scrubbing the packet. >> >> My preference is 1), even if it is a bit more work. >>
Hi Eric, On 07/16/2018 04:15 PM, Eric Dumazet wrote: > > > On 07/16/2018 02:52 PM, Jesus Sanchez-Palencia wrote: >> Hi Eric, >> >> >> >> On 07/13/2018 10:35 AM, Eric Dumazet wrote: >>> >>> >>> On 07/03/2018 03:42 PM, Jesus Sanchez-Palencia wrote: >>>> This is done in preparation for the upcoming time based transmission >>>> patchset. Now that skb->tstamp will be used to hold packet's txtime, >>>> we must ensure that it is being cleared when traversing namespaces. >>>> Also, doing that from skb_scrub_packet() before the early return would >>>> break our feature when tunnels are used. >>>> >>>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> >>>> --- >>>> net/core/skbuff.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>>> index 1357f36c8a5e..c4e24ac27464 100644 >>>> --- a/net/core/skbuff.c >>>> +++ b/net/core/skbuff.c >>>> @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); >>>> */ >>>> void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>>> { >>>> - skb->tstamp = 0; >>>> skb->pkt_type = PACKET_HOST; >>>> skb->skb_iif = 0; >>>> skb->ignore_df = 0; >>>> @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>>> >>>> ipvs_reset(skb); >>>> skb->mark = 0; >>>> + skb->tstamp = 0; >>>> } >>>> EXPORT_SYMBOL_GPL(skb_scrub_packet); >>>> >>>> >>> >>> >>> >>> I believe we had some misunderstanding here. >>> >>> What I meant by forwarding is the following case : >>> >>> - We receive a packet. >>> - netstamp_wanted is >0 (because at least one packet capture is active) >>> - __net_timestamp() is called and does : >>> skb->tstamp = ktime_get_real(); >>> >>> Then this skb is forwarded into an interface where EDT is taken into >>> consideration by either a qdisc or a device. >>> >>> Since CLOCK_TAI is a different base than CLOCK_REALTIME, we might have a problem. >> >> >> I'm not sure we have a problem here. For the Tx path I only see >> net_timestamp_set() being called from dev_queue_xmit_nit(). And even there, it's >> a clone of the skb that gets timestamped. >> >> I believe the original skb, which had the valid txtime copied into skb->tstamp, >> is not modified anywhere along that path. >> >> What am I missing, please? >> >> Thanks, >> Jesus >> > > > I am simply stating that a linux router, receiving packet on ethX and forwarding > them on ethY, could have a problem if ethY has a qdisc looking at skb->tstamp > assuming a timestamp in CLOCK_TAI base. > > In this case, skb->tstamp would have been set at ingress (not using CLOCK_TAI > but CLOCK_REALTIME), and would be read at egress (assuming CLOCK_TAI) > > Normal IPV4 routing path would be in net/ipv4/ip_forward.c, no scrubbing ever happens, > and no cloning either. > > Your patch (Clear skb->tstamp only on the forwarding path) is not handling the > typical forward path, only the cases where 'scrubbing' is used. Thanks for the clarification, I wasn't following you before. I believe we're fine with what we have *today*, because the qdisc will drop skbs that don't have a valid skb->sk, or if the SO_TXTIME flags is not set. This is a problem, however, if another qdisc is developed tomorrow and uses the same information but do not perform the same checks. Or, if the packet somehow gets to the controller and the HW has the "launch time" feature enabled and the driver uses the tstamp information the same way we do. If we want to protect from that now, I think we should go with the most immediate solution and clear skb->tstamp somewhere in ip_forward(), as you've pointed before. It seems to me this wouldn't break any other feature that might be re-using the tstamp information. This is also a more correct fix for the problem you raised before: >>> - We receive a packet. >>> - netstamp_wanted is >0 (because at least one packet capture is active) >>> - __net_timestamp() is called and does : >>> skb->tstamp = ktime_get_real(); >>> >>> Then this skb is forwarded into an interface where EDT is taken into >>> consideration by either a qdisc or a device. IMHO this Rx tstamp should never be forwarded to the Tx path as a valid txtime, regardless if the time base used was UTC or TAI. What do you think? Jesus > > > >> >> >>> >>> >>> Solutions for this problem : >>> >>> 1) Convert all our skb->tstamp usages to CLOCK_TAI base. >>> >>> or >>> >>> 2) clear skb->tstamp in forwarding paths, including the ones not scrubbing the packet. >>> >>> My preference is 1), even if it is a bit more work. >>>
In my dreamworld, a packet with a timestamp achieved at rx time on ethX, or via local traffic, would be consistent with the right clock throughout the system and reliably still be there when it goes to ethY. This would save having to timestamp (again) inside the cb block in fq_codel, cake, etc, and more importantly, measure total system delay from entrance to egress, (e.g tc filters, firewall rules, routing table lookups), not just queuing delay at the qdisc, thus detecting when a system was overloaded and reducing queue size and throughput appropriately as a result to cope.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 1357f36c8a5e..c4e24ac27464 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); */ void skb_scrub_packet(struct sk_buff *skb, bool xnet) { - skb->tstamp = 0; skb->pkt_type = PACKET_HOST; skb->skb_iif = 0; skb->ignore_df = 0; @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) ipvs_reset(skb); skb->mark = 0; + skb->tstamp = 0; } EXPORT_SYMBOL_GPL(skb_scrub_packet);
This is done in preparation for the upcoming time based transmission patchset. Now that skb->tstamp will be used to hold packet's txtime, we must ensure that it is being cleared when traversing namespaces. Also, doing that from skb_scrub_packet() before the early return would break our feature when tunnels are used. Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com> --- net/core/skbuff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)