[PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

In commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in ingress
netem timestamps.), Jarek added a logic to refresh timestamps of
ingressed packets going through netem.

I believe we should generalize this, forcing a refresh of timestamps in
dev_queue_xmit_nit() for all ingress packets, whatever qdisc/class they
used before being delivered.

This way, we can have a good idea when packets are delivered to our
stack (tcpdump -i ifb0), while a tcpdump on original device gives
timestamps right before ingressing.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/dev.c        |    2 +-
 net/sched/sch_netem.c |    8 --------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d28b3a0..a2846f8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1506,7 +1506,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 	struct packet_type *ptype;
 
 #ifdef CONFIG_NET_CLS_ACT
-	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
+	if (!skb->tstamp.tv64 || (G_TC_FROM(skb->tc_verd) & AT_INGRESS))
 		net_timestamp_set(skb);
 #else
 	net_timestamp_set(skb);
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index e5593c0..1c29cc0 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -281,14 +281,6 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
 			if (unlikely(!skb))
 				return NULL;
 
-#ifdef CONFIG_NET_CLS_ACT
-			/*
-			 * If it's at ingress let's pretend the delay is
-			 * from the network (tstamp will be updated).
-			 */
-			if (G_TC_FROM(skb->tc_verd) & AT_INGRESS)
-				skb->tstamp.tv64 = 0;
-#endif
 			pr_debug("netem_dequeue: return skb=%p\n", skb);
 			sch->q.qlen--;
 			return skb;

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Wed, Dec 15, 2010 at 04:50:52PM +0100, Eric Dumazet wrote:
> In commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in ingress
> netem timestamps.), Jarek added a logic to refresh timestamps of
> ingressed packets going through netem.
> 
> I believe we should generalize this, forcing a refresh of timestamps in
> dev_queue_xmit_nit() for all ingress packets, whatever qdisc/class they
> used before being delivered.
> 
> This way, we can have a good idea when packets are delivered to our
> stack (tcpdump -i ifb0), while a tcpdump on original device gives
> timestamps right before ingressing.

I don't think we should do it. IMHO netem on ingress is a special case,
obviously for testing, and otherwise the real (first) timestamp might
be valuable for some users.

Jarek P.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/dev.c        |    2 +-
>  net/sched/sch_netem.c |    8 --------
>  2 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d28b3a0..a2846f8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1506,7 +1506,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  	struct packet_type *ptype;
>  
>  #ifdef CONFIG_NET_CLS_ACT
> -	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
> +	if (!skb->tstamp.tv64 || (G_TC_FROM(skb->tc_verd) & AT_INGRESS))
>  		net_timestamp_set(skb);
>  #else
>  	net_timestamp_set(skb);
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index e5593c0..1c29cc0 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -281,14 +281,6 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
>  			if (unlikely(!skb))
>  				return NULL;
>  
> -#ifdef CONFIG_NET_CLS_ACT
> -			/*
> -			 * If it's at ingress let's pretend the delay is
> -			 * from the network (tstamp will be updated).
> -			 */
> -			if (G_TC_FROM(skb->tc_verd) & AT_INGRESS)
> -				skb->tstamp.tv64 = 0;
> -#endif
>  			pr_debug("netem_dequeue: return skb=%p\n", skb);
>  			sch->q.qlen--;
>  			return skb;
> 
> 

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 22:17 +0100, Jarek Poplawski a écrit :
> On Wed, Dec 15, 2010 at 04:50:52PM +0100, Eric Dumazet wrote:
> > In commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in ingress
> > netem timestamps.), Jarek added a logic to refresh timestamps of
> > ingressed packets going through netem.
> > 
> > I believe we should generalize this, forcing a refresh of timestamps in
> > dev_queue_xmit_nit() for all ingress packets, whatever qdisc/class they
> > used before being delivered.
> > 
> > This way, we can have a good idea when packets are delivered to our
> > stack (tcpdump -i ifb0), while a tcpdump on original device gives
> > timestamps right before ingressing.
> 
> I don't think we should do it. IMHO netem on ingress is a special case,
> obviously for testing, and otherwise the real (first) timestamp might
> be valuable for some users.

Well, I find difficult to check sfq is actually correctly working
because timestamps are mixed.

After this patch, I found the SFQ allot error for example.

