| Message ID | 1504603381-30071-4-git-send-email-yliu@fridaylinux.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OVS-DPDK flow offload with rte_flow | expand |
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); > +} > + > +/* 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? ...
On 9/8/17, 9:46 AM, "ovs-dev-bounces@openvswitch.org on behalf of Simon Horman" <ovs-dev-bounces@openvswitch.org on behalf of simon.horman@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
dev@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=
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
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.
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
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index f58e9be..46f9885 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -36,6 +36,7 @@ #include <rte_meter.h> #include <rte_pci.h> #include <rte_vhost.h> +#include <rte_flow.h> #include "dirs.h" #include "dp-packet.h" @@ -3312,6 +3313,93 @@ unlock: return err; } +/* + * A mapping from ufid to dpdk rte_flow pointer + */ + +static struct hmap ufid_dpdk_flow = HMAP_INITIALIZER(&ufid_dpdk_flow); +static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER; + +struct ufid_dpdk_flow_data { + struct hmap_node node; + ovs_u128 ufid; + struct rte_flow *rte_flow; +}; + +/* + * Find ufid_dpdk_flow_data node associated with @ufid + */ +static struct ufid_dpdk_flow_data * +find_ufid_dpdk_flow_mapping(const ovs_u128 *ufid) +{ + size_t hash = hash_bytes(ufid, sizeof(*ufid), 0); + struct ufid_dpdk_flow_data *data = NULL; + + ovs_mutex_lock(&ufid_lock); + HMAP_FOR_EACH_WITH_HASH (data, node, hash, &ufid_dpdk_flow) { + if (ovs_u128_equals(*ufid, data->ufid)) { + break; + } + } + ovs_mutex_unlock(&ufid_lock); + + return data; +} + +/* + * 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); + } +} + +/* 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); +} + +/* Get rte_flow by ufid. + * + * Returns rte rte_flow if successful. Otherwise returns 0. + */ +static inline struct rte_flow * +get_rte_flow_by_ufid(const ovs_u128 *ufid) +{ + struct ufid_dpdk_flow_data *data; + + data = find_ufid_dpdk_flow_mapping(ufid); + return data ? data->rte_flow : NULL; +} + + #define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT, \ SET_CONFIG, SET_TX_MULTIQ, SEND, \ GET_CARRIER, GET_STATS, \