[dpdk-dev,05/22] ethdev: introduce device lock

Message ID 20180607123849.14439-6-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series enable hotplug on multi-process |

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Qi Zhang June 7, 2018, 12:38 p.m. UTC
  Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let
application lock or unlock on specific ethdev, a locked device
can't be detached, this help applicaiton to prevent unexpected
device detaching, especially in multi-process envrionment.

Aslo the new API let application to register a callback function
which will be invoked before a device is going to be detached,
the return value of the function will decide if device will continue
be detached or not, this support application to do condition check
at runtime.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_ethdev/Makefile          |   1 +
 lib/librte_ethdev/rte_ethdev.c      |  41 ++++++++++++++-
 lib/librte_ethdev/rte_ethdev.h      |  64 ++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_lock.c | 102 ++++++++++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_lock.h |  23 ++++++++
 lib/librte_ethdev/rte_ethdev_mp.c   |   3 +-
 6 files changed, 232 insertions(+), 2 deletions(-)
 create mode 100644 lib/librte_ethdev/rte_ethdev_lock.c
 create mode 100644 lib/librte_ethdev/rte_ethdev_lock.h
  

Comments

Burakov, Anatoly June 15, 2018, 3:42 p.m. UTC | #1
On 07-Jun-18 1:38 PM, Qi Zhang wrote:
> Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let
> application lock or unlock on specific ethdev, a locked device
> can't be detached, this help applicaiton to prevent unexpected
> device detaching, especially in multi-process envrionment.
> 
> Aslo the new API let application to register a callback function
> which will be invoked before a device is going to be detached,
> the return value of the function will decide if device will continue
> be detached or not, this support application to do condition check
> at runtime.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---

<snip>

> +
> +int
> +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> +		 void *user_args)
> +{
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (callback == NULL)
> +		return register_lock_callback(port_id, dev_is_busy, NULL);
> +	else
> +		return register_lock_callback(port_id, callback, user_args);

As much as i don't like seeing negative errno values as return, the rest 
of ethdev library uses those, so this is OK :)

> +}
> +
> +int
> +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> +		   void *user_args)
> +{
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +

<snip>

> + * Also, any callback function return !0 value will prevent device be
> + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param user_args
> + *   This is parameter "user_args" be saved when callback function is
> + *   registered(rte_dev_eth_lock).
> + *
> + * @return
> + *   0  device is allowed be detached.
> + *   !0 device is not allowed be detached.

!0 can be negative or positive. Are we expecting positive return values 
from this API?

> + */
> +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void *user_args);
> +
> +/**
> + * Lock an Ethernet Device directly or register a callback function
> + * for condition check at runtime, this help application to prevent
> + * a device be detached unexpectly.
> + * NOTE: Lock a device mutliple times with same parmeter will increase
> + * a ref_count, and coresponding unlock decrease the ref_count, the
> + * device will be unlocked when ref_count reach 0.

Nitpick: "note" sections should be done with @note marker.

Also, i would mention that this is a per-process lock that does not 
affect other processes (assuming i understood the code correctly, of 
course...).

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param callback
> + *   !NULL the callback function will be added into a pre-detach list,
> + *         it will be invoked when a device is going to be detached. The
> + *         return value will decide if continue detach the device or not.
> + *   NULL  lock the device directly, basically this just regiter a empty
> + *         callback function(dev_is_busy) that return -EBUSY, so we can
> + *         handle the pre-detach check in unified way.
> + * @param user_args
> + *   parameter will be parsed to callback function, only valid when
> + *   callback != NULL.
> + * @return
> + *   0 on success, negative on error.
> + */
> +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> +		     void *user_args);

Nitpicks: DPDK style guide discourages using spaces as indentation 
(other parts of this patch, and other patches have this issue as well).

