> On Feb 19, 2016, at 4:40 PM, Ben Pfaff <[email protected]> wrote:
>
>
> --- a/ovn/TODO
> +++ b/ovn/TODO
> @@ -4,18 +4,11 @@
>
> ** New OVN logical actions
>
>
> +*** rate_limit
>
> TCP/IP stacks typically limit the rate at which ARPs are sent, e.g. to
> one per second for a given target. We might need to do this too.
>
> ....
> +*** Ratelimiting.
>
> +From casual observation, Linux appears to generate at most one ARP per
> +second per destination.
It seems like this is saying similar things in two spots.
> *** Renewal and expiration.
>
> Something needs to make sure that bindings remain valid and expire
> those that become stale.
As we discussed in-person, I wonder if we can introduce the concept of time
into the database. Either the database itself could have time-based limits for
columns or we could provide the ability to define queries based on relative.
For example, ovn-northd could regularly send a delete query that matches any
row with an age older than five seconds.
> +/* Adds a flow to table */
> +static void
> +add_neighbor_flows(struct controller_ctx *ctx,
> + const struct lport_index *lports, struct hmap *flow_table)
I think the comment describing this function didn't get finished.
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 70088f6..2261475 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
>
> @@ -137,6 +161,10 @@ pinctrl_handle_arp(const struct flow *ip_flow, struct
> ofpbuf *userdata)
> goto exit;
> }
>
> + struct ds s = DS_EMPTY_INITIALIZER;
> + ofpacts_format(ofpacts.data, ofpacts.size, &s);
> + ds_destroy(&s);
Is this doing anything useful?
> @@ -183,21 +212,24 @@ process_packet_in(const struct ofp_header *msg)
> struct flow headers;
> flow_extract(&packet, &headers);
>
> - const struct flow *md = &pin.flow_metadata.flow;
> switch (ntohl(ah->opcode)) {
> ...
> default:
> - VLOG_WARN_RL(&rl, "unrecognized packet-in command %#"PRIx32,
> - md->regs[0]);
> + VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
> ah->opcode);
Does "ah->opcode" need an ntohl() around it?
> +static void
> +pinctrl_handle_put_arp(const struct lport_index *lports,
> + const struct flow *md, const struct flow *headers)
> +{
> + if (n_put_arps >= ARRAY_SIZE(put_arps)) {
> + return;
> + }
I don't think we've defined any coverage counters for OVN yet, but I wonder if
we should start doing that. These sorts of silent errors might be good
candidates.
> ...
> + struct put_arp *pa = &put_arps[n_put_arps++];
> + pa->timestamp = time_msec();
> + pa->logical_port = xstrdup(pb->logical_port);
> + pa->ip = htonl(md->regs[0]);
> + pa->mac = headers->dl_src;
This code just uses and array to store the requests, but I worry that if there
are too many ARP replies that this queue could overrun. What about using a
hash table instead? In the case of multiple identical put_arps, that approach
would also lessen the impact of the fairly expensive loop in run_put_arps()
that looks for duplicates.
> +static void
> +put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
> + struct ofpbuf *ofpacts)
> +{
> + struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
> + sf->field = mf_from_id(dst);
> + sf->flow_has_vlan = false;
> +
> + ovs_be64 n_value = htonll(value);
> + bitwise_copy(&n_value, 8, 0, &sf->value, sf->field->n_bytes, ofs,
> n_bits);
> + bitwise_one(&sf->mask, sf->field->n_bytes, ofs, n_bits);
> +}
We discussed this off-line, but there are three put_load() functions in the
"ovn" directory--two of which have identical implementations. Similarly
there's an emit_resubmit() in "ovn/lib/actions.c" and a put_resubmit() in
"ovn/controller/physical.c" with identical implementations but slightly
different arguments. I wonder if it would make sense to start putting these
into a library.
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 1b2912e..4685143 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
>
>
>
> @@ -602,64 +617,73 @@ icmp4 {
> <ul>
> <li>
> <p>
> - Known MAC bindings. For each IP address <var>A</var> whose host is
> - known to have Ethernet address <var>HE</var> and reside on router
> - port <var>P</var> with Ethernet address <var>PE</var>, a
> priority-200
> - flow with match <code>reg0 == <var>A</var></code> has the following
> - actions:
> + Static MAC bindings. MAC bindings can be known statically based on
> + data in the <code>OVN_Northbound</code> database. For router ports
> + connected to logical switches, MAC bindings can be known statically
> + from the <code>addresses</code> column in the
> + <code>Logical_Port</code> table. For router ports connected to
> other
> + logical routers, MAC bindings can be known statically from the
I wonder if it would be clearer to say "For routed ports" instead of "For
router ports" in these two sentence, since they're different from "logical
router ports" but sound similar.
> + Dynamic MAC bindings. This flows resolves MAC-to-IP bindings that
> + have become known dynamically through ARP. (The next table will
> + issue an ARP request for cases where the binding is not yet known.)
> + </p>
It might be nice to point out that the packet that is being resolved is
effectively dropped.
> + Unknown MAC address. A priority-100 flow with match <code>eth.dst
> ==
> + 00:00:00:00:00:00</code> has the following actions:
> </p>
>
> <pre>
> +rate_limit(outport, ip4.dst);
It feels weird to have an action listed here that we don't support, but it's
nice to have a reference where it's needed. Maybe leave it here, but then add
a note at the bottom that it still needs to be implemented?
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index e6271cf..d511b4d 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
>
> @@ -389,17 +391,18 @@ join_datapaths(struct northd_context *ctx, struct hmap
> *datapaths,
> ...
> + /* Set the gateway port to NULL. If there is a gateway, it will get
> + * filled in as we go through the ports later. */
> + od->gateway_port = NULL;
I think this requires that build_datapaths() be called before build_ports().
The code does that, but do you think it's worth mentioning that in comments
describing the functions?
Thanks for working on this--it's pretty cool!
Acked-by: Justin Pettit <[email protected]>
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
