> On Sep 8, 2014, at 3:23 AM, Joe Stringer <joestrin...@nicira.com> wrote:
>
> It's highly unlikely that two flows will hash to the same UID. However,
> we could handle multiple upcalls from the same flow. If we hash the flow
> with an additional hash function and the hashes are the same, then the
> upcalls are most likely from the same flow (assuming that the hash
> collisions are different for each functions). If the hashes are
> different, then we have generated the same UID hash for two different
> flows, and we should log a warning.
I did not look at the patch, but would it be possible to detect a duplicate
upcall by comparing the flow keys when the hashes collide?
Jarno
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> RFC. This is really unlikely, but we don't have any way of detecting
> this case currently. This makes it likely that we'll detect such cases,
> but it's still theoretically possible we'll miss them. Not sure whether
> the extra code is warranted or not. During testing I've only triggered
> the handler_duplicate_upcall case and never the VLOG_WARN case.
> ---
> ofproto/ofproto-dpif-upcall.c | 48 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ad50f79..e663f78 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -46,7 +46,7 @@
>
> VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
>
> -COVERAGE_DEFINE(handler_install_conflict);
> +COVERAGE_DEFINE(handler_duplicate_upcall);
> COVERAGE_DEFINE(upcall_ukey_contention);
> COVERAGE_DEFINE(dumped_duplicate_flow);
> COVERAGE_DEFINE(dumped_new_flow);
> @@ -99,6 +99,8 @@ struct udpif {
> struct dpif *dpif; /* Datapath handle. */
> struct dpif_backer *backer; /* Opaque dpif_backer pointer. */
>
> + uint32_t secret; /* Random seed for udpif_key 'check'.
> */
> +
> struct handler *handlers; /* Upcall handlers. */
> size_t n_handlers;
>
> @@ -207,6 +209,8 @@ struct udpif_key {
> struct ofpbuf *actions; /* Datapath flow actions as nlattrs. */
> ovs_u128 uid; /* Unique flow identifier. */
> uint32_t hash; /* Pre-computed hash for 'key'. */
> + uint32_t check; /* Additional hash for detecting duplicate
> + upcalls. */
> atomic_bool installed; /* True if the datapath flow has been
> installed and handlers are finished with
> this ukey. Used to reduce contention. */
> @@ -268,7 +272,7 @@ static void upcall_unixctl_set_flow_limit(struct
> unixctl_conn *conn, int argc,
> static void upcall_unixctl_dump_wait(struct unixctl_conn *conn, int argc,
> const char *argv[], void *aux);
>
> -static struct udpif_key *ukey_new(struct upcall *);
> +static struct udpif_key *ukey_new(struct udpif *udpif, struct upcall *);
> static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
> static void ukey_install_finish(struct udpif_key *ukey, int error);
> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
> @@ -316,6 +320,7 @@ udpif_create(struct dpif_backer *backer, struct dpif
> *dpif)
> udpif->dpif = dpif;
> udpif->backer = backer;
> atomic_init(&udpif->flow_limit, MIN(ofproto_flow_limit, 10000));
> + udpif->secret = random_uint32();
> udpif->reval_seq = seq_create();
> udpif->dump_seq = seq_create();
> latch_init(&udpif->exit_latch);
> @@ -958,7 +963,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
> &upcall->put_actions);
> }
>
> - upcall->ukey = ukey_new(upcall);
> + upcall->ukey = ukey_new(udpif, upcall);
> }
>
> static void
> @@ -1236,7 +1241,23 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *uid)
> }
>
> static struct udpif_key *
> -ukey_new(struct upcall *upcall)
> +ukey_lookup_by_uid_check(struct udpif *udpif, const ovs_u128 *uid,
> + const uint32_t check)
> +{
> + struct udpif_key *ukey;
> + int idx = get_uid_hash(uid) % N_UMAPS;
> + struct cmap *cmap = &udpif->ukeys[idx].cmap;
> +
> + CMAP_FOR_EACH_WITH_HASH (ukey, cmap_node, get_uid_hash(uid), cmap) {
> + if (!memcmp(&ukey->check, &check, sizeof check)) {
> + return ukey;
> + }
> + }
> + return NULL;
> +}
> +
> +static struct udpif_key *
> +ukey_new(struct udpif *udpif, struct upcall *upcall)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> struct udpif_key *ukey = xzalloc(sizeof *ukey);
> @@ -1268,6 +1289,8 @@ ukey_new(struct upcall *upcall)
> ukey->mask_len = ofpbuf_size(&mask);
> memcpy(&ukey->uid, upcall->uid, sizeof ukey->uid);
> ukey->hash = get_uid_hash(&ukey->uid);
> + ukey->check = hash_words(ALIGNED_CAST(uint32_t*,ukey->key),
> ukey->key_len,
> + udpif->secret);
> ukey->actions = ofpbuf_clone(&upcall->put_actions);
>
> atomic_init(&ukey->installed, false);
> @@ -1300,8 +1323,21 @@ ukey_install_start(struct udpif *udpif, struct
> udpif_key *ukey)
> cmap_insert(&umap->cmap, &ukey->cmap_node, ukey->hash);
> locked = true;
> } else {
> - COVERAGE_INC(handler_install_conflict);
> - VLOG_DBG("Failed to ukey_install_start(): 0x%"PRIx32, ukey->hash);
> + /* It's highly unlikely that two flows will hash to the same UID.
> + * However, we could be handling multiple upcalls from the same flow.
> + * If we hash the flow with a different hash function and the hashes
> + * are the same, then the upcalls are most likely from the same flow.
> + * If the hashes are different, then we have generated the same UID
> + * hash for two different flows. */
> + if (ukey_lookup_by_uid_check(udpif, &ukey->uid, ukey->check)) {
> + COVERAGE_INC(handler_duplicate_upcall);
> + } else {
> + struct ds ds = DS_EMPTY_INITIALIZER;
> +
> + odp_format_uid(&ukey->uid, &ds);
> + VLOG_WARN("Conflicting ukey with %s", ds_cstr(&ds));
> + ds_destroy(&ds);
> + }
> }
> ovs_mutex_unlock(&umap->mutex);
>
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
