From: Eric Dumazet <eric.dumazet@gmail•com>
To: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel•com>,
Eric Dumazet <eric.dumazet@gmail•com>,
netdev@vger•kernel.org
Cc: tglx@linutronix•de, jan.altenberg@linutronix•de,
vinicius.gomes@intel•com, kurt.kanzenbach@linutronix•de,
henrik@austad•us, richardcochran@gmail•com,
ilias.apalodimas@linaro•org, ivan.khoronzhuk@linaro•org,
mlichvar@redhat•com, willemb@google•com, jhs@mojatatu•com,
xiyou.wangcong@gmail•com, jiri@resnulli•us,
jeffrey.t.kirsher@intel•com
Subject: Re: [PATCH v2 net-next 01/14] net: Clear skb->tstamp only on the forwarding path
Date: Mon, 16 Jul 2018 16:15:28 -0700 [thread overview]
Message-ID: <463024c6-e085-ea88-688b-b42df679876a@gmail.com> (raw)
In-Reply-To: <aab83276-c58b-4cfe-d44d-5271cd5163a6@intel.com>
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.
>>
next prev parent reply other threads:[~2018-07-16 23:45 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-03 22:42 [PATCH v2 net-next 00/14] Scheduled packet Transmission: ETF Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 01/14] net: Clear skb->tstamp only on the forwarding path Jesus Sanchez-Palencia
2018-07-13 17:35 ` Eric Dumazet
2018-07-16 21:52 ` Jesus Sanchez-Palencia
2018-07-16 23:15 ` Eric Dumazet [this message]
2018-07-18 18:19 ` Jesus Sanchez-Palencia
2018-07-18 18:40 ` Dave Taht
2018-07-03 22:42 ` [PATCH v2 net-next 02/14] net: Add a new socket option for a future transmit time Jesus Sanchez-Palencia
2018-07-08 0:44 ` Eric Dumazet
2018-07-09 22:21 ` Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 03/14] net: ipv4: Hook into time based transmission Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 04/14] net: ipv6: " Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 05/14] net: packet: " Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 06/14] net/sched: Allow creating a Qdisc watchdog with other clocks Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 07/14] net/sched: Introduce the ETF Qdisc Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 08/14] net/sched: Add HW offloading capability to ETF Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 09/14] igb: Refactor igb_configure_cbs() Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 10/14] igb: Only change Tx arbitration when CBS is on Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 11/14] igb: Refactor igb_offload_cbs() Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 12/14] igb: Only call skb_tx_timestamp after descriptors are ready Jesus Sanchez-Palencia
2018-07-03 22:42 ` [PATCH v2 net-next 13/14] igb: Add support for ETF offload Jesus Sanchez-Palencia
2018-07-03 22:43 ` [PATCH v2 net-next 14/14] net/sched: Make etf report drops on error_queue Jesus Sanchez-Palencia
2018-07-04 13:37 ` [PATCH v2 net-next 00/14] Scheduled packet Transmission: ETF David Miller
2018-07-06 21:38 ` Stephen Hemminger
2018-07-09 15:24 ` Jesus Sanchez-Palencia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=463024c6-e085-ea88-688b-b42df679876a@gmail.com \
--to=eric.dumazet@gmail$(echo .)com \
--cc=henrik@austad$(echo .)us \
--cc=ilias.apalodimas@linaro$(echo .)org \
--cc=ivan.khoronzhuk@linaro$(echo .)org \
--cc=jan.altenberg@linutronix$(echo .)de \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=jesus.sanchez-palencia@intel$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=kurt.kanzenbach@linutronix$(echo .)de \
--cc=mlichvar@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=richardcochran@gmail$(echo .)com \
--cc=tglx@linutronix$(echo .)de \
--cc=vinicius.gomes@intel$(echo .)com \
--cc=willemb@google$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox