diff mbox series

[v2,03/22] ethdev: add function to release port in local process

Message ID 20180621020059.1198-4-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/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Qi Zhang June 21, 2018, 2 a.m. UTC 
  Add driver API rte_eth_release_port_private to support the
requirement that an ethdev only be released on secondary process,
so only local state be set to unused , share data will not be
reset so primary process can still use it.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---

v2:
- rename rte_eth_release_port_local to rte_eth_release_port_private.

 lib/librte_ethdev/rte_ethdev.c        | 24 +++++++++++++++++++++---
 lib/librte_ethdev/rte_ethdev_driver.h | 13 +++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

Comments

Burakov, Anatoly June 21, 2018, 8:06 a.m. UTC | #1 
On 21-Jun-18 3:00 AM, Qi Zhang wrote:
> Add driver API rte_eth_release_port_private to support the
> requirement that an ethdev only be released on secondary process,
> so only local state be set to unused , share data will not be
> reset so primary process can still use it.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---

<snip>

>   
>   /**
>    * @internal
> + * Release the specified ethdev port in local process, only set to ethdev
> + * state to unused, but not reset share data since it assume other process
> + * is still using it, typically it is called by secondary process.
> + *
> + * @param eth_dev
> + * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
> + * @return
> + *   - 0 on success, negative on error
> + */
> +int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);
> +

As far as i can tell, even though the function is marked as internal, it 
should still be exported in the .map file (see rte_eth_dev_allocate() 
for example).

Thomas and others, does this count as new API? Should this be marked as 
__rte_experimental? Presumably, we guarantee ABI stability for internal 
functions too, so my expectation would be yes.

> +/**
> + * @internal
>    * Release device queues and clear its configuration to force the user
>    * application to reconfigure it. It is for internal use only.
>    *
>
Thomas Monjalon June 21, 2018, 8:21 a.m. UTC | #2 
21/06/2018 10:06, Burakov, Anatoly:
> On 21-Jun-18 3:00 AM, Qi Zhang wrote:
> > Add driver API rte_eth_release_port_private to support the
> > requirement that an ethdev only be released on secondary process,
> > so only local state be set to unused , share data will not be
> > reset so primary process can still use it.
> > 
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> 
> <snip>
> 
> >   
> >   /**
> >    * @internal
> > + * Release the specified ethdev port in local process, only set to ethdev
> > + * state to unused, but not reset share data since it assume other process
> > + * is still using it, typically it is called by secondary process.
> > + *
> > + * @param eth_dev
> > + * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
> > + * @return
> > + *   - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);
> > +
> 
> As far as i can tell, even though the function is marked as internal, it 
> should still be exported in the .map file (see rte_eth_dev_allocate() 
> for example).
> 
> Thomas and others, does this count as new API? Should this be marked as 
> __rte_experimental? Presumably, we guarantee ABI stability for internal 
> functions too, so my expectation would be yes.

You know the A in ABI stands for Application :)
If it is not called by application, it has no impact on ABI.

However, I am not sure about having this function at all.
Who is calling it?
Qi Zhang June 21, 2018, 8:21 a.m. UTC | #3 
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, June 21, 2018 4:06 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 v2 03/22] ethdev: add function to release port in local
> process
> 
> On 21-Jun-18 3:00 AM, Qi Zhang wrote:
> > Add driver API rte_eth_release_port_private to support the requirement
> > that an ethdev only be released on secondary process, so only local
> > state be set to unused , share data will not be reset so primary
> > process can still use it.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> 
> <snip>
> 
> >
> >   /**
> >    * @internal
> > + * Release the specified ethdev port in local process, only set to
> > +ethdev
> > + * state to unused, but not reset share data since it assume other
> > +process
> > + * is still using it, typically it is called by secondary process.
> > + *
> > + * @param eth_dev
> > + * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
> > + * @return
> > + *   - 0 on success, negative on error
> > + */
> > +int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);
> > +
> 
> As far as i can tell, even though the function is marked as internal, it should still
> be exported in the .map file (see rte_eth_dev_allocate() for example).
> 
> Thomas and others, does this count as new API? Should this be marked as
> __rte_experimental? Presumably, we guarantee ABI stability for internal
> functions too, so my expectation would be yes.

Sorry, I not intent to mark this as experimental, I must forgot to remove this
It should rte_eth_dev_attach/detach_private and rte_eth_dev_lock/unlock .

I guess internal API is not necessary to have this.
I will remove it in v3

Thanks
Qi

> 
> > +/**
> > + * @internal
> >    * Release device queues and clear its configuration to force the user
> >    * application to reconfigure it. It is for internal use only.
> >    *
> >
> 
> 
> --
> Thanks,
> Anatoly
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index a9977df97..205b2ee33 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -359,6 +359,23 @@  rte_eth_dev_attach_secondary(const char *name)
 }
 
 int
+rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev)
+{
+	if (eth_dev == NULL)
+		return -EINVAL;
+
+	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
+
+	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
+
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
+
+	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
+
+	return 0;
+}
+
+int
 rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
 	if (eth_dev == NULL)
@@ -370,9 +387,10 @@  rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
 
-	eth_dev->state = RTE_ETH_DEV_UNUSED;
-
-	memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+	if (eth_dev->state != RTE_ETH_DEV_UNUSED) {
+		eth_dev->state = RTE_ETH_DEV_UNUSED;
+		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+	}
 
 	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
 
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c9c825e3f..49c27223d 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -70,6 +70,19 @@  int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
 
 /**
  * @internal
+ * Release the specified ethdev port in local process, only set to ethdev
+ * state to unused, but not reset share data since it assume other process
+ * is still using it, typically it is called by secondary process.
+ *
+ * @param eth_dev
+ * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);
+
+/**
+ * @internal
  * Release device queues and clear its configuration to force the user
  * application to reconfigure it. It is for internal use only.
  *