On Tue, Sep 17, 2013 at 10:13:47AM -0700, gy...@nicira.com wrote:
> From: Guolin Yang <gy...@nicira.com>
>
> There are two models in OVS in configure datapath port:
> 1. Datapath port created before user space bridge is configured.
> In this model, datapath can be deleted or added independent of
> user-space.
> 2. Datapath port created when ovsdb is requested to add the relevant
> port.
>
> Traditionally OVS supports the second model which is used in Linux
> platform.
>
> With more platform support, OVS requires to support first model. In
> this case, datapath port change detected by ofproto layer need to trigger
> brdige to reconfigure so that bridge and ofproto layers can be
> synchronized.
>
> This change introduced a API for ofproto to request reconfiguration and
> for bridge to detect the request.
>
> Signed-off-by: Guolin Yang <gy...@nicira.com>
This is where I expected the extensive comment that you added in
process_dpif_port_change():
> +/* ofproto layer set need configure to true to request
> + * bridge reconfiguration, while bridge will clear the
> + * the need by passing false to the API. */
> +void
> +ofproto_set_need_reconfigure(bool need)
> +{
> + need_reconfigure = need;
> +}
> +
The OVS style never puts { on the same line as a function prototype:
> +bool
> +ofproto_need_reconfigure(void) {
> + return need_reconfigure;
> +}
> +
> /* Check for and handle port changes in 'backer''s dpif. */
> static void
> process_dpif_port_changes(struct dpif_backer *backer)
> @@ -961,6 +978,20 @@ process_dpif_port_change(struct dpif_backer *backer,
> const char *devname)
> /* The port was added, but we don't know with which
> * ofproto we should associate it. Delete it. */
> dpif_port_del(backer->dpif, port.port_no);
I am nervous about adding ofproto_set_need_reconfigure() here in this
path, because I suspect that with the in-tree datapaths it will cause
repeated reconfigurations: the user modifies the database, the OVS
bridge module reconfigures the datapaths, the datapaths report that they
changed, and then the bridge module reconfigures again. So I would
prefer, at least for now, to keep the following code in just the
dpif that actually needs it. The comment, on the other hand, is worth
having, probably on ofproto_set_need_reconfigure(), as an explanation
for why that mechanism exists:
> + /* While there are two ways of adding/deleting datapath
> + * port:
> + * 1. Datapath added/deleted as requested by bridge layer
> + * 2. Datapath added/deleted independent of bridge layer
> + *
> + * In second case, when a datapath port is added/deleted,
> + * normally there will be other process to program the bridge
> + * (through ovsdb) to synchronize the user space and
> + * datapath. However there are cases where bridge configuration
> + * is not changed, but datapath port may be changed, in this
> + * case, ofproto layer detects the change and request the bridge
> + * to reconfigure to synchronize user space and datapath
> + * configuration. */
> + ofproto_set_need_reconfigure(true);
> } else {
> struct ofport_dpif *ofport;
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
