Re: [PATCH NET-NEXT 0/10] hardware time stamping with new fields in shinfo

[Patrick Ohly <patrick.ohly@intel.com>]

On Mon, 2009-02-16 at 09:16 +0200, David Miller wrote:
> Ok, I applied everything and pushed it out to net-next-2.6, let's
> see how this goes :-)

Thanks :-)

> That TX clone wrt. skb_orphan() issue will need a happier solution.
> 
> Can you describe that problem in detail?  Maybe someone can come
> up with a way to avoid that stuff.

Bouncing information about a sent packet back to the sender (in
skb_tstamp_tx()) requires access to the socket via which the packet was
sent (orig_skb->sk).

This information must be available after the packet was handed to
ops->ndo_start_xmit() in order to implement the TX time stamping
software fallback for devices which don't implement hardware time
stamping (not enabled and/or not implemented at all).

The problem now is that some device drivers call skb_orphan() on the skb
in ops->ndo_start_xmit(). I suppose the intention is to free some
resources a bit earlier. The unmodified igb driver did this, the bnx2
one didn't.

If skb_orphan() is called, then TX software time stamping is not
possible. A user space daemon cannot detect this reliably, only make a
guess based on the observation that it doesn't receive any TX time
stamps. This is not fatal: the traditional time stamping method used by
PTPd (multicast loop back) still works.

But it would be nicer if that multicast loop back hack wasn't necessary.
TX software time stamping might also be more accurate (it generates the
time stamp after ops->ndo_start_xmit() returns, not before as in the
looped packet code path).

I see several ways to solve this:
      * Always create another reference to skb->sk before
        ops->ndo_start_xmit() if TX time stamping is requested.
        Drawback: performance penalty for drivers which support TX time
        stamping or don't call skb_orphan().
      * Turn skb_orphan() into a nop during ops->ndo_start_xmit(), again
        only if necessary. I think this can done by temporarily
        overriding the skb destructor. Drawback: looks like a hack to
        me, not sure whether that is really safe.
      * Extend the driver API + another reference. Let drivers which
        implement TX time stamping (and thus know when to avoid
        skb_orphan()) signal that. dev_hard_start_xmit() then can avoid
        the "additional sk reference" thing for these drivers. Drawback:
        requires analyzing and potentially flagging all drivers to have
        a real effect (as for bnx2).

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.