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?
--yliu
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
