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

[Jarek Poplawski <jarkao2@gmail.com>]

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.