dp_netdev_get_pmd() is allowed to return NULL (even if we call it with
NON_PMD_CORE_ID) for different reasons:
* Since we use RCU to protect pmd threads, it is possible that
ovs_refcount_try_ref_rcu() has failed.
* During reconfiguration we destroy every thread.
This commit makes sure that we always handle the case when
dp_netdev_get_pmd() returns NULL without crashing (the change in
dpif_netdev_run() doesn't fix anything, because everything is happening
in the main thread, but it's better to honor the interface in case we
change our threading model).
This actually fixes a pretty serious crash that happens if
dpif_netdev_execute() is called from a non pmd thread while
reconfiguration is happening. It can be triggered by enabling bfd
(because it's handled by the monitor thread, which is a non pmd thread)
on an interface and changing something that requires datapath
reconfiguration (n_rxq, pmd-cpu-mask, mtu).
A testcase that reproduces the race condition is included.
This is a possible backtrace of the segfault:
#0 0x000000000060c7f1 in dp_execute_cb (aux_=0x7f1dd2d2a320,
packets_=0x7f1dd2d2a370, a=0x7f1dd2d2a658, may_steal=false) at
../lib/dpif-netdev.c:4357
#1 0x00000000006448b2 in odp_execute_actions (dp=0x7f1dd2d2a320,
batch=0x7f1dd2d2a370, steal=false, actions=0x7f1dd2d2a658,
actions_len=8,
dp_execute_action=0x60c7a5 <dp_execute_cb>) at
../lib/odp-execute.c:538
#2 0x000000000060d00c in dp_netdev_execute_actions (pmd=0x0,
packets=0x7f1dd2d2a370, may_steal=false, flow=0x7f1dd2d2ae70,
actions=0x7f1dd2d2a658, actions_len=8,
now=
44965873) at ../lib/dpif-netdev.c:4577
#3 0x000000000060834a in dpif_netdev_execute (dpif=0x2b67b70,
execute=0x7f1dd2d2a578) at ../lib/dpif-netdev.c:2624
#4 0x0000000000608441 in dpif_netdev_operate (dpif=0x2b67b70,
ops=0x7f1dd2d2a5c8, n_ops=1) at ../lib/dpif-netdev.c:2654
#5 0x0000000000610a30 in dpif_operate (dpif=0x2b67b70,
ops=0x7f1dd2d2a5c8, n_ops=1) at ../lib/dpif.c:1268
#6 0x000000000061098c in dpif_execute (dpif=0x2b67b70,
execute=0x7f1dd2d2aa50) at ../lib/dpif.c:1233
#7 0x00000000005b9008 in ofproto_dpif_execute_actions__
(ofproto=0x2b69360, version=
18446744073709551614, flow=0x7f1dd2d2ae70,
rule=0x0, ofpacts=0x7f1dd2d2b100,
ofpacts_len=16, indentation=0, depth=0, resubmits=0,
packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif.c:3806
#8 0x00000000005b907a in ofproto_dpif_execute_actions
(ofproto=0x2b69360, version=
18446744073709551614, flow=0x7f1dd2d2ae70,
rule=0x0, ofpacts=0x7f1dd2d2b100,
ofpacts_len=16, packet=0x7f1dd2d2b5c0) at
../ofproto/ofproto-dpif.c:3823
#9 0x00000000005dea9b in xlate_send_packet (ofport=0x2b98380,
oam=false, packet=0x7f1dd2d2b5c0) at
../ofproto/ofproto-dpif-xlate.c:5792
#10 0x00000000005bab12 in ofproto_dpif_send_packet (ofport=0x2b98380,
oam=false, packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif.c:4628
#11 0x00000000005c3fc8 in monitor_mport_run (mport=0x2b8cd00,
packet=0x7f1dd2d2b5c0) at ../ofproto/ofproto-dpif-monitor.c:287
#12 0x00000000005c3d9b in monitor_run () at
../ofproto/ofproto-dpif-monitor.c:227
#13 0x00000000005c3cab in monitor_main (args=0x0) at
../ofproto/ofproto-dpif-monitor.c:189
#14 0x00000000006a183a in ovsthread_wrapper (aux_=0x2b8afd0) at
../lib/ovs-thread.c:342
#15 0x00007f1dd75eb444 in start_thread (arg=0x7f1dd2d2c700) at
pthread_create.c:333
#16 0x00007f1dd6e1d20d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Acked-by: Ben Pfaff <blp@ovn.org>
pmd = ovsthread_getspecific(dp->per_pmd_key);
if (!pmd) {
pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID);
+ if (!pmd) {
+ return EBUSY;
+ }
}
/* If the current thread is non-pmd thread, acquires
{
struct dp_netdev_port *port;
struct dp_netdev *dp = get_dp_netdev(dpif);
- struct dp_netdev_pmd_thread *non_pmd = dp_netdev_get_pmd(dp,
- NON_PMD_CORE_ID);
+ struct dp_netdev_pmd_thread *non_pmd;
uint64_t new_tnl_seq;
ovs_mutex_lock(&dp->port_mutex);
- ovs_mutex_lock(&dp->non_pmd_mutex);
- HMAP_FOR_EACH (port, node, &dp->ports) {
- if (!netdev_is_pmd(port->netdev)) {
- int i;
+ non_pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID);
+ if (non_pmd) {
+ ovs_mutex_lock(&dp->non_pmd_mutex);
+ HMAP_FOR_EACH (port, node, &dp->ports) {
+ if (!netdev_is_pmd(port->netdev)) {
+ int i;
- for (i = 0; i < port->n_rxq; i++) {
- dp_netdev_process_rxq_port(non_pmd, port, port->rxqs[i].rxq);
+ for (i = 0; i < port->n_rxq; i++) {
+ dp_netdev_process_rxq_port(non_pmd, port,
+ port->rxqs[i].rxq);
+ }
}
}
- }
- dpif_netdev_xps_revalidate_pmd(non_pmd, time_msec(), false);
- ovs_mutex_unlock(&dp->non_pmd_mutex);
+ dpif_netdev_xps_revalidate_pmd(non_pmd, time_msec(), false);
+ ovs_mutex_unlock(&dp->non_pmd_mutex);
- dp_netdev_pmd_unref(non_pmd);
+ dp_netdev_pmd_unref(non_pmd);
+ }
if (dp_netdev_is_reconf_required(dp) || ports_require_restart(dp)) {
reconfigure_pmd_threads(dp);
}
/* Finds and refs the dp_netdev_pmd_thread on core 'core_id'. Returns
- * the pointer if succeeds, otherwise, NULL.
+ * the pointer if succeeds, otherwise, NULL (it can return NULL even if
+ * 'core_id' is NON_PMD_CORE_ID).
*
* Caller must unrefs the returned reference. */
static struct dp_netdev_pmd_thread *
OVS_VSWITCHD_STOP(["/dpif_netdev|WARN|There is no PMD thread on core/d"])
AT_CLEANUP
+
+AT_SETUP([PMD - monitor threads])
+OVS_VSWITCHD_START(
+ [], [], [], [--dummy-numa 0,0])
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+dnl The two devices are connected together externally using net.sock
+AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd ofport_request=1 options:n_rxq=1 options:pstream=punix:$OVS_RUNDIR/net.sock])
+AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=dummy-pmd ofport_request=2 options:n_rxq=1 options:stream=unix:$OVS_RUNDIR/net.sock])
+
+AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | parse_pmd_rxq_show], [0], [dnl
+p1 0 0 0
+p2 0 0 0
+])
+
+dnl Enable bfd with a very aggressive interval. This will make the monitor very
+dnl busy, and uncover race conditions with the main thread.
+AT_CHECK([ovs-vsctl set Interface p1 bfd:enable=true bfd:min_rx=1 bfd:min_tx=1])
+AT_CHECK([ovs-vsctl set Interface p2 bfd:enable=true bfd:min_rx=1 bfd:min_tx=1])
+
+AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p1 bfd_status:forwarding=true \
+ -- wait-until Interface p2 bfd_status:forwarding=true])
+
+dnl Trigger reconfiguration of the datapath
+AT_CHECK([ovs-vsctl set Interface p1 options:n_rxq=2])
+AT_CHECK([ovs-vsctl set Interface p2 options:n_rxq=2])
+
+dnl Make sure that reconfiguration succeded
+AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | parse_pmd_rxq_show], [0], [dnl
+p1 0 0 0
+p1 1 0 0
+p2 0 0 0
+p2 1 0 0
+])
+
+dnl During reconfiguration some packets will be dropped. This is expected
+OVS_VSWITCHD_STOP(["/dpif(monitor[[0-9]]\+)|WARN|dummy@ovs-dummy: execute [[0-9]]\+ failed/d"])
+AT_CLEANUP