[dpdk-dev] [PATCH v11 6/7] eal: add failure handle mechanism for hot-unplug
Jeff Guo
jia.guo at intel.com
Tue Oct 2 06:01:34 CEST 2018
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.guo at intel.com>
>> Acked-by: Shaopeng He <shaopeng.he at 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
More information about the dev
mailing list