[ovs-dev] [PATCH v2] dpif-netdev: Fix (packet) memory leaks in the slow path.

[Pravin Shelar]

On Fri, Sep 19, 2014 at 4:20 PM, Daniele Di Proietto
<ddiproietto at vmware.com> wrote:
> If a packet didn't match a rule in the fast path classifier its memory was
> never freed. The issue was particularly clear with DPDK devices because it was
> not possible to process more than ~250000 DPDK mbufs in the slow path.
>
> This commit fixes the problem by:
> * calling dpif_packet_delete() if the upcalls are disabled
> * passing may_steal==true to dp_netdev_execute_actions() during normal upcall
>   processing
>
> Signed-off-by: Daniele Di Proietto <ddiproietto at vmware.com>
> ---
> v2:
> * (Pravin's suggestion) restructured dp_execute_cb to return immediately if
>   successful and free packets at the end
> ---
>  lib/dpif-netdev.c | 56 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 90fe01c..6b8201b 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2681,7 +2681,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
>              /* We can't allow the packet batching in the next loop to execute
>               * the actions.  Otherwise, if there are any slow path actions,
>               * we'll send the packet up twice. */
> -            dp_netdev_execute_actions(pmd, &packets[i], 1, false, md,
> +            dp_netdev_execute_actions(pmd, &packets[i], 1, true, md,
>                                        ofpbuf_data(&actions),
>                                        ofpbuf_size(&actions));
>
> @@ -2707,6 +2707,17 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
>          ofpbuf_uninit(&actions);
>          ofpbuf_uninit(&put_actions);
>          fat_rwlock_unlock(&dp->upcall_rwlock);
> +    } else if (OVS_UNLIKELY(any_miss)) {
> +        int dropped_cnt = 0;
> +
> +        for (i = 0; i < cnt; i++) {
> +            if (OVS_UNLIKELY(!rules[i] && mfs[i])) {
> +                dpif_packet_delete(packets[i]);
> +                dropped_cnt++;
> +            }
> +        }
> +
> +        dp_netdev_count_packet(dp, DP_STAT_LOST, dropped_cnt);
>      }
>
>      n_batches = 0;
> @@ -2763,6 +2774,18 @@ dpif_netdev_register_upcall_cb(struct dpif *dpif, upcall_callback *cb,
>  }
>
>  static void
> +dp_netdev_drop_packets(struct dpif_packet ** packets, int cnt, bool may_steal)
> +{
> +    int i;
> +
> +    if (may_steal) {
> +        for (i = 0; i < cnt; i++) {
> +            dpif_packet_delete(packets[i]);
> +        }
> +    }
> +}
> +
> +static void
>  dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>                struct pkt_metadata *md,
>                const struct nlattr *a, bool may_steal)
> @@ -2781,10 +2804,7 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>          p = dp_netdev_lookup_port(dp, u32_to_odp(nl_attr_get_u32(a)));
>          if (OVS_LIKELY(p)) {
>              netdev_send(p->netdev, pmd->core_id, packets, cnt, may_steal);
> -        } else if (may_steal) {
> -            for (i = 0; i < cnt; i++) {
> -                dpif_packet_delete(packets[i]);
> -            }
> +            return;
>          }
>          break;
>
> @@ -2807,19 +2827,18 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>                                           DPIF_UC_ACTION, userdata, &actions,
>                                           NULL);
>                  if (!error || error == ENOSPC) {
> -                    dp_netdev_execute_actions(pmd, &packets[i], 1, false, md,
> -                                              ofpbuf_data(&actions),
> +                    dp_netdev_execute_actions(pmd, &packets[i], 1, may_steal,
> +                                              md, ofpbuf_data(&actions),
>                                                ofpbuf_size(&actions));
> -                }
> -
> -                if (may_steal) {
> +                } else if (may_steal) {
>                      dpif_packet_delete(packets[i]);
>                  }
>              }
>              ofpbuf_uninit(&actions);
>              fat_rwlock_unlock(&dp->upcall_rwlock);
> -        }
>
> +            return;
> +        }
>          break;
>
>      case OVS_ACTION_ATTR_HASH: {
> @@ -2850,7 +2869,7 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>              }
>              dpif_packet_set_dp_hash(packets[i], hash);
>          }
> -        break;
> +        return;
>      }
>
>      case OVS_ACTION_ATTR_RECIRC:
> @@ -2874,15 +2893,10 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>              }
>              (*depth)--;
>
> -            break;
> -        } else {
> -            VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
> -            if (may_steal) {
> -                for (i = 0; i < cnt; i++) {
> -                    dpif_packet_delete(packets[i]);
> -                }
> -            }
> +            return;
>          }
> +
> +        VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
>          break;
>
>      case OVS_ACTION_ATTR_PUSH_VLAN:
> @@ -2896,6 +2910,8 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
>      }
> +
> +    dp_netdev_drop_packets(packets, cnt, may_steal);
>  }
>
>  static void
> --

LGTM.

> 2.1.0.rc1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev