hi, constantin

On 10/1/2018 3:46 AM, Ananyev, Konstantin wrote:
Hi Jeff,

The mechanism can initially register the sigbus handler after the device
event monitor is enabled. When a sigbus event is captured, it will check
the failure address and accordingly handle the memory failure of the
corresponding device by invoke the hot-unplug handler. It could prevent
the application from crashing when a device is hot-unplugged.

By this patch, users could call below new added APIs to enable/disable
the device hotplug handle mechanism. Note that it just implement the
hot-unplug handler in these functions, the other handler of hotplug, such
as handler for hotplug binding, could be add in the future if need:
   - rte_dev_hotplug_handle_enable
   - rte_dev_hotplug_handle_disable

Signed-off-by: Jeff Guo <jia....@intel.com>
Acked-by: Shaopeng He <shaopeng...@intel.com>
---
v11->v10:
change some words and change the invoked func name.
add experimental tag
---
  doc/guides/rel_notes/release_18_08.rst  |   5 +
  lib/librte_eal/bsdapp/eal/eal_dev.c     |  14 +++
  lib/librte_eal/common/eal_private.h     |  26 +++++
  lib/librte_eal/common/include/rte_dev.h |  26 +++++
  lib/librte_eal/linuxapp/eal/eal_dev.c   | 162 +++++++++++++++++++++++++++++++-
  lib/librte_eal/rte_eal_version.map      |   2 +
  6 files changed, 234 insertions(+), 1 deletion(-)

diff --git a/doc/guides/rel_notes/release_18_08.rst 
b/doc/guides/rel_notes/release_18_08.rst
index 321fa84..fe0e60f 100644
--- a/doc/guides/rel_notes/release_18_08.rst
+++ b/doc/guides/rel_notes/release_18_08.rst
@@ -117,6 +117,11 @@ New Features

    Added support for chained mbufs (input and output).

+* **Added hot-unplug handle mechanism.**
+
+  ``rte_dev_hotplug_handle_enable`` and ``rte_dev_hotplug_handle_disable`` are
+  for enabling or disabling hotplug handle mechanism.
+

  API Changes
  -----------
diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c 
b/lib/librte_eal/bsdapp/eal/eal_dev.c
index 1c6c51b..255d611 100644
--- a/lib/librte_eal/bsdapp/eal/eal_dev.c
+++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
@@ -19,3 +19,17 @@ rte_dev_event_monitor_stop(void)
        RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
        return -1;
  }
+
+int __rte_experimental
+rte_dev_hotplug_handle_enable(void)
+{
+       RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
+       return -1;
+}
+
+int __rte_experimental
+rte_dev_hotplug_handle_disable(void)
+{
+       RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
+       return -1;
+}
diff --git a/lib/librte_eal/common/eal_private.h 
b/lib/librte_eal/common/eal_private.h
index a2d1528..637f20d 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -317,4 +317,30 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
   */
  int rte_bus_sigbus_handler(const void *failure_addr);

