net/memif: allow stopping and closing device
PATCH 1/1 net/memif: allow stopping and closing device
Bugzilla ID: 888
--- drivers/net/memif/rte_eth_memif.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/net/memif/rte_eth_memif.c +8 -3
@@ -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,
Signature · 1 line
2.17.1
Quoted 4 lines · Junxiao Shi
Bugzilla ID: 888
Fixes febc855b358e ("ethdev: forbid closing started device")Signed-off-by Junxiao Shi <[email protected]>
Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.
Quoted 9 lines
--- drivers/net/memif/rte_eth_memif.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/net/memif/rte_eth_memif.c +0
-0
drivers/net/memif/rte_eth_memif.c memif_dev_start(struct rte_eth_dev *dev) · line 1266
Quoted 7 lines
drivers/net/memif/rte_eth_memif.c +4
-0@@ -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.
Quoted 15 lines
drivers/net/memif/rte_eth_memif.c +3
-1+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);
drivers/net/memif/rte_eth_memif.c memif_dev_close(struct rte_eth_dev *dev) · line 1280
Quoted 5 lines
drivers/net/memif/rte_eth_memif.c +0
-2@@ -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.
Quoted 12 lines
drivers/net/memif/rte_eth_memif.c +1
-0}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,
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
Quoted 70 lines · On Wed, Nov 24, 2021, 06:02 Ferruh Yigit <[email protected]> wrote:
Quoted 4 lines · Junxiao Shi
Bugzilla ID: 888
Fixes febc855b358e ("ethdev: forbid closing started device")Signed-off-by Junxiao Shi <[email protected]>Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.
Review
Quoted 5 lines
--- drivers/net/memif/rte_eth_memif.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/memif/rte_eth_memif.cdrivers/net/memif/rte_eth_memif.c memif_dev_start(struct rte_eth_dev *dev) · line 1266
Quoted 11 lines
index
43d7378329..e3d523af57100644drivers/net/memif/rte_eth_memif.c +4
-0@@ -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.Quoted 15 lines
drivers/net/memif/rte_eth_memif.c +3
-1+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);drivers/net/memif/rte_eth_memif.c memif_dev_close(struct rte_eth_dev *dev) · line 1280
Quoted 5 lines
drivers/net/memif/rte_eth_memif.c +0
-2@@ -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.drivers/net/memif/rte_eth_memif.c memif_rx_queue_intr_disable(struct rte_eth_dev
Quoted 4 lines
drivers/net/memif/rte_eth_memif.c +0
-0}rte_free (dev->process_private);@@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev*dev,
uint16_tqid__rte_unused)Quoted 8 lines
drivers/net/memif/rte_eth_memif.c +1
-0static 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,
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.
Quoted 59 lines
Yours, Junxiao
On Wed, Nov 24, 2021, 06:02 Ferruh Yigit <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> 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.
Quoted 6 lines
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.
Quoted 13 lines
> }
>
> 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,
>Quoted 4 lines · Junxiao Shi
Bugzilla ID: 888
Fixes febc855b358e ("ethdev: forbid closing started device")Signed-off-by Junxiao Shi <[email protected]>
Applied to dpdk-next-net/main, thanks.