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.zh...@intel.com>; tho...@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richard...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>; Shelton, Benjamin H
> <benjamin.h.shel...@intel.com>; Vangati, Narender
> <narender.vang...@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.zh...@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
