[1/3] eal: add --dev-hotplug option

Message ID 1544773540-89825-2-git-send-email-jia.guo@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series use a common eal device event for hot-unplug |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Guo, Jia Dec. 14, 2018, 7:45 a.m. UTC
  This command-line option will enable hotplug event detecting and enable
hotplug handling for device hotplug.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal.c            | 16 ++++++++++++++++
 lib/librte_eal/common/eal_common_options.c |  5 +++++
 lib/librte_eal/common/eal_internal_cfg.h   |  1 +
 lib/librte_eal/common/eal_options.h        |  4 +++-
 lib/librte_eal/linuxapp/eal/eal.c          | 16 ++++++++++++++++
 5 files changed, 41 insertions(+), 1 deletion(-)
  

Comments

David Marchand Dec. 17, 2018, 10:15 a.m. UTC | #1
On Fri, Dec 14, 2018 at 8:41 AM Jeff Guo <jia.guo@intel.com> wrote:

> This command-line option will enable hotplug event detecting and enable
> hotplug handling for device hotplug.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>

Is there a reason why we would want this disabled by default and enabled
via option ?


diff --git a/lib/librte_eal/common/eal_options.h
> b/lib/librte_eal/common/eal_options.h
> index 5271f94..4d8a12e 100644
> --- a/lib/librte_eal/common/eal_options.h
> +++ b/lib/librte_eal/common/eal_options.h
> @@ -65,7 +65,9 @@ enum {
>         OPT_SINGLE_FILE_SEGMENTS_NUM,
>  #define OPT_IOVA_MODE          "iova-mode"
>         OPT_IOVA_MODE_NUM,
> -       OPT_LONG_MAX_NUM
> +       OPT_LONG_MAX_NUM,
> +#define OPT_DEV_HOTPLUG              "dev-hotplug"
> +       OPT_DEV_HOTPLUG_NUM,
>  };
>
>  extern const char eal_short_options[];
>

OPT_LONG_MAX_NUM is supposed to be the last enum.
  
Guo, Jia Dec. 29, 2018, 4:06 a.m. UTC | #2
hi, david

On 12/17/2018 6:15 PM, David Marchand wrote:
>
> On Fri, Dec 14, 2018 at 8:41 AM Jeff Guo <jia.guo@intel.com 
> <mailto:jia.guo@intel.com>> wrote:
>
>     This command-line option will enable hotplug event detecting and
>     enable
>     hotplug handling for device hotplug.
>
>     Signed-off-by: Jeff Guo <jia.guo@intel.com <mailto:jia.guo@intel.com>>
>
>
> Is there a reason why we would want this disabled by default and 
> enabled via option ?
>
>

Before i can give you an answer, let's see what will bring on if enable it.

When enable the hotplug will means that it will bring a new netlink 
socket communication

and a sigbus detecting and specific processing. So if user not want to 
add this work load, just

let it to be optional. Do you agree with that? If not please show what 
is your concern. Thanks.


>     diff --git a/lib/librte_eal/common/eal_options.h
>     b/lib/librte_eal/common/eal_options.h
>     index 5271f94..4d8a12e 100644
>     --- a/lib/librte_eal/common/eal_options.h
>     +++ b/lib/librte_eal/common/eal_options.h
>     @@ -65,7 +65,9 @@ enum {
>             OPT_SINGLE_FILE_SEGMENTS_NUM,
>      #define OPT_IOVA_MODE          "iova-mode"
>             OPT_IOVA_MODE_NUM,
>     -       OPT_LONG_MAX_NUM
>     +       OPT_LONG_MAX_NUM,
>     +#define OPT_DEV_HOTPLUG              "dev-hotplug"
>     +       OPT_DEV_HOTPLUG_NUM,
>      };
>
>      extern const char eal_short_options[];
>
>
> OPT_LONG_MAX_NUM is supposed to be the last enum.
>
> -- 
> David Marchand
  
David Marchand Jan. 2, 2019, 2:46 p.m. UTC | #3
Hello Jeff,

On Sat, Dec 29, 2018 at 5:06 AM Jeff Guo <jia.guo@intel.com> wrote:

> On 12/17/2018 6:15 PM, David Marchand wrote:
>
>
> On Fri, Dec 14, 2018 at 8:41 AM Jeff Guo <jia.guo@intel.com> wrote:
>
>> This command-line option will enable hotplug event detecting and enable
>> hotplug handling for device hotplug.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>
>
> Is there a reason why we would want this disabled by default and enabled
> via option ?
>
>
> Before i can give you an answer, let's see what will bring on if enable
> it.
>
> When enable the hotplug will means that it will bring a new netlink socket
> communication
>
> and a sigbus detecting and specific processing. So if user not want to add
> this work load, just
>
> let it to be optional. Do you agree with that? If not please show what is
> your concern. Thanks.
>