I dont know, I feel adding a sysctl like netdev_tstamp_prequeue is not
worth it...

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Thu, Dec 16, 2010 at 10:30:39PM +0100, Eric Dumazet wrote:
> Le jeudi 16 décembre 2010 ?? 22:17 +0100, Jarek Poplawski a écrit :
> > On Wed, Dec 15, 2010 at 04:50:52PM +0100, Eric Dumazet wrote:
> > > In commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in ingress
> > > netem timestamps.), Jarek added a logic to refresh timestamps of
> > > ingressed packets going through netem.
> > > 
> > > I believe we should generalize this, forcing a refresh of timestamps in
> > > dev_queue_xmit_nit() for all ingress packets, whatever qdisc/class they
> > > used before being delivered.
> > > 
> > > This way, we can have a good idea when packets are delivered to our
> > > stack (tcpdump -i ifb0), while a tcpdump on original device gives
> > > timestamps right before ingressing.
> > 
> > I don't think we should do it. IMHO netem on ingress is a special case,
> > obviously for testing, and otherwise the real (first) timestamp might
> > be valuable for some users.
> 
> Well, I find difficult to check sfq is actually correctly working
> because timestamps are mixed.
> 
> After this patch, I found the SFQ allot error for example.
> 
> I dont know, I feel adding a sysctl like netdev_tstamp_prequeue is not
> worth it...

Hmm... Do you expect more people start debugging SFQ or I missed your
point? ;-) Maybe such a change would be reasonable on a cloned skb?

Jarek P. 

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 23:08 +0100, Jarek Poplawski a écrit :

> Hmm... Do you expect more people start debugging SFQ or I missed your
> point? ;-) Maybe such a change would be reasonable on a cloned skb?

My point was to permit an admin to check his ingress shaping works or
not. In this respect, netem was a specialization of a general problem.

We had a prior discussion on timestamping skb in RX path, when RPS came
in : Should we take timestamp before RPS or after. At that time we added
a sysctl. Not sure it was the right choice :-(

I feel tcpdump (on TX side) should really display time of packet right
before being given to device, but this is probably a matter of taste.

Before commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in
ingress netem timestamps.), this is what was done.

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Thu, Dec 16, 2010 at 11:26:03PM +0100, Eric Dumazet wrote:
> Le jeudi 16 décembre 2010 ?? 23:08 +0100, Jarek Poplawski a écrit :
> 
> > Hmm... Do you expect more people start debugging SFQ or I missed your
> > point? ;-) Maybe such a change would be reasonable on a cloned skb?
> 
> My point was to permit an admin to check his ingress shaping works or
> not. In this respect, netem was a specialization of a general problem.
> 
> We had a prior discussion on timestamping skb in RX path, when RPS came
> in : Should we take timestamp before RPS or after. At that time we added
> a sysctl. Not sure it was the right choice :-(
> 
> I feel tcpdump (on TX side) should really display time of packet right
> before being given to device, but this is probably a matter of taste.

It might be reasonable unless it changes data for later users. That's
why I mentioned cloning.

> 
> Before commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in
> ingress netem timestamps.), this is what was done.

Then why don't you try to let turn it off in netem, where it only
matters?

Jarek P.

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

Le jeudi 16 décembre 2010 à 23:42 +0100, Jarek Poplawski a écrit :
> On Thu, Dec 16, 2010 at 11:26:03PM +0100, Eric Dumazet wrote:
> > Le jeudi 16 décembre 2010 ?? 23:08 +0100, Jarek Poplawski a écrit :
> > 
> > > Hmm... Do you expect more people start debugging SFQ or I missed your
> > > point? ;-) Maybe such a change would be reasonable on a cloned skb?
> > 
> > My point was to permit an admin to check his ingress shaping works or
> > not. In this respect, netem was a specialization of a general problem.
> > 
> > We had a prior discussion on timestamping skb in RX path, when RPS came
> > in : Should we take timestamp before RPS or after. At that time we added
> > a sysctl. Not sure it was the right choice :-(
> > 
> > I feel tcpdump (on TX side) should really display time of packet right
> > before being given to device, but this is probably a matter of taste.
> 
> It might be reasonable unless it changes data for later users. That's
> why I mentioned cloning.
> 
> > 
> > Before commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in
> > ingress netem timestamps.), this is what was done.
> 
> Then why don't you try to let turn it off in netem, where it only
> matters?
> 

Because, if you read again my patch, you'll see :

#ifdef CONFIG_NET_CLS_ACT
       if (!skb->tstamp.tv64 || (G_TC_FROM(skb->tc_verd) & AT_INGRESS))
                net_timestamp_set(skb);
#else

So : 

If we are handling an INGRESS packet, we force a timestamp renew.

Therefore :

We dont need in netem_dequeue() to force :

-#ifdef CONFIG_NET_CLS_ACT
-                       /*
-                        * If it's at ingress let's pretend the delay is
-                        * from the network (tstamp will be updated).
-                        */
-                       if (G_TC_FROM(skb->tc_verd) & AT_INGRESS)
-                               skb->tstamp.tv64 = 0;
-#endif

Since :

We are going to renew timestamp anyway.

Conclusion :