> +
> +/**
> + * Reverse operation of rte_eth_dev_lock.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param callback
> + *   NULL  decrease the ref_count of default callback function.
> + *   !NULL decrease the ref_count of specific callback with matched
> + *         user_args.
> + * @param user_args
> + *   parameter to match, only valid when callback != NULL.
> + * @return
> + *   0 on success, negative on error.
> + */
> +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> +		       void *user_args);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/librte_ethdev/rte_ethdev_lock.c b/lib/librte_ethdev/rte_ethdev_lock.c

rte_ethdev_lock.* seem to be internal-only files. Perhaps you should 
name them without the rte_ prefix to indicate that they're not exported?

> new file mode 100644
> index 000000000..688d1d70a
> --- /dev/null
> +++ b/lib/librte_ethdev/rte_ethdev_lock.c
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +#include "rte_ethdev_lock.h"
> +
> +struct lock_entry {
> +	TAILQ_ENTRY(lock_entry) next;
> +	rte_eth_dev_lock_callback_t callback;
> +	uint16_t port_id;

<snip>

> +register_lock_callback(uint16_t port_id,
> +		       rte_eth_dev_lock_callback_t callback,
> +		       void *user_args)
> +{
> +	struct lock_entry *le;
> +
> +	rte_spinlock_lock(&lock_entry_lock);
> +
> +	TAILQ_FOREACH(le, &lock_entry_list, next) {
> +		if (le->port_id == port_id &&
> +		    le->callback == callback &&
> +		    le->user_args == user_args)
> +			break;
> +	}
> +
> +	if (!le) {
> +		le = calloc(1, sizeof(struct lock_entry));
> +		if (!le) {

Nitpick: generally, DPDK style guide prefers "if (value)" or "if 
(!value)" to only be reserved for boolean values, and use explicit 
comparison (e.g. "if (value == NULL)" or "if (value == 0)") for all 
other cases.
  
Stephen Hemminger June 15, 2018, 4:09 p.m. UTC | #2
On Thu,  7 Jun 2018 20:38:32 +0800
Qi Zhang <qi.z.zhang@intel.com> wrote:

> +/**
> + * Lock an Ethernet Device directly or register a callback function
> + * for condition check at runtime, this help application to prevent
> + * a device be detached unexpectly.
> + * NOTE: Lock a device mutliple times with same parmeter will increase
> + * a ref_count, and coresponding unlock decrease the ref_count, the
> + * device will be unlocked when ref_count reach 0.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param callback
> + *   !NULL the callback function will be added into a pre-detach list,
> + *         it will be invoked when a device is going to be detached. The
> + *         return value will decide if continue detach the device or not.
> + *   NULL  lock the device directly, basically this just regiter a empty
> + *         callback function(dev_is_busy) that return -EBUSY, so we can
> + *         handle the pre-detach check in unified way.
> + * @param user_args
> + *   parameter will be parsed to callback function, only valid when
> + *   callback != NULL.
> + * @return
> + *   0 on success, negative on error.
> + */
> +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> +		     void *user_args);

I prefer API's that do one thing with one function.
Why not
	rte_eth_dev_lock(uint16_t port_id);
	rte_eth_dev_ondetach(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
           		     void *user_args);
  
Qi Zhang June 19, 2018, 2:16 p.m. UTC | #3
Hi Stephen:


> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, June 16, 2018 12:09 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock
> 
> On Thu,  7 Jun 2018 20:38:32 +0800
> Qi Zhang <qi.z.zhang@intel.com> wrote:
> 
> > +/**
> > + * Lock an Ethernet Device directly or register a callback function
> > + * for condition check at runtime, this help application to prevent
> > + * a device be detached unexpectly.
> > + * NOTE: Lock a device mutliple times with same parmeter will increase
> > + * a ref_count, and coresponding unlock decrease the ref_count, the
> > + * device will be unlocked when ref_count reach 0.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param callback
> > + *   !NULL the callback function will be added into a pre-detach list,
> > + *         it will be invoked when a device is going to be detached. The
> > + *         return value will decide if continue detach the device or not.
> > + *   NULL  lock the device directly, basically this just regiter a empty
> > + *         callback function(dev_is_busy) that return -EBUSY, so we can
> > + *         handle the pre-detach check in unified way.
> > + * @param user_args
> > + *   parameter will be parsed to callback function, only valid when
> > + *   callback != NULL.
> > + * @return
> > + *   0 on success, negative on error.
> > + */
> > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
> > +		     void *user_args);
> 
> I prefer API's that do one thing with one function.

Agree

> Why not
> 	rte_eth_dev_lock(uint16_t port_id);
> 	rte_eth_dev_ondetach(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
>            		     void *user_args);

Rte_eth_dev_ondetach looks like a callback function, 
but this is the function to register some condition check.
How about rte_eth_dev_lock and rte_eth_dev_lock_with_cond?

Thanks
Qi
  
Qi Zhang June 20, 2018, 4 a.m. UTC | #4
Hi Anatoly:
	Sorry to miss this email and reply late.

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Friday, June 15, 2018 11:43 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [PATCH 05/22] ethdev: introduce device lock
> 
> On 07-Jun-18 1:38 PM, Qi Zhang wrote:
> > Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let
> > application lock or unlock on specific ethdev, a locked device can't
> > be detached, this help applicaiton to prevent unexpected device
> > detaching, especially in multi-process envrionment.
> >
> > Aslo the new API let application to register a callback function which
> > will be invoked before a device is going to be detached, the return
> > value of the function will decide if device will continue be detached
> > or not, this support application to do condition check at runtime.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> 
> <snip>
> 
> > +
> > +int
> > +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> > +		 void *user_args)
> > +{
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> > +	if (callback == NULL)
> > +		return register_lock_callback(port_id, dev_is_busy, NULL);
> > +	else
> > +		return register_lock_callback(port_id, callback, user_args);
> 
> As much as i don't like seeing negative errno values as return, the rest of
> ethdev library uses those, so this is OK :)
> 
> > +}
> > +
> > +int
> > +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
> > +		   void *user_args)
> > +{
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> 
> <snip>
> 
> > + * Also, any callback function return !0 value will prevent device be
> > + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param user_args
> > + *   This is parameter "user_args" be saved when callback function is
> > + *   registered(rte_dev_eth_lock).
> > + *
> > + * @return
> > + *   0  device is allowed be detached.
> > + *   !0 device is not allowed be detached.
> 
> !0 can be negative or positive. Are we expecting positive return values from
> this API?

I have no strong opinion, but if you think below or other option is better, I can change

>=0  device is allowed be detached.
<0 device is not allowed be detached.

> 
> > + */
> > +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void
> > +*user_args);
> > +
> > +/**
> > + * Lock an Ethernet Device directly or register a callback function
> > + * for condition check at runtime, this help application to prevent
> > + * a device be detached unexpectedly 
> > + * NOTE: Lock a device multiple times with same parmeter will
> > +increase
> > + * a ref_count, and corresponding unlock decrease the ref_count, the
> > + * device will be unlocked when ref_count reach 0.
> 
> Nitpick: "note" sections should be done with @note marker.
> 
> Also, i would mention that this is a per-process lock that does not affect other
> processes (assuming i understood the code correctly, of course...).

OK, I will add more comment to explain this.

> 
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param callback
> > + *   !NULL the callback function will be added into a pre-detach list,
> > + *         it will be invoked when a device is going to be detached. The
> > + *         return value will decide if continue detach the device or not.
> > + *   NULL  lock the device directly, basically this just regiter a empty
> > + *         callback function(dev_is_busy) that return -EBUSY, so we can
> > + *         handle the pre-detach check in unified way.
> > + * @param user_args
> > + *   parameter will be parsed to callback function, only valid when
> > + *   callback != NULL.
> > + * @return
> > + *   0 on success, negative on error.
> > + */
> > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
> > +		     void *user_args);
> 
> Nitpicks: DPDK style guide discourages using spaces as indentation (other parts
> of this patch, and other patches have this issue as well).

