On Tue, Nov 28, 2017 at 5:49 PM Ben Pfaff <[email protected]> wrote:
> 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!
>
Thanks for your review, Ben. I've folded all of these changes (plus your
comments on patch 1) into a v6.
>
> --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
