On Mon, Sep 11, 2017 at 01:46:28PM +0800, Yuanhan Liu wrote:
> On Fri, Sep 08, 2017 at 05:48:36PM +0000, Darrell Ball wrote:
> >
> > > +static inline void
> > > +del_ufid_dpdk_flow_mapping(const ovs_u128 *ufid)
> > > +{
> > > + struct ufid_dpdk_flow_data *data;
> > > +
> > > + data = find_ufid_dpdk_flow_mapping(ufid);
> > > + if (data) {
> > > + ovs_mutex_lock(&ufid_lock);
> > > + hmap_remove(&ufid_dpdk_flow, &data->node);
> > > + free(data);
> > > + ovs_mutex_unlock(&ufid_lock);
> > > + }
> >
> > I think it would be nicer to exit early:
> >
> > + if (!data) {
> > + return;
> > + }
> > +
> > + ovs_mutex_lock(&ufid_lock);
> > + hmap_remove(&ufid_dpdk_flow, &data->node);
> > + free(data);
> > + ovs_mutex_unlock(&ufid_lock);
> >
> >
> > [Darrell] It is a minor point, but the preference is typically given to the
> > affirmative
> > condition check. A slight variation of Yuanhan’s version
> > would be:
> >
> > + data = find_ufid_dpdk_flow_mapping(ufid);
> > + if (OVS_LIKELY(data)) {
> > + ovs_mutex_lock(&ufid_lock);
> > + hmap_remove(&ufid_dpdk_flow, &data->node);
> > + free(data);
> > + ovs_mutex_unlock(&ufid_lock);
> > + } else {
> > + VLOG_WARN….. <<<<< since this is probably unexpected
> > + }
> >
> > //////////////////////////
>
> I could do that.
>
> > > +}
> > > +
> > > +/* 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
