Thanks, I feel better about my understanding of the code now.
I'll compose a patch in the not to distant future.
On Tue, Aug 20, 2013 at 05:44:49PM -0700, Ethan Jackson wrote:
> Just an oversight. Feel free to fix it.
>
> Ethan
>
> On Mon, Aug 19, 2013 at 6:29 PM, Simon Horman <ho...@verge.net.au> wrote:
> > On Sat, Aug 03, 2013 at 06:42:06PM -0700, Ethan Jackson wrote:
> >> Signed-off-by: Ethan Jackson <et...@nicira.com>
> >> ---
> >> ofproto/ofproto-dpif.c | 69
> >> +++++++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 51 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> index 17bc12f..e2dcd3f 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -486,8 +486,9 @@ struct ofproto_dpif {
> >> long long int stp_last_tick;
> >>
> >> /* VLAN splinters. */
> >> - struct hmap realdev_vid_map; /* (realdev,vid) -> vlandev. */
> >> - struct hmap vlandev_map; /* vlandev -> (realdev,vid). */
> >> + struct ovs_mutex vsp_mutex;
> >> + struct hmap realdev_vid_map OVS_GUARDED; /* (realdev,vid) -> vlandev.
> >> */
> >> + struct hmap vlandev_map OVS_GUARDED; /* vlandev -> (realdev,vid).
> >> */
> >>
> >> /* Ports. */
> >> struct sset ports; /* Set of standard port names. */
> >> @@ -1273,6 +1274,7 @@ construct(struct ofproto *ofproto_)
> >> ofproto->ml = mac_learning_create(MAC_ENTRY_DEFAULT_IDLE_TIME);
> >> ofproto->mbridge = mbridge_create();
> >> ofproto->has_bonded_bundles = false;
> >> + ovs_mutex_init(&ofproto->vsp_mutex, PTHREAD_MUTEX_NORMAL);
> >>
> >> classifier_init(&ofproto->facets);
> >> ofproto->consistency_rl = LLONG_MIN;
> >> @@ -1459,6 +1461,8 @@ destruct(struct ofproto *ofproto_)
> >> sset_destroy(&ofproto->ghost_ports);
> >> sset_destroy(&ofproto->port_poll_set);
> >>
> >> + ovs_mutex_destroy(&ofproto->vsp_mutex);
> >> +
> >> close_dpif_backer(ofproto->backer);
> >> }
> >>
> >> @@ -6384,20 +6388,20 @@ hash_realdev_vid(ofp_port_t realdev_ofp_port, int
> >> vid)
> >>
> >> bool
> >> ofproto_has_vlan_splinters(const struct ofproto_dpif *ofproto)
> >> + OVS_EXCLUDED(ofproto->vsp_mutex)
> >> {
> >> - return !hmap_is_empty(&ofproto->realdev_vid_map);
> >> + bool ret;
> >> +
> >> + ovs_mutex_lock(&ofproto->vsp_mutex);
> >> + ret = !hmap_is_empty(&ofproto->realdev_vid_map);
> >> + ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> + return ret;
> >> }
> >>
> >> -/* Returns the OFP port number of the Linux VLAN device that corresponds
> >> to
> >> - * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> >> - * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0
> >> and
> >> - * 'vlan_tci' 9, it would return the port number of eth0.9.
> >> - *
> >> - * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> >> - * function just returns its 'realdev_ofp_port' argument. */
> >> -ofp_port_t
> >> -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> >> - ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> +static ofp_port_t
> >> +vsp_realdev_to_vlandev__(const struct ofproto_dpif *ofproto,
> >> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> + OVS_REQUIRES(ofproto->vsp_mutex)
> >> {
> >> if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
> >> int vid = vlan_tci_to_vid(vlan_tci);
> >> @@ -6415,6 +6419,26 @@ vsp_realdev_to_vlandev(const struct ofproto_dpif
> >> *ofproto,
> >> return realdev_ofp_port;
> >> }
> >>
> >> +/* Returns the OFP port number of the Linux VLAN device that corresponds
> >> to
> >> + * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> >> + * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0
> >> and
> >> + * 'vlan_tci' 9, it would return the port number of eth0.9.
> >> + *
> >> + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> >> + * function just returns its 'realdev_ofp_port' argument. */
> >> +ofp_port_t
> >> +vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> >> + ofp_port_t realdev_ofp_port, ovs_be16 vlan_tci)
> >> + OVS_EXCLUDED(ofproto->vsp_mutex)
> >> +{
> >> + ofp_port_t ret;
> >> +
> >> + ovs_mutex_lock(&ofproto->vsp_mutex);
> >> + ret = vsp_realdev_to_vlandev__(ofproto, realdev_ofp_port, vlan_tci);
> >> + ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> + return ret;
> >> +}
> >> +
> >> static struct vlan_splinter *
> >> vlandev_find(const struct ofproto_dpif *ofproto, ofp_port_t
> >> vlandev_ofp_port)
> >> {
> >> @@ -6443,6 +6467,7 @@ vlandev_find(const struct ofproto_dpif *ofproto,
> >> ofp_port_t vlandev_ofp_port)
> >> static ofp_port_t
> >> vsp_vlandev_to_realdev(const struct ofproto_dpif *ofproto,
> >> ofp_port_t vlandev_ofp_port, int *vid)
> >> + OVS_REQ_WRLOCK(ofproto->vsp_mutex)
> >> {
> >> if (!hmap_is_empty(&ofproto->vlandev_map)) {
> >> const struct vlan_splinter *vsp;
> >> @@ -6466,11 +6491,14 @@ vsp_vlandev_to_realdev(const struct ofproto_dpif
> >> *ofproto,
> >> * making any changes. */
> >> static bool
> >> vsp_adjust_flow(const struct ofproto_dpif *ofproto, struct flow *flow)
> >> + OVS_EXCLUDED(ofproto->vsp_mutex)
> >> {
> >> ofp_port_t realdev;
> >> int vid;
> >>
> >> + ovs_mutex_lock(&ofproto->vsp_mutex);
> >> realdev = vsp_vlandev_to_realdev(ofproto, flow->in_port.ofp_port,
> >> &vid);
> >> + ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> if (!realdev) {
> >> return false;
> >> }
> >> @@ -6488,6 +6516,7 @@ vsp_remove(struct ofport_dpif *port)
> >> struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
> >> struct vlan_splinter *vsp;
> >
> > I am curious to know why vsp_remove isn't annotated with
> > OVS_EXCLUDED(ofproto->vsp_mutex). Likewise for vsp_add();
> >
> >>
> >> + ovs_mutex_lock(&ofproto->vsp_mutex);
> >> vsp = vlandev_find(ofproto, port->up.ofp_port);
> >> if (vsp) {
> >> hmap_remove(&ofproto->vlandev_map, &vsp->vlandev_node);
> >> @@ -6498,6 +6527,7 @@ vsp_remove(struct ofport_dpif *port)
> >> } else {
> >> VLOG_ERR("missing vlan device record");
> >> }
> >> + ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> }
> >>
> >> static void
> >> @@ -6505,24 +6535,27 @@ vsp_add(struct ofport_dpif *port, ofp_port_t
> >> realdev_ofp_port, int vid)
> >> {
> >> struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
> >>
> >> + ovs_mutex_lock(&ofproto->vsp_mutex);
> >> if (!vsp_vlandev_to_realdev(ofproto, port->up.ofp_port, NULL)
> >> - && (vsp_realdev_to_vlandev(ofproto, realdev_ofp_port, htons(vid))
> >> + && (vsp_realdev_to_vlandev__(ofproto, realdev_ofp_port,
> >> htons(vid))
> >> == realdev_ofp_port)) {
> >> struct vlan_splinter *vsp;
> >>
> >> vsp = xmalloc(sizeof *vsp);
> >> - hmap_insert(&ofproto->vlandev_map, &vsp->vlandev_node,
> >> - hash_ofp_port(port->up.ofp_port));
> >> - hmap_insert(&ofproto->realdev_vid_map, &vsp->realdev_vid_node,
> >> - hash_realdev_vid(realdev_ofp_port, vid));
> >> vsp->realdev_ofp_port = realdev_ofp_port;
> >> vsp->vlandev_ofp_port = port->up.ofp_port;
> >> vsp->vid = vid;
> >>
> >> port->realdev_ofp_port = realdev_ofp_port;
> >> +
> >> + hmap_insert(&ofproto->vlandev_map, &vsp->vlandev_node,
> >> + hash_ofp_port(port->up.ofp_port));
> >> + hmap_insert(&ofproto->realdev_vid_map, &vsp->realdev_vid_node,
> >> + hash_realdev_vid(realdev_ofp_port, vid));
> >> } else {
> >> VLOG_ERR("duplicate vlan device record");
> >> }
> >> + ovs_mutex_unlock(&ofproto->vsp_mutex);
> >> }
> >>
> >> static odp_port_t
> >> --
> >> 1.7.9.5
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
> >>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
