diff mbox series

net/memif: allow stopping and closing device

Message ID 119df28c7cc9d267@cs.arizona.edu (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/memif: allow stopping and closing device |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS

Commit Message

Junxiao Shi Nov. 18, 2021, 5:33 p.m. UTC 
  Bugzilla ID: 888
Fixes: febc855b358e ("ethdev: forbid closing started device")

Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
---
 drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Nov. 24, 2021, 11:02 a.m. UTC | #1 
On 11/18/2021 5:33 PM, Junxiao Shi wrote:
> Bugzilla ID: 888
> Fixes: febc855b358e ("ethdev: forbid closing started device")
> 
> Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>

Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.

> ---
>   drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
> index 43d7378329..e3d523af57 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev)
>   	return ret;
>   }
>   
> +static int
> +memif_dev_stop(struct rte_eth_dev *dev)
> +{
> +	memif_disconnect(dev);

Is the 'memif_dev_stop()' safe to be called multiple times?
If 'memif_dev_close()' calls 'memif_dev_stop()' (see below), it is possible
to call 'memif_dev_stop()' multiple times, so it should be protected.

> +	return 0;
> +}
> +
>   static int
>   memif_dev_close(struct rte_eth_dev *dev)
>   {
> @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>   
>   	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>   		memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
> -		memif_disconnect(dev);
>   
>   		for (i = 0; i < dev->data->nb_rx_queues; i++)
>   			(*dev->dev_ops->rx_queue_release)(dev, i);
> @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>   			(*dev->dev_ops->tx_queue_release)(dev, i);
>   
>   		memif_socket_remove_device(dev);
> -	} else {
> -		memif_disconnect(dev);

Should we add 'memif_dev_stop()' within the close function?
Otherwise we are relying on user to stop, but at least in remove path
('rte_pmd_memif_remove()') that may not be the case.

>   	}
>   
>   	rte_free(dev->process_private);
> @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t qid __rte_unused)
>   
>   static const struct eth_dev_ops ops = {
>   	.dev_start = memif_dev_start,
> +	.dev_stop = memif_dev_stop,
>   	.dev_close = memif_dev_close,
>   	.dev_infos_get = memif_dev_info,
>   	.dev_configure = memif_dev_configure,
>
yoursunny Nov. 24, 2021, 2:01 p.m. UTC | #2 
Hi Ferruh

You have to rely on user to call stop before calling close/remove.
This is mandated in ethdev library, as implemented in:
febc855b358e ("ethdev: forbid closing started device")

Yours, Junxiao

On Wed, Nov 24, 2021, 06:02 Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 11/18/2021 5:33 PM, Junxiao Shi wrote:
> > Bugzilla ID: 888
> > Fixes: febc855b358e ("ethdev: forbid closing started device")
> >
> > Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
>
> Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.
>
> > ---
> >   drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/memif/rte_eth_memif.c
> b/drivers/net/memif/rte_eth_memif.c
> > index 43d7378329..e3d523af57 100644
> > --- a/drivers/net/memif/rte_eth_memif.c
> > +++ b/drivers/net/memif/rte_eth_memif.c
> > @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev)
> >       return ret;
> >   }
> >
> > +static int
> > +memif_dev_stop(struct rte_eth_dev *dev)
> > +{
> > +     memif_disconnect(dev);
>
> Is the 'memif_dev_stop()' safe to be called multiple times?
> If 'memif_dev_close()' calls 'memif_dev_stop()' (see below), it is possible
> to call 'memif_dev_stop()' multiple times, so it should be protected.
>
> > +     return 0;
> > +}
> > +
> >   static int
> >   memif_dev_close(struct rte_eth_dev *dev)
> >   {
> > @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev)
> >
> >       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >               memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
> > -             memif_disconnect(dev);
> >
> >               for (i = 0; i < dev->data->nb_rx_queues; i++)
> >                       (*dev->dev_ops->rx_queue_release)(dev, i);
> > @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev)
> >                       (*dev->dev_ops->tx_queue_release)(dev, i);
> >
> >               memif_socket_remove_device(dev);
> > -     } else {
> > -             memif_disconnect(dev);
>
> Should we add 'memif_dev_stop()' within the close function?
> Otherwise we are relying on user to stop, but at least in remove path
> ('rte_pmd_memif_remove()') that may not be the case.
>
> >       }
> >
> >       rte_free(dev->process_private);
> > @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev
> *dev, uint16_t qid __rte_unused)
> >
> >   static const struct eth_dev_ops ops = {
> >       .dev_start = memif_dev_start,
> > +     .dev_stop = memif_dev_stop,
> >       .dev_close = memif_dev_close,
> >       .dev_infos_get = memif_dev_info,
> >       .dev_configure = memif_dev_configure,
> >
>
>
Ferruh Yigit Nov. 24, 2021, 2:43 p.m. UTC | #3 
On 11/24/2021 2:01 PM, yoursunny wrote:

Hi Junxiao, comment moved down, please don't top post,
it makes following the discussion very hard in the archive.

> Yours, Junxiao
> 
> On Wed, Nov 24, 2021, 06:02 Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 11/18/2021 5:33 PM, Junxiao Shi wrote:
>      > Bugzilla ID: 888
>      > Fixes: febc855b358e ("ethdev: forbid closing started device")
>      >
>      > Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com <mailto:git@mail1.yoursunny.com>>
> 
>     Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.
> 
>      > ---
>      >   drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
>      >   1 file changed, 8 insertions(+), 3 deletions(-)
>      >
>      > diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
>      > index 43d7378329..e3d523af57 100644
>      > --- a/drivers/net/memif/rte_eth_memif.c
>      > +++ b/drivers/net/memif/rte_eth_memif.c
>      > @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev)
>      >       return ret;
>      >   }
>      >
>      > +static int
>      > +memif_dev_stop(struct rte_eth_dev *dev)
>      > +{
>      > +     memif_disconnect(dev);
> 
>     Is the 'memif_dev_stop()' safe to be called multiple times?
>     If 'memif_dev_close()' calls 'memif_dev_stop()' (see below), it is possible
>     to call 'memif_dev_stop()' multiple times, so it should be protected.
> 
>      > +     return 0;
>      > +}
>      > +
>      >   static int
>      >   memif_dev_close(struct rte_eth_dev *dev)
>      >   {
>      > @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>      >
>      >       if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>      >               memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
>      > -             memif_disconnect(dev);
>      >
>      >               for (i = 0; i < dev->data->nb_rx_queues; i++)
>      >                       (*dev->dev_ops->rx_queue_release)(dev, i);
>      > @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>      >                       (*dev->dev_ops->tx_queue_release)(dev, i);
>      >
>      >               memif_socket_remove_device(dev);
>      > -     } else {
>      > -             memif_disconnect(dev);
> 
>     Should we add 'memif_dev_stop()' within the close function?
>     Otherwise we are relying on user to stop, but at least in remove path
>     ('rte_pmd_memif_remove()') that may not be the case.
> 

> Hi Ferruh
> 
> You have to rely on user to call stop before calling close/remove.
> This is mandated in ethdev library, as implemented in:
> febc855b358e ("ethdev: forbid closing started device")
> 


Yes it is enforced,
and 'rte_pmd_memif_remove()' calls 'rte_eth_dev_close()' instead of
'memif_dev_close()', so agree that it won't mean much to add stop()
within close().

I will proceed with the patch without waiting review from Jakup
to have it in the release.

>      >       }
>      >
>      >       rte_free(dev->process_private);
>      > @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t qid __rte_unused)
>      >
>      >   static const struct eth_dev_ops ops = {
>      >       .dev_start = memif_dev_start,
>      > +     .dev_stop = memif_dev_stop,
>      >       .dev_close = memif_dev_close,
>      >       .dev_infos_get = memif_dev_info,
>      >       .dev_configure = memif_dev_configure,
>      >
>
Ferruh Yigit Nov. 24, 2021, 3:05 p.m. UTC | #4 
On 11/18/2021 5:33 PM, Junxiao Shi wrote:
> Bugzilla ID: 888
> Fixes: febc855b358e ("ethdev: forbid closing started device")
> 
> Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.
diff mbox series

Patch

diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 43d7378329..e3d523af57 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -1260,6 +1260,13 @@  memif_dev_start(struct rte_eth_dev *dev)
 	return ret;
 }
 
+static int
+memif_dev_stop(struct rte_eth_dev *dev)
+{
+	memif_disconnect(dev);
+	return 0;
+}
+
 static int
 memif_dev_close(struct rte_eth_dev *dev)
 {
@@ -1268,7 +1275,6 @@  memif_dev_close(struct rte_eth_dev *dev)
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		memif_msg_enq_disconnect(pmd->cc, "Device closed", 0);
-		memif_disconnect(dev);
 
 		for (i = 0; i < dev->data->nb_rx_queues; i++)
 			(*dev->dev_ops->rx_queue_release)(dev, i);
@@ -1276,8 +1282,6 @@  memif_dev_close(struct rte_eth_dev *dev)
 			(*dev->dev_ops->tx_queue_release)(dev, i);
 
 		memif_socket_remove_device(dev);
-	} else {
-		memif_disconnect(dev);
 	}
 
 	rte_free(dev->process_private);
@@ -1515,6 +1519,7 @@  memif_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t qid __rte_unused)
 
 static const struct eth_dev_ops ops = {
 	.dev_start = memif_dev_start,
+	.dev_stop = memif_dev_stop,
 	.dev_close = memif_dev_close,
 	.dev_infos_get = memif_dev_info,
 	.dev_configure = memif_dev_configure,