Message-ID: <20090924134859.GA19787@hmsreliant.think-freely.org>
Date:	Thu, 24 Sep 2009 09:48:59 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] af_packet: add interframe drop cmsg (v2)

On Thu, Sep 24, 2009 at 07:24:47AM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> >    
> > Ok, Version 2 of this patch, Taking Erics change notes into account.  Again,
> > tested by me, using the same test harness, and it works fine.
> > Neil
> >  
> > 
> >     Add Ancilliary data to better represent loss information
> >     
> >     I've had a few requests recently to provide more detail regarding frame loss
> >     during an AF_PACKET packet capture session.  Specifically the requestors want to
> >     see where in a packet sequence frames were lost, i.e. they want to see that 40
> >     frames were lost between frames 302 and 303 in a packet capture file.  In order
> >     to do this we need:
> >     
> >     1) The kernel to export this data to user space
> >     2) The applications to make use of it
> >     
> >     This patch addresses item (1).  It does this by doing the following:
> >     
> >     A) attaching ancilliary data to any skb enqueued to a socket recieve queue for
> >     which frames were lost between it and the previously enqueued frame.  Note I use
> >     a ring buffer with a correlator value (the skb pointer) to do this.  This was
> >     done because the skb->cb array is exhausted already for AF_PACKET
> 
> Yes, skb->cb array is fully used... Hmm...
> 
> As we do skb_dst_drop(skb) before queueing skb into sk_receive_queue,
> we could overide _skb_dst to store the 'gap', (but would need 
> a special skb_queue_purge() implementation to ignore _skb_dst or force it to 0)
> 
Yeah, we could do that, but I really hate the idea of overriding fields in an
skb.  I think that leads to so much confusion should someone have a need for
that fields legitimate purpose in the future.  I was thinking about perhaps
changing origlen to be a u16, and making gap a u16 so that we could fit both
fields into skb->cb. It would imply that we would loose info if any gap exceeded
65535 packets, or if the origional length of an skb exceeded 65535 bytes.  I
think both conditions are highly unlikely.  We could mitigate the gap overflow
by checking the gap counter on increment and letting the 0xffff value be a
special 'more than 65k frames lost' meaning. We could perhaps do something
simmilar with the origional length.  Thoughts?

> Problem with your implementation is that the 'ring buffer' could be exhausted as well
> and user could miss some information. (not counting you didnt rate limit the error message ;) )
> 
Yeah, I'm not a fan of that either.  As I write it down above, I'm liking
splitting the origlen value into two 16 bit values more and more.  Let me know
what you think, and I'll make the appropriate changes.

Thanks!
Neil

P.S. I was thinking about your mmap comments last night, and I recalled that
Arnaldo was pushing some patches to allows for multiple skb receive and send
operations with a single syscall.  I'm wondering if the judicious use of such a
syscall might mitigate the performance advantage of mmap for libpcap?


> >     
> >     B) For any frame dequeued that has ancilliary data in the ring buffer (as
> >     determined by the correlator value), we add a cmsg structure to the msghdr that
> >     gets copied to user space, this cmsg structure is of cmsg_level AF_PACKET, and
> >     cmsg_type PACKET_GAPDATA.  It contains a u32 value which counts the number of
> >     frames lost between the reception of the frame being currently recevied and the
> >     frame most recently preceding it.  Note this creates a situation in which if we
> >     have packet loss followed immediately by a socket close operation we might miss
> >     some gap information.  This situation is covered by the use of the
> >     PACKET_AUXINFO socket option, which provides total loss stats (from which the
> >     final gap can be computed).
> >     
> >     I've tested this patch myself, and it works well.
> >     
> >     Signed-off-by: Neil Horman <nhorman@...driver.com>
> > 
> 
> 
--
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