On Wed, Sep 13, 2017 at 11:52:34AM +0200, Simon Horman wrote:
> > > > +/* Add ufid to dpdk_flow mapping */
> > > > +static inline void
> > > > +add_ufid_dpdk_flow_mapping(const ovs_u128 *ufid, struct rte_flow
> > > *rte_flow)
> > > > +{
> > > > + size_t hash = hash_bytes(ufid, sizeof(*ufid), 0);
> > > > + struct ufid_dpdk_flow_data *data = xzalloc(sizeof(*data));
> > > > +
> > > > + /*
> > > > + * We should not simply overwrite an existing rte flow.
> > > > + * We should have deleted it first before re-adding it.
> > > > + * Thus, if following assert triggers, something is wrong:
> > > > + * the rte_flow is not destroyed.
> > > > + */
> > > > + ovs_assert(find_ufid_dpdk_flow_mapping(ufid) == NULL);
> > > > +
> > > > + data->ufid = *ufid;
> > > > + data->rte_flow = rte_flow;
> > > > +
> > > > + ovs_mutex_lock(&ufid_lock);
> > > > + hmap_insert(&ufid_dpdk_flow, &data->node, hash);
> > > > + ovs_mutex_unlock(&ufid_lock);
> > > > +}
> > >
> > > I am not sure that the locking in the add and del functions above
> > > can't
> > > race. f.e. can two deletion requests for the same flow occur in
> > > parallel?
> >
> > Seems no to me, but I'm also note sure about that. If that's concerned,
> > I could add a locked version of find: find_ufid_dpdk_flow_mapping_locked.
> > Good to you?
>
> I see that access to ufid_dpdk_flow is protected by ufid_lock, but I'm
> still not sure how one protects against concurrent access to an
> element of the hash.
>
> f.e.:
>
> * Two concurrent deletes of the same element
> * A concurrent get and delete of the same element
>
> Its also not clear to me that adding the same id twice is guarded against.
>
> Perhaps the use of this API makes my concerns moot as callers
> never manipulate the same ufid concurrently.
As Darrell pointed out (privately), they are protected by the flow_mutex
lock, that I thin this will not happen?
--yliu
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
