On Wed, Dec 09, 2015 at 05:57:47PM -0800, Justin Pettit wrote: > > > On Dec 8, 2015, at 5:08 PM, Ben Pfaff <b...@ovn.org> wrote: > > > > --- a/NEWS > > +++ b/NEWS > > @@ -1,5 +1,7 @@ > > Post-v2.5.0 > > --------------------- > > + - OpenFlow: > > + * New "arp" OpenFlow extension action for generating ARP packets. > > Once you have a design for packet injection from a controller, I think > we should revisit this patch. My initial reaction is that I'd prefer > to not introduce changes to OVS unless they're really needed, and it's > not clear to me whether that's the case here. We're going to be > handling the IPv6 equivalent from the controller, so there'd be some > parity.
That's true. > > + * @OVS_ACTION_ATTR_ARP: Transform IPv4 packet into ARP packet, execute > > nested > > + * actions, restore IPv4 packet. > > Another advantage of initiating the ARP in the controller is that we > can buffer the packet, since this means that an ARP miss will always > require a retransmission of the original packet. We'd also have the > ability to rate-limit ARP generation from the controller. I want to equivocate here. Rate-limiting always works better the closer you get to the source. If we rate-limit at the controller, then we're not rate-limiting the packets sent to the controller by the switch. That makes it hard to achieve fairness. That is, if we send all the packets that need an ARP to the controller, and there's a big stream of those that the controller is going to drop, and some packet slips in that won't be dropped, then the socket buffer between the switch and controller is what's actually going to drop them, and statistically the fairness there isn't going to be good. Therefore, I don't think we should rate-limit from the controller. We do have some mechanisms to rate-limit from the switch. One way we can do it today is to "learn" a flow that drops packets with a suitable (e.g. 1-second) hard timeout; I don't know whether this is the best way though. I don't think that ARP generation from the controller is mutually exclusive with buffering packets. > > +/* Parses 'arg' as the argument to a "arp" action, and appends such an > > + * action to 'ofpacts'. > > s/a "arp"/an "arp"/ Thanks, fixed. > > --- a/lib/packets.h > > +++ b/lib/packets.h > > @@ -1041,6 +1041,7 @@ void packet_set_nd(struct dp_packet *, const ovs_be32 > > target[4], > > > > void packet_format_tcp_flags(struct ds *, uint16_t); > > const char *packet_tcp_flag_to_string(uint32_t flag); > > +void compose_arp__(struct dp_packet *); > > Somehow it seems kind of strange to have a "__" function defined in a > header file for external users. Sorry. > Acked-by: Justin Pettit <jpet...@ovn.org> I'm going to hold off on pushing this one for the moment. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
