Eric Dumazet wrote:
quoted text
> 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.


quoted text
> 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".

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