[ovs-dev] [PATCH 1/2] OVN: add buffering support for ipv4 packets

[Ben Pfaff]

On Wed, Sep 26, 2018 at 03:07:52PM -0700, Ben Pfaff wrote:
> On Fri, Sep 14, 2018 at 05:19:24PM +0200, Lorenzo Bianconi wrote:
> > Add buffering support for IPv4 packets that will be processed
> > by arp {} action when L2 address is not discovered yet since
> > otherwise the packet will be substituted with an ARP frame and
> > this will result in the lost of the first packet of the connection
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> 
> Thanks for working on this!  I believe that users will find it useful.
> 
> A buffer queue depth of 64 sounds big to me.  Linux appears to default
> to a depth of 3 (see unres_qlen in arp(7)).
> 
> A 30-second timeout seems long to me.  After that long, the sender will
> have retransmitted the packet, probably multiple times.  I wasn't able
> to figure out how long Linux buffers packets.
> 
> Do you think it makes sense to use the same timestamp for all of the
> packets buffered for a single IP?  I believe that, currently, no packets
> will ever expire from a buffer unless either the buffer limit is
> exceeded or no new packets have been queued for the timeout interval.  I
> think that would mean that there could be a packet buffered for a total
> of 30 + 64 seconds.
> 
> It seems odd to me to convert the IP address to a string then use that
> as the key.  Why not the binary form of the IP address?  I see that the
> following patch adds IPv6 support, which could be one reason, but in
> many places in OVS we use IPv6-mapped IPv4 addresses in places where
> IPv4 and IPv6 are both possible (see in6_addr_mapped_ipv4() and
> in6_addr_get_mapped_ipv4(), for example).
> 
> In the buffered_send_packets() prototype, I would use struct eth_addr
> instead of an unsigned char *.

One more thing: I recommend making "head" and "tail" unsigned int so
that it is obvious that taking % BUFFER_QUEUE_DEPTH is efficient.