+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Register the sigbus handler.
+ *
+ * @return
+ *   - On success, zero.
+ *   - On failure, a negative value.
+ */
+int
+rte_dev_sigbus_handler_register(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unregister the sigbus handler.
+ *
+ * @return
+ *   - On success, zero.
+ *   - On failure, a negative value.
+ */
+int
+rte_dev_sigbus_handler_unregister(void);
+
  #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/common/include/rte_dev.h 
b/lib/librte_eal/common/include/rte_dev.h
index b80a805..ff580a0 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -460,4 +460,30 @@ rte_dev_event_monitor_start(void);
  int __rte_experimental
  rte_dev_event_monitor_stop(void);

+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Enable hotplug handling for devices.
+ *
+ * @return
+ *   - On success, zero.
+ *   - On failure, a negative value.
+ */
+int __rte_experimental
+rte_dev_hotplug_handle_enable(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Disable hotplug handling for devices.
+ *
+ * @return
+ *   - On success, zero.
+ *   - On failure, a negative value.
+ */
+int __rte_experimental
+rte_dev_hotplug_handle_disable(void);
+
  #endif /* _RTE_DEV_H_ */
diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c 
b/lib/librte_eal/linuxapp/eal/eal_dev.c
index 1cf6aeb..14b18d8 100644
--- a/lib/librte_eal/linuxapp/eal/eal_dev.c
+++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
@@ -4,6 +4,8 @@

  #include <string.h>
  #include <unistd.h>
+#include <fcntl.h>
+#include <signal.h>
  #include <sys/socket.h>
  #include <linux/netlink.h>

@@ -14,15 +16,32 @@
  #include <rte_malloc.h>
  #include <rte_interrupts.h>
  #include <rte_alarm.h>
+#include <rte_bus.h>
+#include <rte_eal.h>
+#include <rte_spinlock.h>
+#include <rte_errno.h>

  #include "eal_private.h"

  static struct rte_intr_handle intr_handle = {.fd = -1 };
  static bool monitor_started;
+static bool hotplug_handle;

  #define EAL_UEV_MSG_LEN 4096
  #define EAL_UEV_MSG_ELEM_LEN 128

+/*
+ * spinlock for device hot-unplug failure handling. If it try to access bus or
+ * device, such as handle sigbus on bus or handle memory failure for device
+ * just need to use this lock. It could protect the bus and the device to avoid
+ * race condition.
+ */
+static rte_spinlock_t failure_handle_lock = RTE_SPINLOCK_INITIALIZER;
+
+static struct sigaction sigbus_action_old;
+
+static int sigbus_need_recover;
+
  static void dev_uev_handler(__rte_unused void *param);

  /* identify the system layer which reports this event. */
@@ -33,6 +52,49 @@ enum eal_dev_event_subsystem {
        EAL_DEV_EVENT_SUBSYSTEM_MAX
  };

+static void
+sigbus_action_recover(void)
+{
+       if (sigbus_need_recover) {
+               sigaction(SIGBUS, &sigbus_action_old, NULL);
+               sigbus_need_recover = 0;
+       }
+}
+
+static void sigbus_handler(int signum, siginfo_t *info,
+                               void *ctx __rte_unused)
+{
+       int ret;
+
+       RTE_LOG(INFO, EAL, "Thread[%d] catch SIGBUS, fault address:%p\n",
+               (int)pthread_self(), info->si_addr);
+
+       rte_spinlock_lock(&failure_handle_lock);
+       ret = rte_bus_sigbus_handler(info->si_addr);
+       rte_spinlock_unlock(&failure_handle_lock);
+       if (ret == -1) {
+               rte_exit(EXIT_FAILURE,
+                        "Failed to handle SIGBUS for hot-unplug, "
+                        "(rte_errno: %s)!", strerror(rte_errno));
+       } else if (ret == 1) {
+               if (sigbus_action_old.sa_handler)
+                       (*(sigbus_action_old.sa_handler))(signum);
+               else
+                       rte_exit(EXIT_FAILURE,
+                                "Failed to handle generic SIGBUS!");
+       }
+
+       RTE_LOG(INFO, EAL, "Success to handle SIGBUS for hot-unplug!\n");
+}
+
+static int cmp_dev_name(const struct rte_device *dev,
+       const void *_name)
+{
+       const char *name = _name;
+
+       return strcmp(dev->name, name);
+}
+
  static int
  dev_uev_socket_fd_create(void)
  {
@@ -147,6 +209,9 @@ dev_uev_handler(__rte_unused void *param)
        struct rte_dev_event uevent;
        int ret;
        char buf[EAL_UEV_MSG_LEN];
+       struct rte_bus *bus;
+       struct rte_device *dev;
+       const char *busname = "";

        memset(&uevent, 0, sizeof(struct rte_dev_event));
        memset(buf, 0, EAL_UEV_MSG_LEN);
@@ -171,8 +236,43 @@ dev_uev_handler(__rte_unused void *param)
        RTE_LOG(DEBUG, EAL, "receive uevent(name:%s, type:%d, subsystem:%d)\n",
                uevent.devname, uevent.type, uevent.subsystem);

-       if (uevent.devname)
+       switch (uevent.subsystem) {
+       case EAL_DEV_EVENT_SUBSYSTEM_PCI:
+       case EAL_DEV_EVENT_SUBSYSTEM_UIO:
+               busname = "pci";
+               break;
+       default:
+               break;
+       }
+
+       if (uevent.devname) {
+               if (uevent.type == RTE_DEV_EVENT_REMOVE && hotplug_handle) {
+                       rte_spinlock_lock(&failure_handle_lock);
+                       bus = rte_bus_find_by_name(busname);
+                       if (bus == NULL) {
+                               RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n",
+                                       busname);
+                               return;
+                       }
+
+                       dev = bus->find_device(NULL, cmp_dev_name,
+                                              uevent.devname);
+                       if (dev == NULL) {
+                               RTE_LOG(ERR, EAL, "Cannot find device (%s) on "
+                                       "bus (%s)\n", uevent.devname, busname);
+                               return;
+                       }
+
+                       ret = bus->hot_unplug_handler(dev);
+                       rte_spinlock_unlock(&failure_handle_lock);
+                       if (ret) {
+                               RTE_LOG(ERR, EAL, "Can not handle hot-unplug "
+                                       "for device (%s)\n", dev->name);
+                               return;
+                       }
+               }
                dev_callback_process(uevent.devname, uevent.type);
+       }
  }

  int __rte_experimental
@@ -220,5 +320,65 @@ rte_dev_event_monitor_stop(void)
        close(intr_handle.fd);
        intr_handle.fd = -1;
        monitor_started = false;
+
        return 0;
  }
