[ovs-dev] [PATCH v2] dpif: allow adding ukeys for same flow by different pmds

[Alex Wang]

Thx for reporting this, I worry this may not be an ovs issue, but to do with
the dpdk driver...  Here is my analysis, please correct me if wrong,

When pmd threads are spawned, each of them will take ownership of port
queues in a round robin fashion...  In other words, each port queue will
be read by only one pmd thread.

Then based on my understanding the dpdk module will hash packets to one
particular queue on the receiving port.  Thusly packets from the same flow
(or stream) should only be handled by one pmd thread.

Next, the issue you reported will only happen when the packets from same
flow (or stream) are handled by multiple pmd threads.  [In that case, only
one
pmd thread will have the flow installed in its classifier (and exact match
cache)...  and all other pmd threads fail to install the flow due to
duplicate
ufid]

So, based on the analysis, this really should not happen, and if happens,
indicate a packet queueing issue in dpdk.

Ilya, could you reproduce this issue and provide the `ovs-appctl
coverage/show` output?  Want to confirm that the 'handler_duplicate_upcall'
counter is increasing when this issue happens.

Daniele, could you also provide thoughts on this?  Since I'm not sure if
the
ovs dpdk code changes (since i last touched it long time ago) affect the
issue
here.


Thanks,
Alex Wang,




On Fri, Aug 7, 2015 at 2:35 AM, Ilya Maximets <i.maximets at samsung.com>
wrote:

> In multiqueue mode several pmd threads may process one
> port, but different queues. Flow doesn't depend on queue.
>
> So, while miss upcall processing, all threads (except first
> for that port) will receive error = ENOSPC due to
> ukey_install failure. Therefore they will not add the flow
> to flow_table and will not insert it to exact match cache.
>
> As a result all threads (except first for that port) will
> always execute a miss.
>
> Fix that by mixing pmd_id with the bits from the ufid
> for ukey->hash calculation.
>
> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> ---
>  ofproto/ofproto-dpif-upcall.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 0f2e186..57a7f34 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -291,7 +291,8 @@ static bool ukey_install_start(struct udpif *, struct
> udpif_key *ukey);
>  static bool ukey_install_finish(struct udpif_key *ukey, int error);
>  static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
>  static struct udpif_key *ukey_lookup(struct udpif *udpif,
> -                                     const ovs_u128 *ufid);
> +                                     const ovs_u128 *ufid,
> +                                     const unsigned pmd_id);
>  static int ukey_acquire(struct udpif *, const struct dpif_flow *,
>                          struct udpif_key **result, int *error);
>  static void ukey_delete__(struct udpif_key *);
> @@ -1143,7 +1144,8 @@ process_upcall(struct udpif *udpif, struct upcall
> *upcall,
>              }
>              if (actions_len == 0) {
>                  /* Lookup actions in userspace cache. */
> -                struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
> +                struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid,
> +                                                     upcall->pmd_id);
>                  if (ukey) {
>                      actions = ukey->actions->data;
>                      actions_len = ukey->actions->size;
> @@ -1320,19 +1322,19 @@ handle_upcalls(struct udpif *udpif, struct upcall
> *upcalls,
>  }
>
>  static uint32_t
> -get_ufid_hash(const ovs_u128 *ufid)
> +get_ukey_hash(const ovs_u128 *ufid, const unsigned pmd_id)
>  {
> -    return ufid->u32[0];
> +    return ufid->u32[0] + pmd_id;
>  }
>
>  static struct udpif_key *
> -ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
> +ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid, const unsigned
> pmd_id)
>  {
>      struct udpif_key *ukey;
> -    int idx = get_ufid_hash(ufid) % N_UMAPS;
> +    int idx = get_ukey_hash(ufid, pmd_id) % N_UMAPS;
>      struct cmap *cmap = &udpif->ukeys[idx].cmap;
>
> -    CMAP_FOR_EACH_WITH_HASH (ukey, cmap_node, get_ufid_hash(ufid), cmap) {
> +    CMAP_FOR_EACH_WITH_HASH (ukey, cmap_node, get_ukey_hash(ufid,
> pmd_id), cmap) {
>          if (ovs_u128_equals(&ukey->ufid, ufid)) {
>              return ukey;
>          }
> @@ -1362,7 +1364,7 @@ ukey_create__(const struct nlattr *key, size_t
> key_len,
>      ukey->ufid_present = ufid_present;
>      ukey->ufid = *ufid;
>      ukey->pmd_id = pmd_id;
> -    ukey->hash = get_ufid_hash(&ukey->ufid);
> +    ukey->hash = get_ukey_hash(&ukey->ufid, pmd_id);
>      ukey->actions = ofpbuf_clone(actions);
>
>      ovs_mutex_init(&ukey->mutex);
> @@ -1490,7 +1492,7 @@ ukey_install_start(struct udpif *udpif, struct
> udpif_key *new_ukey)
>      idx = new_ukey->hash % N_UMAPS;
>      umap = &udpif->ukeys[idx];
>      ovs_mutex_lock(&umap->mutex);
> -    old_ukey = ukey_lookup(udpif, &new_ukey->ufid);
> +    old_ukey = ukey_lookup(udpif, &new_ukey->ufid, new_ukey->pmd_id);
>      if (old_ukey) {
>          /* Uncommon case: A ukey is already installed with the same UFID.
> */
>          if (old_ukey->key_len == new_ukey->key_len
> @@ -1572,7 +1574,7 @@ ukey_acquire(struct udpif *udpif, const struct
> dpif_flow *flow,
>      struct udpif_key *ukey;
>      int retval;
>
> -    ukey = ukey_lookup(udpif, &flow->ufid);
> +    ukey = ukey_lookup(udpif, &flow->ufid, flow->pmd_id);
>      if (ukey) {
>          retval = ovs_mutex_trylock(&ukey->mutex);
>      } else {
> --
> 2.1.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>