OK, will fix all.
> 
> > +
> > +/**
> > + * Reverse operation of rte_eth_dev_lock.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param callback
> > + *   NULL  decrease the ref_count of default callback function.
> > + *   !NULL decrease the ref_count of specific callback with matched
> > + *         user_args.
> > + * @param user_args
> > + *   parameter to match, only valid when callback != NULL.
> > + * @return
> > + *   0 on success, negative on error.
> > + */
> > +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t
> callback,
> > +		       void *user_args);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/lib/librte_ethdev/rte_ethdev_lock.c
> > b/lib/librte_ethdev/rte_ethdev_lock.c
> 
> rte_ethdev_lock.* seem to be internal-only files. Perhaps you should name
> them without the rte_ prefix to indicate that they're not exported?
> 
> > new file mode 100644
> > index 000000000..688d1d70a
> > --- /dev/null
> > +++ b/lib/librte_ethdev/rte_ethdev_lock.c
> > @@ -0,0 +1,102 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation  */ #include
> > +"rte_ethdev_lock.h"
> > +
> > +struct lock_entry {
> > +	TAILQ_ENTRY(lock_entry) next;
> > +	rte_eth_dev_lock_callback_t callback;
> > +	uint16_t port_id;
> 
> <snip>
> 
> > +register_lock_callback(uint16_t port_id,
> > +		       rte_eth_dev_lock_callback_t callback,
> > +		       void *user_args)
> > +{
> > +	struct lock_entry *le;
> > +
> > +	rte_spinlock_lock(&lock_entry_lock);
> > +
> > +	TAILQ_FOREACH(le, &lock_entry_list, next) {
> > +		if (le->port_id == port_id &&
> > +		    le->callback == callback &&
> > +		    le->user_args == user_args)
> > +			break;
> > +	}
> > +
> > +	if (!le) {
> > +		le = calloc(1, sizeof(struct lock_entry));
> > +		if (!le) {
> 
> Nitpick: generally, DPDK style guide prefers "if (value)" or "if (!value)" to only
> be reserved for boolean values, and use explicit comparison (e.g. "if (value ==
> NULL)" or "if (value == 0)") for all other cases.

OK, will fix
> 
> --
> Thanks,
> Anatoly
  

Patch

diff --git a/lib/librte_ethdev/Makefile b/lib/librte_ethdev/Makefile
index 04e93f337..5c4646469 100644
--- a/lib/librte_ethdev/Makefile
+++ b/lib/librte_ethdev/Makefile
@@ -20,6 +20,7 @@  LIBABIVER := 9
 
 SRCS-y += rte_ethdev.c
 SRCS-y += rte_ethdev_mp.c
+SRCS-y += rte_ethdev_lock.c
 SRCS-y += rte_flow.c
 SRCS-y += rte_tm.c
 SRCS-y += rte_mtr.c
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 24360f522..6494e71a4 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -42,6 +42,7 @@ 
 #include "rte_ethdev_driver.h"
 #include "ethdev_profile.h"
 #include "rte_ethdev_mp.h"
+#include "rte_ethdev_lock.h"
 
 int ethdev_logtype;
 
@@ -787,7 +788,6 @@  rte_eth_dev_attach(const char *devargs, uint16_t *port_id)
 int
 rte_eth_dev_attach_private(const char *devargs, uint16_t *port_id)
 {
-
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		return -ENOTSUP;
 
@@ -828,6 +828,10 @@  rte_eth_dev_detach(uint16_t port_id, char *name __rte_unused)
 		return req.result;
 	}
 
+	ret = process_lock_callbacks(port_id);
+	if (ret)
+		return ret;
+
 	/* check pre_detach */
 	req.t = REQ_TYPE_PRE_DETACH;
 	req.port_id = port_id;
@@ -870,6 +874,7 @@  int
 rte_eth_dev_detach_private(uint16_t port_id, char *name __rte_unused)
 {
 	uint32_t dev_flags;
+	int ret;
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
 		return -ENOTSUP;
@@ -883,6 +888,10 @@  rte_eth_dev_detach_private(uint16_t port_id, char *name __rte_unused)
 		return -ENOTSUP;
 	}
 
+	ret = process_lock_callbacks(port_id);
+	if (ret)
+		return ret;
+
 	return do_eth_dev_detach(port_id);
 }
 
@@ -4683,6 +4692,36 @@  rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	return result;
 }
 
+static int
+dev_is_busy(uint16_t port_id __rte_unused, void *user_args __rte_unused)
+{
+	return -EBUSY;
+}
+
+int
+rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
+		 void *user_args)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	if (callback == NULL)
+		return register_lock_callback(port_id, dev_is_busy, NULL);
+	else
+		return register_lock_callback(port_id, callback, user_args);
+}
+
+int
+rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
+		   void *user_args)
+{
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	if (callback == NULL)
+		return unregister_lock_callback(port_id, dev_is_busy, NULL);
+	else
+		return unregister_lock_callback(port_id, callback, user_args);
+}
+
 RTE_INIT(ethdev_init_log);
 static void
 ethdev_init_log(void)
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index bb03d613b..506b6acdd 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4356,6 +4356,70 @@  rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
 	return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
 }
 