+
+int __rte_experimental
+rte_dev_sigbus_handler_register(void)
+{
+       sigset_t mask;
+       struct sigaction action;
+
+       rte_errno = 0;
+
Shouldn't you call sigaction only if sigbus_need_recover == 0?


i guess what you mean is that the sigbus_need_recover is need check before call sigaction, because the register could be call many times, if so, i think it is make sense to check it every time.


+       sigemptyset(&mask);
+       sigaddset(&mask, SIGBUS);
+       action.sa_flags = SA_SIGINFO;
+       action.sa_mask = mask;
+       action.sa_sigaction = sigbus_handler;
+       sigbus_need_recover = !sigaction(SIGBUS, &action, &sigbus_action_old);
+
+       return rte_errno;
+}
+
+int __rte_experimental
+rte_dev_sigbus_handler_unregister(void)
+{
+       rte_errno = 0;
+       sigbus_need_recover = 1;
Hmm, why to set sugbus_need_recover to 1 here?
If user called rte_dev_sigbus_handler_register() before, and it was successful, 
it already would be 1.
In other cases, you probably don't have to do anything.
Konstantin


you are right, it should let each sigaction calling to manage this macro but no other more place, thanks.


+
+       sigbus_action_recover();
+
+       return rte_errno;
+}
+
+int __rte_experimental
+rte_dev_hotplug_handle_enable(void)
+{
+       int ret = 0;
+
+       ret = rte_dev_sigbus_handler_register();
+       if (ret < 0)
+               RTE_LOG(ERR, EAL, "fail to register sigbus handler for "
+                       "devices.\n");
+
+       hotplug_handle = true;
+
+       return ret;
+}
+
+int __rte_experimental
+rte_dev_hotplug_handle_disable(void)
+{
+       int ret = 0;
+
+       ret = rte_dev_sigbus_handler_unregister();
+       if (ret < 0)
+               RTE_LOG(ERR, EAL, "fail to unregister sigbus handler for "
+                       "devices.\n");
+
+       hotplug_handle = false;
+
+       return ret;
+}
diff --git a/lib/librte_eal/rte_eal_version.map 
b/lib/librte_eal/rte_eal_version.map
index 73282bb..a3255aa 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -281,6 +281,8 @@ EXPERIMENTAL {
        rte_dev_event_callback_unregister;
        rte_dev_event_monitor_start;
        rte_dev_event_monitor_stop;
+       rte_dev_hotplug_handle_enable;
+       rte_dev_hotplug_handle_disable;
        rte_dev_iterator_init;
        rte_dev_iterator_next;
        rte_devargs_add;
--
2.7.4

Reply via email to