On Mon, Nov 20, 2017 at 05:10:53PM -0600, Mark Michelson wrote:
> This change adds three new options to the Northbound
> Logical_Router_Port's ipv6_ra_configs option:
>
> * send_periodic: If set to "true", then OVN will send periodic router
> advertisements out of this router port.
> * max_interval: The maximum amount of time to wait between sending
> periodic router advertisements.
> * min_interval: The minimum amount of time to wait between sending
> periodic router advertisements.
>
> When send_periodic is true, then IPv6 RA configs, as well as some layer
> 2 and layer 3 information about the router port, are copied to the
> southbound database. From there, ovn-controller can use this information
> to know when to send periodic RAs and what to send in them.
>
> Because periodic RAs originate from each ovn-controller, the new
> keep-local flag is set on the packet so that ports don't receive an
> overabundance of RAs.
>
> Signed-off-by: Mark Michelson <[email protected]>
Thank you for working on this feature. It looks pretty good! I have a
few comments to help polish it.
It seems odd that struct ipv6_ra_config keeps mtu in network byte
order. I guess that it is because compose_nd_ra() accepts it in network
byte order, but that seems odd too. Maybe both of them should be
changed.
The following code is risky because, if NDEBUG is defined, the fields
will not be initialized:
ovs_assert(eth_addr_from_string("33:33:00:00:00:01", &config->eth_dst));
ovs_assert(ipv6_parse("ff02::1", &config->ipv6_dst));
I guess that the eth_addr_from_string() call is because it's difficult
to write a "struct eth_addr" constant. We can introduce a new macro to
help with that:
https://patchwork.ozlabs.org/patch/842364/
Once we've done that, then it's easier:
config->eth_dst = ETH_ADDR_C(33,33,00,00,00,01);
I have a number of other minor suggestions, most of them trivial style
or documentation related. I think it's easiest to just express them as
an incremental diff. I'm appending it below. If they do not make sense
to you, or you object, you can ignore them, otherwise please fold them
into v6.
Thanks again!
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index a417841af1de..a7c786b3eeda 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -92,8 +92,8 @@ static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
static void init_ipv6_ras(void);
static void destroy_ipv6_ras(void);
static void ipv6_ra_wait(void);
-static void send_ipv6_ras(const struct controller_ctx *ctx,
- struct hmap *local_datapaths);
+static void send_ipv6_ras(const struct controller_ctx *,
+ struct hmap *local_datapaths);
COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
@@ -1132,8 +1132,8 @@ ipv6_ra_config_delete(struct ipv6_ra_config *config)
{
if (config) {
destroy_lport_addresses(&config->prefixes);
+ free(config);
}
- free(config);
}
static void
@@ -1141,8 +1141,8 @@ ipv6_ra_delete(struct ipv6_ra_state *ra)
{
if (ra) {
ipv6_ra_config_delete(ra->config);
+ free(ra);
}
- free(ra);
}
static void
@@ -1219,8 +1219,8 @@ fail:
static long long int
ipv6_ra_calc_next_announce(time_t min_interval, time_t max_interval)
{
- long long int min_interval_ms = min_interval * 1000;
- long long int max_interval_ms = max_interval * 1000;
+ long long int min_interval_ms = min_interval * 1000LL;
+ long long int max_interval_ms = max_interval * 1000LL;
return time_msec() + min_interval_ms +
random_range(max_interval_ms - min_interval_ms);
@@ -1238,7 +1238,7 @@ put_load(uint64_t value, enum mf_field_id dst, int ofs,
int n_bits,
bitwise_one(ofpact_set_field_mask(sf), sf->field->n_bytes, ofs, n_bits);
}
-static time_t
+static long long int
ipv6_ra_send(struct ipv6_ra_state *ra)
{
if (time_msec() < ra->next_announce) {
@@ -1252,7 +1252,7 @@ ipv6_ra_send(struct ipv6_ra_state *ra)
&ra->config->ipv6_src, &ra->config->ipv6_dst,
255, ra->config->mo_flags, 0, 0, 0, ra->config->mtu);
- for (int i = 0; i < ra->config->prefixes.n_ipv6_addrs; ++i) {
+ for (int i = 0; i < ra->config->prefixes.n_ipv6_addrs; i++) {
ovs_be128 addr;
memcpy(&addr, &ra->config->prefixes.ipv6_addrs[i].addr, sizeof addr);
packet_put_ra_prefix_opt(&packet,
@@ -1263,7 +1263,6 @@ ipv6_ra_send(struct ipv6_ra_state *ra)
uint64_t ofpacts_stub[4096 / 8];
struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
- enum ofp_version version = rconn_get_version(swconn);
/* Set MFF_LOG_DATAPATH and MFF_LOG_INPORT. */
uint32_t dp_key = ra->metadata;
@@ -1284,6 +1283,7 @@ ipv6_ra_send(struct ipv6_ra_state *ra)
};
match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER);
+ enum ofp_version version = rconn_get_version(swconn);
enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
queue_msg(ofputil_encode_packet_out(&po, proto));
dp_packet_uninit(&packet);
@@ -1302,8 +1302,7 @@ ipv6_ra_wait(void)
}
static void
-send_ipv6_ras(const struct controller_ctx *ctx OVS_UNUSED,
- struct hmap *local_datapaths)
+send_ipv6_ras(const struct controller_ctx *ctx, struct hmap *local_datapaths)
{
struct shash_node *iter, *iter_next;
@@ -1316,7 +1315,6 @@ send_ipv6_ras(const struct controller_ctx *ctx OVS_UNUSED,
const struct local_datapath *ld;
HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
-
struct sbrec_port_binding *lpval;
const struct sbrec_port_binding *pb;
struct ovsdb_idl_index_cursor cursor;
@@ -1331,32 +1329,30 @@ send_ipv6_ras(const struct controller_ctx *ctx
OVS_UNUSED,
continue;
}
- const char *peer_s;
- peer_s = smap_get(&pb->options, "peer");
+ const char *peer_s = smap_get(&pb->options, "peer");
if (!peer_s) {
continue;
}
- const struct sbrec_port_binding *peer;
- peer = lport_lookup_by_name(ctx->ovnsb_idl, peer_s);
+ const struct sbrec_port_binding *peer
+ = lport_lookup_by_name(ctx->ovnsb_idl, peer_s);
if (!peer) {
continue;
}
- struct ipv6_ra_config *config;
- config = ipv6_ra_update_config(pb);
+ struct ipv6_ra_config *config = ipv6_ra_update_config(pb);
if (!config) {
continue;
}
- struct ipv6_ra_state *ra;
- ra = shash_find_data(&ipv6_ras, pb->logical_port);
+ struct ipv6_ra_state *ra
+ = shash_find_data(&ipv6_ras, pb->logical_port);
if (!ra) {
ra = xzalloc(sizeof *ra);
ra->config = config;
ra->next_announce = ipv6_ra_calc_next_announce(
- ra->config->min_interval,
- ra->config->max_interval);
+ ra->config->min_interval,
+ ra->config->max_interval);
shash_add(&ipv6_ras, pb->logical_port, ra);
} else {
ipv6_ra_config_delete(ra->config);
@@ -1371,8 +1367,7 @@ send_ipv6_ras(const struct controller_ctx *ctx OVS_UNUSED,
ra->metadata = peer->datapath->tunnel_key;
ra->delete_me = false;
- long long int next_ra;
- next_ra = ipv6_ra_send(ra);
+ long long int next_ra = ipv6_ra_send(ra);
if (send_ipv6_ra_time > next_ra) {
send_ipv6_ra_time = next_ra;
}
@@ -1389,7 +1384,6 @@ send_ipv6_ras(const struct controller_ctx *ctx OVS_UNUSED,
}
}
-
void
pinctrl_wait(struct controller_ctx *ctx)
{
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 85a380254020..20166becb909 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -1385,22 +1385,23 @@
</column>
<column name="ipv6_ra_configs" key="send_periodic">
- Should the router port send periodic router advertisements? If set to
- true, then this router interface will send router advertisements
- out periodically. The default is false.
+ If set to true, then this router interface will send router
+ advertisements periodically. The default is false.
</column>
<column name="ipv6_ra_configs" key="max_interval">
The maximum number of seconds to wait between sending periodic router
- advertisements. This option has no effect if the "send_periodic" value
- is set to false. The default is 600.
+ advertisements. This option has no effect if <ref
+ column="ipv6_ra_configs" key="send_periodic"/> is false. The default
+ is 600.
</column>
<column name="ipv6_ra_configs" key="min_interval">
The minimum number of seconds to wait between sending periodic router
- advertisements. This option has no effect if the "send_periodic" value
- is set to "false". The default is 0.33 * max_interval. If max_interval
- is set to its default, then min_interval will be 200.
+ advertisements. This option has no effect if <ref
+ column="ipv6_ra_configs" key="send_periodic"/> is false. The default
+ is one-third of <ref column="ipv6_ra_configs" key="max_interval"/>,
+ i.e. 200 seconds if that key is unset.
</column>
</group>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