I am eliminating dead code.

Is that OK ?

Thanks

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Fri, Dec 17, 2010 at 12:31:45AM +0100, Eric Dumazet wrote:
> Le jeudi 16 décembre 2010 ?? 23:42 +0100, Jarek Poplawski a écrit :
> > On Thu, Dec 16, 2010 at 11:26:03PM +0100, Eric Dumazet wrote:
> > > Le jeudi 16 décembre 2010 ?? 23:08 +0100, Jarek Poplawski a écrit :
> > > 
> > > > Hmm... Do you expect more people start debugging SFQ or I missed your
> > > > point? ;-) Maybe such a change would be reasonable on a cloned skb?
> > > 
> > > My point was to permit an admin to check his ingress shaping works or
> > > not. In this respect, netem was a specialization of a general problem.
> > > 
> > > We had a prior discussion on timestamping skb in RX path, when RPS came
> > > in : Should we take timestamp before RPS or after. At that time we added
> > > a sysctl. Not sure it was the right choice :-(
> > > 
> > > I feel tcpdump (on TX side) should really display time of packet right
> > > before being given to device, but this is probably a matter of taste.
> > 
> > It might be reasonable unless it changes data for later users. That's
> > why I mentioned cloning.
> > 
> > > 
> > > Before commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in
> > > ingress netem timestamps.), this is what was done.
> > 
> > Then why don't you try to let turn it off in netem, where it only
> > matters?

Forget this advice - I lost the point how you tested that.

> Because, if you read again my patch, you'll see :
> 
> #ifdef CONFIG_NET_CLS_ACT
>        if (!skb->tstamp.tv64 || (G_TC_FROM(skb->tc_verd) & AT_INGRESS))
>                 net_timestamp_set(skb);
> #else
> 
> So : 
> 
> If we are handling an INGRESS packet, we force a timestamp renew.

It is wrong because it brings back the inconsistency with ping etc.
described by Alex Sidorenko in the changelog of netem patch, but
use normal (not netem) ingress scheduling (ping results shouldn't
depend on sniffers).

> 
> Therefore :
> 
> We dont need in netem_dequeue() to force :
> 
> -#ifdef CONFIG_NET_CLS_ACT
> -                       /*
> -                        * If it's at ingress let's pretend the delay is
> -                        * from the network (tstamp will be updated).
> -                        */
> -                       if (G_TC_FROM(skb->tc_verd) & AT_INGRESS)
> -                               skb->tstamp.tv64 = 0;
> -#endif
> 
> Since :
> 
> We are going to renew timestamp anyway.
> 
> Conclusion :
> 
> I am eliminating dead code.
> 
> Is that OK ?

Not to me.

Thanks,
Jarek P.

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

Le vendredi 17 décembre 2010 à 07:30 +0000, Jarek Poplawski a écrit :

> It is wrong because it brings back the inconsistency with ping etc.
> described by Alex Sidorenko in the changelog of netem patch, but
> use normal (not netem) ingress scheduling (ping results shouldn't
> depend on sniffers).

Care to explain why only netem should take care of this delaying
problem ?
If a packet is delayed on other qdisc, dont we have the same problem ?

Right now, ping lies, giving different results if a sniffer is active or
not.

Care to suggest an alternative patch, because I am lost at this point ?

Thanks

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Fri, Dec 17, 2010 at 09:08:17AM +0100, Eric Dumazet wrote:
> Le vendredi 17 décembre 2010 ?? 07:30 +0000, Jarek Poplawski a écrit :
> 
> > It is wrong because it brings back the inconsistency with ping etc.
> > described by Alex Sidorenko in the changelog of netem patch, but
> > use normal (not netem) ingress scheduling (ping results shouldn't
> > depend on sniffers).
> 
> Care to explain why only netem should take care of this delaying
> problem ?
> If a packet is delayed on other qdisc, dont we have the same problem ?

netem was treated as a special case just to pretend (lie) about the
net outside (still not depending on sniffers), but it's hard to
believe there are "normal" ping users after netem.

> 
> Right now, ping lies, giving different results if a sniffer is active or
> not.
> 
> Care to suggest an alternative patch, because I am lost at this point ?

Just what I wrote earlier: consider one additional cloning in
dev_queue_xmit_nit or maybe resetting the timestamp with act_skbedit?

Thanks,
Jarek P.

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Jarek Poplawski]

On Fri, Dec 17, 2010 at 08:34:13AM +0000, Jarek Poplawski wrote:
> On Fri, Dec 17, 2010 at 09:08:17AM +0100, Eric Dumazet wrote:
> > Care to suggest an alternative patch, because I am lost at this point ?
> 
> Just what I wrote earlier: consider one additional cloning in
> dev_queue_xmit_nit