+/**
+ * Callback function before device is detached.
+ *
+ * This type of function will be added into a function list, and will be
+ * invoked before device be detached. Application can register a callback
+ * function so it can be notified and do some cleanup before detach happen.
+ * Also, any callback function return !0 value will prevent device be
+ * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param user_args
+ *   This is parameter "user_args" be saved when callback function is
+ *   registered(rte_dev_eth_lock).
+ *
+ * @return
+ *   0  device is allowed be detached.
+ *   !0 device is not allowed be detached.
+ */
+typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void *user_args);
+
+/**
+ * Lock an Ethernet Device directly or register a callback function
+ * for condition check at runtime, this help application to prevent
+ * a device be detached unexpectly.
+ * NOTE: Lock a device mutliple times with same parmeter will increase
+ * a ref_count, and coresponding unlock decrease the ref_count, the
+ * device will be unlocked when ref_count reach 0.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param callback
+ *   !NULL the callback function will be added into a pre-detach list,
+ *         it will be invoked when a device is going to be detached. The
+ *         return value will decide if continue detach the device or not.
+ *   NULL  lock the device directly, basically this just regiter a empty
+ *         callback function(dev_is_busy) that return -EBUSY, so we can
+ *         handle the pre-detach check in unified way.
+ * @param user_args
+ *   parameter will be parsed to callback function, only valid when
+ *   callback != NULL.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
+		     void *user_args);
+
+/**
+ * Reverse operation of rte_eth_dev_lock.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param callback
+ *   NULL  decrease the ref_count of default callback function.
+ *   !NULL decrease the ref_count of specific callback with matched
+ *         user_args.
+ * @param user_args
+ *   parameter to match, only valid when callback != NULL.
+ * @return
+ *   0 on success, negative on error.
+ */
+int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,
+		       void *user_args);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ethdev/rte_ethdev_lock.c b/lib/librte_ethdev/rte_ethdev_lock.c
new file mode 100644
index 000000000..688d1d70a
--- /dev/null
+++ b/lib/librte_ethdev/rte_ethdev_lock.c
@@ -0,0 +1,102 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+#include "rte_ethdev_lock.h"
+
+struct lock_entry {
+	TAILQ_ENTRY(lock_entry) next;
+	rte_eth_dev_lock_callback_t callback;
+	uint16_t port_id;
+	void *user_args;
+	int ref_count;
+};
+
+TAILQ_HEAD(lock_entry_list, lock_entry);
+static struct lock_entry_list lock_entry_list =
+	TAILQ_HEAD_INITIALIZER(lock_entry_list);
+static rte_spinlock_t lock_entry_lock = RTE_SPINLOCK_INITIALIZER;
+
+int
+register_lock_callback(uint16_t port_id,
+		       rte_eth_dev_lock_callback_t callback,
+		       void *user_args)
+{
+	struct lock_entry *le;
+
+	rte_spinlock_lock(&lock_entry_lock);
+
+	TAILQ_FOREACH(le, &lock_entry_list, next) {
+		if (le->port_id == port_id &&
+		    le->callback == callback &&
+		    le->user_args == user_args)
+			break;
+	}
+
+	if (!le) {
+		le = calloc(1, sizeof(struct lock_entry));
+		if (!le) {
+			rte_spinlock_unlock(&lock_entry_lock);
+			return -ENOMEM;
+		}
+		le->callback = callback;
+		le->port_id = port_id;
+		le->user_args = user_args;
+		TAILQ_INSERT_TAIL(&lock_entry_list, le, next);
+	}
+	le->ref_count++;
+
+	rte_spinlock_unlock(&lock_entry_lock);
+	return 0;
+}
+
+int
+unregister_lock_callback(uint16_t port_id,
+			 rte_eth_dev_lock_callback_t callback,
+			 void *user_args)
+{
+	struct lock_entry *le;
+	int ret = 0;
+
+	rte_spinlock_lock(&lock_entry_lock);
+
+	TAILQ_FOREACH(le, &lock_entry_list, next) {
+		if (le->port_id == port_id &&
+		    le->callback == callback &&
+		    le->user_args == user_args)
+			break;
+	}
+
+	if (le) {
+		le->ref_count--;
+		if (!le->ref_count) {
+			TAILQ_REMOVE(&lock_entry_list, le, next);
+			free(le);
+		}
+	} else {
+		ret = -ENOENT;
+	}
+
+	rte_spinlock_unlock(&lock_entry_lock);
+	return ret;
+}
+
+int
+process_lock_callbacks(uint16_t port_id)
+{
+	struct lock_entry *le;
+
+	rte_spinlock_lock(&lock_entry_lock);
+
+	TAILQ_FOREACH(le, &lock_entry_list, next) {
+		if (le->port_id != port_id)
+			continue;
+
+		if (le->callback(port_id, le->user_args)) {
+			rte_spinlock_unlock(&lock_entry_lock);
+			return -EBUSY;
+		}
+	}
+
+	rte_spinlock_unlock(&lock_entry_lock);
+	return 0;
+}
diff --git a/lib/librte_ethdev/rte_ethdev_lock.h b/lib/librte_ethdev/rte_ethdev_lock.h
new file mode 100644
index 000000000..7b370c926
--- /dev/null
+++ b/lib/librte_ethdev/rte_ethdev_lock.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2018 Intel Corporation
+ */
+
+#ifndef _RTE_ETHDEV_LOCK_H_
+#define _RTE_ETHDEV_LOCK_H_
+
+#include "rte_ethdev.h"
+
+int
+register_lock_callback(uint16_t port_id,
+		       rte_eth_dev_lock_callback_t callback,
+		       void *user_args);
+
+int
+unregister_lock_callback(uint16_t port_id,
+			 rte_eth_dev_lock_callback_t callback,
+			 void *user_args);
+
+int
+process_lock_callbacks(uint16_t port_id);
+
+#endif
diff --git a/lib/librte_ethdev/rte_ethdev_mp.c b/lib/librte_ethdev/rte_ethdev_mp.c
index 8ede8151d..e23c8b010 100644
--- a/lib/librte_ethdev/rte_ethdev_mp.c
+++ b/lib/librte_ethdev/rte_ethdev_mp.c
@@ -4,6 +4,7 @@ 
 
 #include "rte_ethdev_driver.h"
 #include "rte_ethdev_mp.h"
+#include "rte_ethdev_lock.h"
 
 static int detach_on_secondary(uint16_t port_id)
 {
@@ -101,7 +102,7 @@  static int handle_primary_request(const struct rte_mp_msg *msg, const void *peer
 		ret = attach_on_secondary(req->devargs, req->port_id);
 		break;
 	case REQ_TYPE_PRE_DETACH:
-		ret = 0;
+		ret = process_lock_callbacks(req->port_id);
 		break;
 	case REQ_TYPE_DETACH:
 	case REQ_TYPE_ATTACH_ROLLBACK: