[ovs-git] [openvswitch/ovs] 7c2699: dpif-netdev: Fix memory leak.

Mon Jan 16 04:13:31 UTC 2017

  Branch: refs/heads/master
  Home:   https://github.com/openvswitch/ovs
  Commit: 7c269972577da6aa3022216d754e9a86bb9461b5
      https://github.com/openvswitch/ovs/commit/7c269972577da6aa3022216d754e9a86bb9461b5
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Fix memory leak.

We keep all the per-port classifiers around, since they can be reused,
but when a pmd thread is destroyed we should free them.

Found using valgrind.

Fixes: 3453b4d62a98("dpif-netdev: dpcls per in_port with sorted
subtables")

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>
Acked-by: Ben Pfaff <blp at ovn.org>

  Commit: febf4a7a8748311e76e462d6843dd6ccb9b4c5de
      https://github.com/openvswitch/ovs/commit/febf4a7a8748311e76e462d6843dd6ccb9b4c5de
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Take non_pmd_mutex to access tx cached ports.

As documented in dp_netdev_pmd_thread, we must take non_pmd_mutex to
access the tx port caches for the non pmd thread.

Found by inspection.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 57eebbb4c3151cdf4c56419c1fea15cfdf0acba4
      https://github.com/openvswitch/ovs/commit/57eebbb4c3151cdf4c56419c1fea15cfdf0acba4
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c
    M lib/netdev.c
    M lib/netdev.h

  Log Message:
  -----------
  dpif-netdev: Don't try to output on a device without txqs.

Tunnel devices have 0 txqs and don't support netdev_send().  While
netdev_send() simply returns EOPNOTSUPP, the XPS logic is still executed
on output, and that might be confused by devices with no txqs.

It seems better to have different structures in the fast path for ports
that support netdev_{push,pop}_header (tunnel devices), and ports that
support netdev_send.  With this we can also remove a branch in
netdev_send().

This is also necessary for a future commit, which starts DPDK devices
without txqs.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 3b1fb0779b87788968c1a6a9ff295a9883547485
      https://github.com/openvswitch/ovs/commit/3b1fb0779b87788968c1a6a9ff295a9883547485
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/netdev-dpdk.c

  Log Message:
  -----------
  netdev-dpdk: Don't call rte_dev_stop() in update_flags().

Calling rte_eth_dev_stop() while the device is running causes a crash.

We could use rte_eth_dev_set_link_down(), but not every PMD implements
that, and I found one NIC where that has no effect.

Instead, this commit checks if the device has the NETDEV_UP flag when
transmitting or receiving (similarly to what we do for vhostuser). I
didn't notice any performance difference with this check in case the
device is up.

An alternative would be to remove the device queues from the pmd threads
tx and receive cache, but that requires reconfiguration and I'd prefer
to avoid it, because the change can come from OpenFlow.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 7f381c2e5455babf27f634f4ca8e62962460dbc4
      https://github.com/openvswitch/ovs/commit/7f381c2e5455babf27f634f4ca8e62962460dbc4
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/netdev-dpdk.c
    M lib/netdev.c
    M vswitchd/bridge.c

  Log Message:
  -----------
  netdev-dpdk: Start also dpdkr devices only once on port-add.

Since commit 55e075e65ef9("netdev-dpdk: Arbitrary 'dpdk' port naming"),
we don't call rte_eth_start() from netdev_open() anymore, we only call
it from netdev_reconfigure().  This commit does that also for 'dpdkr'
devices, and remove some useless code.

Calling rte_eth_start() also from netdev_open() was unnecessary and
wasteful. Not doing it reduces code duplication and makes adding a port
faster (~900ms before the patch, ~400ms after).

Another reason why this is useful is that some DPDK driver might have
problems with reconfiguration. For example, until DPDK commit
8618d19b52b1("net/vmxnet3: reallocate shared memzone on re-config"),
vmxnet3 didn't support being restarted with a different number of
queues.

Technically, the netdev interface changed because before opening rxqs or
calling netdev_send() the user must check if reconfiguration is
required.  This patch also documents that, even though no change to the
userspace datapath (the only user) is required.

Lastly, this patch makes sure the errors returned by ofproto_port_add
(which includes the first port reconfiguration) are reported back to the
database.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 1ce30dfd344e137c1ed716ef82cb7b7b23f04a05
      https://github.com/openvswitch/ovs/commit/1ce30dfd344e137c1ed716ef82cb7b7b23f04a05
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/netdev-dpdk.c

  Log Message:
  -----------
  netdev-dpdk: Refactor construct and destruct.

Some refactoring for _construct() and _destruct() methods:
* Rename netdev_dpdk_init() to common_construct(). init() has a
  different meaning in the netdev context.
* Remove DPDK_DEV_ETH and DPDK_DEV_VHOST checks in common_construct()
  and move them to different functions
* Introduce common_destruct().
* Avoid taking 'dev->mutex' in construct and destruct: we're guaranteed
  to be the only thread with access to the object.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 14e3e12ac3fe58d818fcb276a4f0e6f976dae9e9
      https://github.com/openvswitch/ovs/commit/14e3e12ac3fe58d818fcb276a4f0e6f976dae9e9
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Use a boolean instead of pmd->port_seq.

There's no need for a sequence number, since the main thread has to wait
for the pmd thread, so there's no chance that an update will be
undetected.

A seq object will be introduced for another purpose in the next commit,
and changing this to boolean makes the code more readable.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 2788a1b13822291a7f0eeda4055eafe85410feb1
      https://github.com/openvswitch/ovs/commit/2788a1b13822291a7f0eeda4055eafe85410feb1
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Block pmd threads if there are no ports.

There's no reason for a pmd thread to perform its main loop if there are
no queues in its poll_list.

This commit introduces a seq object on which the pmd thread can be
blocked, if there are no queues.

When the main thread wants to reload a pmd threads it must now change
the seq object (in case it's blocked) and set 'reload' to true.

This is useful to avoid wasting CPU cycles and is also necessary for a
future commit.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: b9584f2122f406ce6c732978ad5244c0eee17ff2
      https://github.com/openvswitch/ovs/commit/b9584f2122f406ce6c732978ad5244c0eee17ff2
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c
    M tests/pmd.at

  Log Message:
  -----------
  dpif-netdev: Create pmd threads for every numa node.

A lot of the complexity in the code that handles pmd threads and ports
in dpif-netdev is due to the fact that we postpone the creation of pmd
threads on a numa node until we have a port that needs to be polled on
that particular node.

Since the previous commit, a pmd thread with no ports will not consume
any CPU, so it seems easier to create all the threads at once.

This will also make future commits easier.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 82d765f6f8fdfdf8e629ec8fdf1de603bc5f1c86
      https://github.com/openvswitch/ovs/commit/82d765f6f8fdfdf8e629ec8fdf1de603bc5f1c86
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Make 'static_tx_qid' const.

Since previous commit, 'static_tx_qid' doesn't need to be atomic and is
actually never touched (except for initialization), so it can be made
const.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: f5d317a1568a21dbcde8c22db2a6f55aa15f0cd9
      https://github.com/openvswitch/ovs/commit/f5d317a1568a21dbcde8c22db2a6f55aa15f0cd9
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpctl.c
    M lib/dpif-netdev.c
    M lib/dpif.c
    M lib/dpif.h
    M tests/pmd.at

  Log Message:
  -----------
  dpctl: Avoid making assumptions on pmd threads.

Currently dpctl depends on ovs-numa module to delete and create flows on
different pmd threads for pmd devices.

The next commits will move away the pmd threads state from ovs-numa to
dpif-netdev, so the ovs-numa interface will not be supported.

Also, the assignment between ports and thread is an implementation
detail of dpif-netdev, dpctl shouldn't know anything about it.

This commit changes the dpif_flow_put() and dpif_flow_del() calls to
iterate over all the pmd threads, if pmd_id is PMD_ID_NULL.

A simple test is added.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: b2ce05edeef52770ab3e2dbd982d52c8d9124993
      https://github.com/openvswitch/ovs/commit/b2ce05edeef52770ab3e2dbd982d52c8d9124993
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/ovs-numa.c
    M lib/ovs-numa.h

  Log Message:
  -----------
  ovs-numa: New ovs_numa_dump_contains_core() function.

It will be used by a future commit.  struct ovs_numa_dump now uses an
hmap instead of a list to make ovs_numa_dump_contains_core() more
efficient.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: dbedeb9dd4cc799a2f2097457f73e7f1922bc8a4
      https://github.com/openvswitch/ovs/commit/dbedeb9dd4cc799a2f2097457f73e7f1922bc8a4
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/ovs-numa.c
    M lib/ovs-numa.h

  Log Message:
  -----------
  ovs-numa: Add new dump types.

They will be used by a future commit.

This patch introduces some code duplication which will be removed in a
future commit.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 0900ca8e6447baa0c06ccbe25fa1948f88224948
      https://github.com/openvswitch/ovs/commit/0900ca8e6447baa0c06ccbe25fa1948f88224948
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/ovs-numa.c

  Log Message:
  -----------
  ovs-numa: Don't use hmap_first_with_hash().

I think it's better to iterate the hmap than to use
hmap_first_with_hash(), because it handles hash collisions.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 90f9f8392fbd39be9fc4418a34db42145bcd893d
      https://github.com/openvswitch/ovs/commit/90f9f8392fbd39be9fc4418a34db42145bcd893d
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/ovs-numa.c
    M lib/ovs-numa.h

  Log Message:
  -----------
  ovs-numa: Add per numa and global counts in dump.

They will be used by a future commit.

Suggested-by: Ilya Maximets <i.maximets at samsung.com>
Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: 947dc56767b81f22808f1f98f0948ed6524745a9
      https://github.com/openvswitch/ovs/commit/947dc56767b81f22808f1f98f0948ed6524745a9
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c

  Log Message:
  -----------
  dpif-netdev: Use hmap for poll_list in pmd threads.

A future commit will use this to determine if a queue is already
contained in a pmd thread.

To keep the behavior unaltered we now have to sort queues before
printing them in pmd_info_show_rxq().

Also this commit introduces 'struct polled_queue' that will be used
exclusively in the fast path, uses 'struct dp_netdev_rxq' from 'struct
rxq_poll' and uses 'rx' for 'netdev_rxq' and 'rxq' for 'dp_netdev_rxq'.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: e32971b8ddb457e37a52af73ba0c37110fe900bb
      https://github.com/openvswitch/ovs/commit/e32971b8ddb457e37a52af73ba0c37110fe900bb
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/dpif-netdev.c
    M tests/pmd.at

  Log Message:
  -----------
  dpif-netdev: Centralized threads and queues handling code.

Currently we have three different code paths that deal with pmd threads
and queues, in response to different input

1. When a port is added
2. When a port is deleted
3. When the cpumask changes or a port must be reconfigured.

1. and 2. are carefully written to minimize disruption to the running
datapath, while 3. brings down all the threads reconfigure all the ports
and restarts everything.

This commit removes the three separate code paths by introducing the
reconfigure_datapath() function, that takes care of adapting the pmd
threads and queues to the current datapath configuration, no matter how
we got there.

This aims at simplifying maintenance and introduces a long overdue
improvement: port reconfiguration (can happen quite frequently for
dpdkvhost ports) is now done without shutting down the whole datapath,
but just by temporarily removing the port that needs to be reconfigured
(while the rest of the datapath is running).

We now also recompute the rxq scheduling from scratch every time a port
is added of deleted.  This means that the queues will be more balanced,
especially when dealing with explicit rxq-affinity from the user
(without shutting down the threads and restarting them), but it also
means that adding or deleting a port might cause existing queues to be
moved between pmd threads.  This negative effect can be avoided by
taking into account the existing distribution when computing the new
scheduling, but I considered code clarity and fast reconfiguration more
important than optimizing port addition or removal (a port is added and
removed only once, but can be reconfigured many times)

Lastly, this commit moves the pmd threads state away from ovs-numa.  Now
the pmd threads state is kept only in dpif-netdev.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Co-authored-by: Ilya Maximets <i.maximets at samsung.com>
Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

  Commit: ea58c9699c1fc0c7bd86e91f8b3d81894a844520
      https://github.com/openvswitch/ovs/commit/ea58c9699c1fc0c7bd86e91f8b3d81894a844520
  Author: Daniele Di Proietto <diproiettod at vmware.com>
  Date:   2017-01-15 (Sun, 15 Jan 2017)

  Changed paths:
    M lib/ovs-numa.c
    M lib/ovs-numa.h

  Log Message:
  -----------
  ovs-numa: Remove unused functions.

ovs-numa doesn't need to keep the state of the pmd threads, it is an
implementation detail of dpif-netdev.

Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
Acked-by: Ilya Maximets <i.maximets at samsung.com>

Compare: https://github.com/openvswitch/ovs/compare/871a38766b47...ea58c9699c1f