Actually, it's enough to get time once and change it for all current
clones in the loop. 

Jarek P.

Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

[Eric Dumazet]

Le vendredi 17 décembre 2010 à 08:59 +0000, Jarek Poplawski a écrit :
> On Fri, Dec 17, 2010 at 08:34:13AM +0000, Jarek Poplawski wrote:
> > On Fri, Dec 17, 2010 at 09:08:17AM +0100, Eric Dumazet wrote:
> > > Care to suggest an alternative patch, because I am lost at this point ?
> > 
> > Just what I wrote earlier: consider one additional cloning in
> > dev_queue_xmit_nit
> 
> Actually, it's enough to get time once and change it for all current
> clones in the loop. 
> 
> Jarek P.

I think we can add this after latest Changli patch :

He does one skb_clone() before calling the sniffers.
We could set timestamp on this clone, instead of original skb.

Problem solved.

[PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit

[Eric Dumazet]

Le vendredi 17 décembre 2010 à 10:26 +0100, Eric Dumazet a écrit :

> 
> I think we can add this after latest Changli patch :
> 
> He does one skb_clone() before calling the sniffers.
> We could set timestamp on this clone, instead of original skb.
> 
> Problem solved.
> 

[PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit

Now we do one clone of skb if at least one sniffer might take packet,
we also can do the skb timestamping on the clone and let original packet
unchanged.

This is a generalization of commit 8caf153974f2 (net: sch_netem: Fix an
inconsistency in ingress netem timestamps.)

This way, we can have a good idea when packets are delivered to our
stack (tcpdump -i ifb0), while a tcpdump on original device gives
timestamps right before ingressing.

This also speedup our stack, avoiding taking timestamps if not needed.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jarek Poplawski <jarkao2@gmail.com>
---
 net/core/dev.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5987729..a215269 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1547,13 +1547,6 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 	struct sk_buff *skb2 = NULL;
 	struct packet_type *pt_prev = NULL;
 
-#ifdef CONFIG_NET_CLS_ACT
-	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
-		net_timestamp_set(skb);
-#else
-	net_timestamp_set(skb);
-#endif
-
 	rcu_read_lock();
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
 		/* Never send packets back to the socket
@@ -1572,6 +1565,8 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 			if (!skb2)
 				break;
 
+			net_timestamp_set(skb2);
+
 			/* skb->nh should be correctly
 			   set by sender, so that the second statement is
 			   just protection against buggy protocols.

Re: [PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit

[Changli Gao]

On Tue, Dec 21, 2010 at 3:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 17 décembre 2010 à 10:26 +0100, Eric Dumazet a écrit :
>
>>
>> I think we can add this after latest Changli patch :
>>
>> He does one skb_clone() before calling the sniffers.
>> We could set timestamp on this clone, instead of original skb.
>>
>> Problem solved.
>>
>
> [PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit
>
> Now we do one clone of skb if at least one sniffer might take packet,
> we also can do the skb timestamping on the clone and let original packet
> unchanged.
>
> This is a generalization of commit 8caf153974f2 (net: sch_netem: Fix an
> inconsistency in ingress netem timestamps.)
>
> This way, we can have a good idea when packets are delivered to our
> stack (tcpdump -i ifb0), while a tcpdump on original device gives
> timestamps right before ingressing.
>
> This also speedup our stack, avoiding taking timestamps if not needed.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Jarek Poplawski <jarkao2@gmail.com>

Acked-by: Changli Gao <xiaosuo@gmail.com>


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit

[David Miller]

From: Changli Gao <xiaosuo@gmail.com>
Date: Tue, 21 Dec 2010 15:56:17 +0800

> On Tue, Dec 21, 2010 at 3:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Le vendredi 17 décembre 2010 à 10:26 +0100, Eric Dumazet a écrit :
>> [PATCH net-next-2.6] net: timestamp cloned packet in dev_queue_xmit_nit
>>
>> Now we do one clone of skb if at least one sniffer might take packet,
>> we also can do the skb timestamping on the clone and let original packet
>> unchanged.
>>
>> This is a generalization of commit 8caf153974f2 (net: sch_netem: Fix an
>> inconsistency in ingress netem timestamps.)
>>
>> This way, we can have a good idea when packets are delivered to our
>> stack (tcpdump -i ifb0), while a tcpdump on original device gives
>> timestamps right before ingressing.
>>
>> This also speedup our stack, avoiding taking timestamps if not needed.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Cc: Changli Gao <xiaosuo@gmail.com>
>> Cc: Patrick McHardy <kaber@trash.net>
>> Cc: Jarek Poplawski <jarkao2@gmail.com>
> 
> Acked-by: Changli Gao <xiaosuo@gmail.com>

Applied, thanks.