Date:	Wed, 12 Nov 2008 21:56:31 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Andi Kleen <ak@...ux.intel.com>
CC:	Oliver Hartkopp <oliver@...tkopp.net>,
	Patrick Ohly <patrick.ohly@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Octavian Purdila <opurdila@...acom.com>,
	Stephen Hemminger <shemminger@...tta.com>,
	Ingo Oeser <netdev@...eo.de>,
	"Ronciak, John" <john.ronciak@...el.com>
Subject: Re: [RFC PATCH 00/13] hardware time stamping + igb example	implementation

Andi Kleen a écrit :
> Eric Dumazet wrote:
>> Oliver Hartkopp a écrit :
>>> Patrick Ohly wrote:
>>>> On Wed, 2008-11-12 at 16:06 +0000, Andi Kleen wrote:
>>>>  
>>>>> As a general comment on the patch series I'm still a little sceptical
>>>>> the time stamp offset method is a good idea. Since it tries to 
>>>>> approximate
>>>>> several unsynchronized clocks the result will always be of a little 
>>>>> poor
>>>>> quality, which will likely lead to problems sooner or later (or rather
>>>>> require ugly workarounds in the user).
>>>>>
>>>>> I think it would be better to just bite the bullet and add new fields
>>>>> for this to the skbs. Hardware timestamps are useful enough to justify
>>>>> this.
>>>>>     
>>>>
>>>> I'm all for it, as long as it doesn't keep this feature out of the
>>>> mainline.
>>>>
>>>> At least one additional ktime_t field would be needed for the raw
>>>> hardware time stamp. Transformation to system time (as needed by PTP)
>>>> would have to be delayed until the packet is delivered via a socket. 
>>>> The
>>>> code would be easier (and a bit more accurate) if also another ktime_t
>>>> was added to store the transformed value directly after generating it.
>>>>
>>>> An extra field would also solve one of the open problems (tstamp set to
>>>> time stamp when dev_start_xmit_hard is called for IP_MULTICAST_LOOP).
>>>>
>>>>   
>>>
>>> I really wondered if you posted the series to get an impression why 
>>> adding a new field is a good idea ;-)
>>> Ok, i'm not that experienced on timestamps but i really got confused 
>>> reading the patches and it's documentation (even together with the 
>>> discussion on the ML). I would also vote for having a new field in 
>>> the skb instead of this current 'bit-compression' approach which 
>>> smells quite expensive at runtime and in code size. Not talking about 
>>> the mentioned potential locking issues ...
>>
>> New fields in skb are probably the easy way to handle the problem, we
>> all know that.
>>
>> But adding fields on such heavy duty structure for less than 0.001 % of
>> handled frames is disgusting.
> 
> You have a strange definition of "disgusting".
> 
> But if that's true that applies to the existing timestamp in there then too
> (and a couple of other fields in there too)
> 
> Also I suspect that your percent numbers are wrong, depending on the 
> workload.
> 
> Personally I think hardware time stamps should replace the existing
> time stamp and I suspect more and more applications will move to that 
> eventually.
> 

tstamp is the time stamp at the time NIC driver got the frame. Not the time
the NIC got the frame from the wire.

In about five years, maybe libpcap is updated to use timespec instead of timeval.

> 
>> If an application needs skb hw timestamp, get it when reading message, 
>> with appropriate
>> API, that calls NIC driver method, giving skb pointer as an argument. 
>> NIC driver
>> search in its local table a match of skb pointer (giving the most 
>> recent match of course),
>> and converts hwtimestamp into "generic application format". No need 
>> for a fast search, just
>> a linear search in the table, so that feeding it is really easy (maybe 
>> lockless)
> 
> This will probably be a disaster on e.g. high speed network sniffing
> (which is one of the primary use cases of the hardware
> As soon as there is any reordering in the queue (and that is inevitable
> if you scale over multiple CPUs) your linear searches could get quite
> long and bounce cache lines like mad. Also I doubt it can be really
> done lockless.
> 
> Also to be honest such a complicated and likely badly performing scheme 
> just to save 4-8 bytes
> would match my own definition of "disgusting".
> 

This scheme only is needed for special devices, used by PTP.

Only *selected* frames really need to gather hwtstamp.

TCP trafic wont use hwtstamp. Most UDP trafic wont use hwstamp.

I threw a "crazy idea", that can be changed if necessary, say with a cookie
that identifies the slot in NIC driver structure. O(1) lookup if really needed.

Yes, I find year 2008 not appropriate to enlarge skb with a hwstamp,
but then YMMV



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists