Hi,
Thanks again for the patch,
As I said in the discussion of v2, I think we should keep the
condition variable, so that we can return from the datapath
functions when the port operations are effectively completed.
I also have a couple of minor coding style suggestions (inline).
Other than this the patch looks good to me
Thanks,
Daniele
On 25/01/2016 07:00, "Ilya Maximets" <i.maxim...@samsung.com> wrote:
>Current rx queue management model is buggy and will not work properly
>without additional barriers and other syncronization between PMD
>threads and main thread.
>
>Known BUGS of current model:
> * While reloading, two PMD threads, one already reloaded and
> one not yet reloaded, can poll same queue of the same port.
> This behavior may lead to dpdk driver failure, because they
> are not thread-safe.
> * Same bug as fixed in commit e4e74c3a2b
> ("dpif-netdev: Purge all ukeys when reconfigure pmd.") but
> reproduced while only reconfiguring of pmd threads without
> restarting, because addition may change the sequence of
> other ports, which is important in time of reconfiguration.
>
>Introducing the new model, where distribution of queues made by main
>thread with minimal synchronizations and without data races between
>pmd threads. Also, this model should work faster, because only
>needed threads will be interrupted for reconfiguraition and total
>computational complexity of reconfiguration is less.
>
>Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>---
> lib/dpif-netdev.c | 245
>++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 147 insertions(+), 98 deletions(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index cd72e62..f055c6e 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -372,6 +372,13 @@ struct dp_netdev_pmd_cycles {
> atomic_ullong n[PMD_N_CYCLES];
> };
>
>+/* Contained by struct dp_netdev_pmd_thread's 'poll_list' member. */
>+struct rxq_poll {
>+ struct dp_netdev_port *port;
>+ struct netdev_rxq *rx;
>+ struct ovs_list node;
>+};
>+
> /* PMD: Poll modes drivers. PMD accesses devices via polling to
>eliminate
> * the performance overhead of interrupt processing. Therefore netdev
>can
> * not implement rx-wait for these devices. dpif-netdev needs to poll
>@@ -393,9 +400,6 @@ struct dp_netdev_pmd_thread {
> struct ovs_refcount ref_cnt; /* Every reference must be
>refcount'ed. */
> struct cmap_node node; /* In 'dp->poll_threads'. */
>
>- pthread_cond_t cond; /* For synchronizing pmd thread
>reload. */
>- struct ovs_mutex cond_mutex; /* Mutex for condition variable. */
>-
> /* Per thread exact-match cache. Note, the instance for cpu core
> * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
> * need to be protected (e.g. by 'dp_netdev_mutex'). All other
>@@ -430,6 +434,11 @@ struct dp_netdev_pmd_thread {
> int tx_qid; /* Queue id used by this pmd thread
>to
> * send packets on all netdevs */
>
>+ struct ovs_mutex poll_mutex; /* Mutex for poll_list. */
>+ /* List of rx queues to poll. */
>+ struct ovs_list poll_list OVS_GUARDED;
>+ int poll_cnt; /* Number of elemints in poll_list.
>*/
>+
> /* Only a pmd thread can write on its own 'cycles' and 'stats'.
> * The main thread keeps 'stats_zero' and 'cycles_zero' as base
> * values and subtracts them from 'stats' and 'cycles' before
>@@ -469,7 +478,6 @@ static void dp_netdev_input(struct
>dp_netdev_pmd_thread *,
> struct dp_packet **, int cnt);
>
> static void dp_netdev_disable_upcall(struct dp_netdev *);
>-void dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd);
Assuming we're keeping this function, I think it should be made static.
This wasn't introduced by this patch, but it would be nice to fix it
anyway.
> static void dp_netdev_configure_pmd(struct dp_netdev_pmd_thread *pmd,
> struct dp_netdev *dp, int index,
> unsigned core_id, int numa_id);
>@@ -482,6 +490,11 @@ dp_netdev_pmd_get_next(struct dp_netdev *dp, struct
>cmap_position *pos);
> static void dp_netdev_destroy_all_pmds(struct dp_netdev *dp);
> static void dp_netdev_del_pmds_on_numa(struct dp_netdev *dp, int
>numa_id);
> static void dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int
>numa_id);
>+static void
>+dp_netdev_add_rxq_to_pmd(struct dp_netdev_pmd_thread *pmd,
>+ struct dp_netdev_port *port, struct netdev_rxq
>*rx);
>+static struct dp_netdev_pmd_thread *
>+dp_netdev_less_loaded_pmd_on_numa(struct dp_netdev *dp, int numa_id);
> static void dp_netdev_reset_pmd_threads(struct dp_netdev *dp);
> static bool dp_netdev_pmd_try_ref(struct dp_netdev_pmd_thread *pmd);
> static void dp_netdev_pmd_unref(struct dp_netdev_pmd_thread *pmd);
>@@ -1019,22 +1032,7 @@ dp_netdev_reload_pmd__(struct dp_netdev_pmd_thread
>*pmd)
> return;
> }
>
>- ovs_mutex_lock(&pmd->cond_mutex);
> atomic_add_relaxed(&pmd->change_seq, 1, &old_seq);
>- ovs_mutex_cond_wait(&pmd->cond, &pmd->cond_mutex);
>- ovs_mutex_unlock(&pmd->cond_mutex);
>-}
>-
>-/* Causes all pmd threads to reload its tx/rx devices.
>- * Must be called after adding/removing ports. */
>-static void
>-dp_netdev_reload_pmds(struct dp_netdev *dp)
>-{
>- struct dp_netdev_pmd_thread *pmd;
>-
>- CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>- dp_netdev_reload_pmd__(pmd);
>- }
> }
>
> static uint32_t
>@@ -1128,8 +1126,26 @@ do_add_port(struct dp_netdev *dp, const char
>*devname, const char *type,
> cmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
>
> if (netdev_is_pmd(netdev)) {
>- dp_netdev_set_pmds_on_numa(dp, netdev_get_numa_id(netdev));
>- dp_netdev_reload_pmds(dp);
>+ int numa_id = netdev_get_numa_id(netdev);
>+ struct dp_netdev_pmd_thread *pmd;
>+
>+ /* Cannot create pmd threads for invalid numa node. */
>+ ovs_assert(ovs_numa_numa_id_is_valid(numa_id));
>+
>+ for (i = 0; i < netdev_n_rxq(netdev); i++) {
>+ pmd = dp_netdev_less_loaded_pmd_on_numa(dp, numa_id);
>+ if (!pmd) {
>+ /* There is no pmd threads on this numa node. */
>+ dp_netdev_set_pmds_on_numa(dp, numa_id);
>+ /* Assigning of rx queues done. */
>+ break;
>+ }
>+
>+ ovs_mutex_lock(&pmd->poll_mutex);
>+ dp_netdev_add_rxq_to_pmd(pmd, port, port->rxq[i]);
>+ ovs_mutex_unlock(&pmd->poll_mutex);
>+ dp_netdev_reload_pmd__(pmd);
>+ }
> }
> seq_change(dp->port_seq);
>
>@@ -1226,16 +1242,6 @@ port_ref(struct dp_netdev_port *port)
> }
> }
>
>-static bool
>-port_try_ref(struct dp_netdev_port *port)
>-{
>- if (port) {
>- return ovs_refcount_try_ref_rcu(&port->ref_cnt);
>- }
>-
>- return false;
>-}
>-
> static void
> port_unref(struct dp_netdev_port *port)
> {
>@@ -1313,12 +1319,38 @@ do_del_port(struct dp_netdev *dp, struct
>dp_netdev_port *port)
> if (netdev_is_pmd(port->netdev)) {
> int numa_id = netdev_get_numa_id(port->netdev);
>
>+ /* PMD threads can not be on invalid numa node. */
>+ ovs_assert(ovs_numa_numa_id_is_valid(numa_id));
> /* If there is no netdev on the numa node, deletes the pmd
>threads
>- * for that numa. Else, just reloads the queues. */
>+ * for that numa. Else, deletes the queues from polling lists.
>*/
> if (!has_pmd_port_for_numa(dp, numa_id)) {
> dp_netdev_del_pmds_on_numa(dp, numa_id);
> }
>- dp_netdev_reload_pmds(dp);
>+ else {
This should be
} else {
on the same line
>+ struct dp_netdev_pmd_thread *pmd;
>+ struct rxq_poll *poll, *next;
>+
>+ CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
>+ if (pmd->numa_id == numa_id) {
>+ bool found = false;
>+
>+ ovs_mutex_lock(&pmd->poll_mutex);
>+ LIST_FOR_EACH_SAFE (poll, next, node,
>&pmd->poll_list) {
>+ if (poll->port == port) {
>+ found = true;
>+ port_unref(poll->port);
>+ list_remove(&poll->node);
>+ pmd->poll_cnt--;
>+ free(poll);
>+ }
>+ }
>+ ovs_mutex_unlock(&pmd->poll_mutex);
>+ if (found) {
>+ dp_netdev_reload_pmd__(pmd);
>+ }
>+ }
>+ }
>+ }
> }
>
> port_unref(port);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
