On 9/8/17, 9:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of Simon Horman"
<ovs-dev-boun...@openvswitch.org on behalf of simon.hor...@netronome.com> wrote:
On Tue, Sep 05, 2017 at 05:22:56PM +0800, Yuanhan Liu wrote:
> Flows offloaded to DPDK are identified by rte_flow pointer while OVS
> flows are identified by ufid. This patches adds a hmap to convert ufid
> to dpdk flow (rte_flow).
>
> Most of the code are stolen from netdev-tc-offloads.c, with some
> modificatons.
>
> Some functions are marked as "inline", which is a trick to workaround
> the temp "functiond defined but not used" warnings.
...
> +/*
> + * Remove ufid_dpdk_flow_data node associated with @ufid
> + */
> +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
+ }
//////////////////////////
> +}
> +
> +/* 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?
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EqZ7zhHzkcbfWx0DXX2Yi0ilPUo3gLVNfo5C7ZFlXkc&s=VZc7ZWCSFtFZCpNq-3YBxOJkzVgP-EmVevjn7nwMJTI&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