If the user does nothing about the sigbus signal handling but the eal
signal handler was not registered, the dpdk app will end up being
terminated by the kernel.
If the user wants to do its own things and don't want the eal to mess with
it... I am under the impression that he can disable the eal sigbus handler
by calling rte_dev_hotplug_handle_disable().
The netlink stuff is handled in the interrupt thread, no impact on the
processing threads and no additional thread afaics.
  

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index b8152a7..b9c29e4 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -699,6 +699,22 @@  rte_eal_init(int argc, char **argv)
 #endif
 	}
 
+	if (internal_config.dev_hotplug) {
+		ret = rte_dev_hotplug_handle_enable();
+		if (ret) {
+			RTE_LOG(ERR, EAL,
+				"fail to enable hotplug handling.");
+			return -1;
+		}
+
+		ret = rte_dev_event_monitor_start();
+		if (ret) {
+			RTE_LOG(ERR, EAL,
+				"fail to start device event monitoring.");
+			return -1;
+		}
+	}
+
 	rte_srand(rte_rdtsc());
 
 	/* in secondary processes, memory init may allocate additional fbarrays
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index e31eca5..70cb374 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -79,6 +79,7 @@  eal_long_options[] = {
 	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
 	{OPT_LEGACY_MEM,        0, NULL, OPT_LEGACY_MEM_NUM       },
 	{OPT_SINGLE_FILE_SEGMENTS, 0, NULL, OPT_SINGLE_FILE_SEGMENTS_NUM},
+	{OPT_DEV_HOTPLUG,       0, NULL, OPT_DEV_HOTPLUG_NUM      },
 	{0,                     0, NULL, 0                        }
 };
 
@@ -1309,6 +1310,9 @@  eal_parse_common_option(int opt, const char *optarg,
 			return -1;
 		}
 		break;
+	case OPT_DEV_HOTPLUG_NUM:
+		conf->dev_hotplug = 1;
+		break;
 
 	/* don't know what to do, leave this to caller */
 	default:
@@ -1476,6 +1480,7 @@  eal_common_usage(void)
 	       "  -h, --help          This help\n"
 	       "  --"OPT_IN_MEMORY"   Operate entirely in memory. This will\n"
 	       "                      disable secondary process support\n"
+	       "  --"OPT_DEV_HOTPLUG" Enable device hotplug\n"
 	       "\nEAL options for DEBUG use only:\n"
 	       "  --"OPT_HUGE_UNLINK"       Unlink hugepage files after init\n"
 	       "  --"OPT_NO_HUGE"           Use malloc instead of hugetlbfs\n"
diff --git a/lib/librte_eal/common/eal_internal_cfg.h b/lib/librte_eal/common/eal_internal_cfg.h
index 737f17e..d160ee3 100644
--- a/lib/librte_eal/common/eal_internal_cfg.h
+++ b/lib/librte_eal/common/eal_internal_cfg.h
@@ -73,6 +73,7 @@  struct internal_config {
 	enum rte_iova_mode iova_mode ;    /**< Set IOVA mode on this system  */
 	volatile unsigned int init_complete;
 	/**< indicates whether EAL has completed initialization */
+	volatile unsigned dev_hotplug;    /**< true to enable device hotplug */
 };
 extern struct internal_config internal_config; /**< Global EAL configuration. */
 
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index 5271f94..4d8a12e 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -65,7 +65,9 @@  enum {
 	OPT_SINGLE_FILE_SEGMENTS_NUM,
 #define OPT_IOVA_MODE          "iova-mode"
 	OPT_IOVA_MODE_NUM,
-	OPT_LONG_MAX_NUM
+	OPT_LONG_MAX_NUM,
+#define OPT_DEV_HOTPLUG	      "dev-hotplug"
+	OPT_DEV_HOTPLUG_NUM,
 };
 
 extern const char eal_short_options[];
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 361744d..8de7401 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -960,6 +960,22 @@  rte_eal_init(int argc, char **argv)
 #endif
 	}
 
+	if (internal_config.dev_hotplug) {
+		ret = rte_dev_hotplug_handle_enable();
+		if (ret) {
+			RTE_LOG(ERR, EAL,
+				"fail to enable hotplug handling.");
+			return -1;
+		}
+
+		ret = rte_dev_event_monitor_start();
+		if (ret) {
+			RTE_LOG(ERR, EAL,
+				"fail to start device event monitoring.");
+			return -1;
+		}
+	}
+
 	rte_srand(rte_rdtsc());
 
 	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0) {