[PATCH] examples/ntb: remove redundant logic for dev close

[PATCH] examples/ntb: remove redundant logic for dev close

[Junfeng Guo]

During EAL cleanup stage, all bus devices are cleaned up properly.
Based on this, there is no need to do extra device close process,
which may call the dev ops '*dev->dev_ops->dev_close' twice.

If this dev ops for ntb was called twice, the interrupt handle for
EAL will be disabled twice and will lead to error for the seconde
time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"

Thus, this patch just remove the redundant logic for device close.
Ports will be closed at rte_eal_cleanup() in the main loop.

Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
Cc: stable@dpdk.org

Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
 examples/ntb/ntb_fwd.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/examples/ntb/ntb_fwd.c b/examples/ntb/ntb_fwd.c
index 585aad9d70..3385b4b73c 100644
--- a/examples/ntb/ntb_fwd.c
+++ b/examples/ntb/ntb_fwd.c
@@ -177,13 +177,11 @@ cmd_quit_parsed(__rte_unused void *parsed_result,
 	rte_eal_mp_wait_lcore();
 	in_test = 0;
 
-	/* Stop traffic and Close port. */
+	/* Stop traffic only. */
 	rte_rawdev_stop(dev_id);
-	rte_rawdev_close(dev_id);
-	if (eth_port_id < RTE_MAX_ETHPORTS && fwd_mode == IOFWD) {
+	if (eth_port_id < RTE_MAX_ETHPORTS && fwd_mode == IOFWD)
 		rte_eth_dev_stop(eth_port_id);
-		rte_eth_dev_close(eth_port_id);
-	}
+	/* Ports will be closed at rte_eal_cleanup() in the main loop. */
 
 	cmdline_quit(cl);
 }
-- 
2.34.1

RE: [PATCH] examples/ntb: remove redundant logic for dev close

[Ling, WeiX]

> -----Original Message-----
> From: Junfeng Guo <junfeng.guo@intel.com>
> Sent: Wednesday, June 14, 2023 1:14 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>; Guo,
> Junfeng <junfeng.guo@intel.com>
> Subject: [PATCH] examples/ntb: remove redundant logic for dev close
> 
> During EAL cleanup stage, all bus devices are cleaned up properly.
> Based on this, there is no need to do extra device close process, which may
> call the dev ops '*dev->dev_ops->dev_close' twice.
> 
> If this dev ops for ntb was called twice, the interrupt handle for EAL will be
> disabled twice and will lead to error for the seconde time. Like this: "EAL:
> Error disabling MSI-X interrupts for fd xx"
> 
> Thus, this patch just remove the redundant logic for device close.
> Ports will be closed at rte_eal_cleanup() in the main loop.
> 
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> ---

Tested-by: Wei Ling <weix.ling@intel.com>

Re: [PATCH] examples/ntb: remove redundant logic for dev close

[Thomas Monjalon]

14/06/2023 07:14, Junfeng Guo:
> During EAL cleanup stage, all bus devices are cleaned up properly.
> Based on this, there is no need to do extra device close process,
> which may call the dev ops '*dev->dev_ops->dev_close' twice.
> 
> If this dev ops for ntb was called twice, the interrupt handle for
> EAL will be disabled twice and will lead to error for the seconde
> time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"

You should fix ntb to not disable interrupt twice.

RE: [PATCH] examples/ntb: remove redundant logic for dev close

[Guo, Junfeng]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 28, 2023 08:12
> To: Wu, Jingjing <jingjing.wu@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH] examples/ntb: remove redundant logic for dev close
> 
> 14/06/2023 07:14, Junfeng Guo:
> > During EAL cleanup stage, all bus devices are cleaned up properly.
> > Based on this, there is no need to do extra device close process,
> > which may call the dev ops '*dev->dev_ops->dev_close' twice.
> >
> > If this dev ops for ntb was called twice, the interrupt handle for
> > EAL will be disabled twice and will lead to error for the seconde
> > time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> 
> You should fix ntb to not disable interrupt twice.

Thanks for the comment!

Yes, it is exactly what this patch did.

The interrupt disabling process is part of 'dev_close' ops, and will be
called twice (one in EAL cleanup, another in ntb example cleanup).

Now that fixed & covered by the EAL cleanup, there is no need to
run dev_close at ntb example. Just remove that redundant logic.

Regards,
Junfeng

> 

RE: [PATCH] examples/ntb: remove redundant logic for dev close

[Guo, Junfeng]

> -----Original Message-----
> From: Guo, Junfeng
> Sent: Wednesday, June 28, 2023 10:19
> To: Thomas Monjalon <thomas@monjalon.net>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; Laatz, Kevin <Kevin.Laatz@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: RE: [PATCH] examples/ntb: remove redundant logic for dev close
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, June 28, 2023 08:12
> > To: Wu, Jingjing <jingjing.wu@intel.com>; Guo, Junfeng
> > <junfeng.guo@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> > <xingguang.he@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>;
> > Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [PATCH] examples/ntb: remove redundant logic for dev
> close
> >
> > 14/06/2023 07:14, Junfeng Guo:
> > > During EAL cleanup stage, all bus devices are cleaned up properly.
> > > Based on this, there is no need to do extra device close process,
> > > which may call the dev ops '*dev->dev_ops->dev_close' twice.
> > >
> > > If this dev ops for ntb was called twice, the interrupt handle for
> > > EAL will be disabled twice and will lead to error for the seconde
> > > time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> >
> > You should fix ntb to not disable interrupt twice.
> 
> Thanks for the comment!
> 
> Yes, it is exactly what this patch did.

Another solution is to add extra logic in dev_close, i.e., to do the
interrupt disabling process only when the interrupt handler is valid.
That is, to disable interrupt only when 'intr_handle->max_intr' or
'intr_handle->nb_efd' is non-zero.

But I'm not sure if this method is a better solution.
Please help give some advice, thanks!


> 
> The interrupt disabling process is part of 'dev_close' ops, and will be
> called twice (one in EAL cleanup, another in ntb example cleanup).
> 
> Now that fixed & covered by the EAL cleanup, there is no need to
> run dev_close at ntb example. Just remove that redundant logic.
> 
> Regards,
> Junfeng
> 
> >

Re: [PATCH] examples/ntb: remove redundant logic for dev close

[Thomas Monjalon]

28/06/2023 04:37, Guo, Junfeng:
> From: Guo, Junfeng
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 14/06/2023 07:14, Junfeng Guo:
> > > > During EAL cleanup stage, all bus devices are cleaned up properly.
> > > > Based on this, there is no need to do extra device close process,
> > > > which may call the dev ops '*dev->dev_ops->dev_close' twice.
> > > >
> > > > If this dev ops for ntb was called twice, the interrupt handle for
> > > > EAL will be disabled twice and will lead to error for the seconde
> > > > time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> > >
> > > You should fix ntb to not disable interrupt twice.
> > 
> > Thanks for the comment!
> > 
> > Yes, it is exactly what this patch did.
> 
> Another solution is to add extra logic in dev_close, i.e., to do the
> interrupt disabling process only when the interrupt handler is valid.
> That is, to disable interrupt only when 'intr_handle->max_intr' or
> 'intr_handle->nb_efd' is non-zero.
> 
> But I'm not sure if this method is a better solution.
> Please help give some advice, thanks!

Yes this is what I suggest.
You cannot fix all applications in the world,
but you can fix the driver, so it is a better way :)

> > The interrupt disabling process is part of 'dev_close' ops, and will be
> > called twice (one in EAL cleanup, another in ntb example cleanup).
> > 
> > Now that fixed & covered by the EAL cleanup, there is no need to
> > run dev_close at ntb example. Just remove that redundant logic.

RE: [PATCH] examples/ntb: remove redundant logic for dev close

[Guo, Junfeng]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 28, 2023 15:53
> To: Wu, Jingjing <jingjing.wu@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH] examples/ntb: remove redundant logic for dev close
> 
> 28/06/2023 04:37, Guo, Junfeng:
> > From: Guo, Junfeng
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 14/06/2023 07:14, Junfeng Guo:
> > > > > During EAL cleanup stage, all bus devices are cleaned up properly.
> > > > > Based on this, there is no need to do extra device close process,
> > > > > which may call the dev ops '*dev->dev_ops->dev_close' twice.
> > > > >
> > > > > If this dev ops for ntb was called twice, the interrupt handle for
> > > > > EAL will be disabled twice and will lead to error for the seconde
> > > > > time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> > > >
> > > > You should fix ntb to not disable interrupt twice.
> > >
> > > Thanks for the comment!
> > >
> > > Yes, it is exactly what this patch did.
> >
> > Another solution is to add extra logic in dev_close, i.e., to do the
> > interrupt disabling process only when the interrupt handler is valid.
> > That is, to disable interrupt only when 'intr_handle->max_intr' or
> > 'intr_handle->nb_efd' is non-zero.
> >
> > But I'm not sure if this method is a better solution.
> > Please help give some advice, thanks!
> 
> Yes this is what I suggest.
> You cannot fix all applications in the world,
> but you can fix the driver, so it is a better way :)

Sure! Thanks for the advice!

Will update with this solution, thanks!

> 
> > > The interrupt disabling process is part of 'dev_close' ops, and will be
> > > called twice (one in EAL cleanup, another in ntb example cleanup).
> > >
> > > Now that fixed & covered by the EAL cleanup, there is no need to
> > > run dev_close at ntb example. Just remove that redundant logic.
> 
> 

[PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops

[Junfeng Guo]

During EAL cleanup stage, all bus devices are cleaned up properly.
In the meantime, the ntb example app will also do the device cleanup
process, which may call the dev ops '*dev->dev_ops->dev_close' twice.

If this dev ops for ntb was called twice, the interrupt handle for
EAL will be disabled twice and will lead to error for the seconde
time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"

Thus, this patch added the check process for disabling interrupt in
dev_close ops, to ensure that interrupt only be disabled once.

Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
Cc: stable@dpdk.org

Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
 drivers/raw/ntb/ntb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c
index 76e98fe515..0ed4c14592 100644
--- a/drivers/raw/ntb/ntb.c
+++ b/drivers/raw/ntb/ntb.c
@@ -1045,6 +1045,11 @@ ntb_dev_close(struct rte_rawdev *dev)
 	hw->queue_pairs = 0;
 
 	intr_handle = hw->pci_dev->intr_handle;
+	/* Disable interrupt only once */
+	if (!rte_intr_nb_efd_get(intr_handle) &&
+	    !rte_intr_max_intr_get(intr_handle))
+		return 0;
+
 	/* Clean datapath event and vec mapping */
 	rte_intr_efd_disable(intr_handle);
 	rte_intr_vec_list_free(intr_handle);
-- 
2.34.1

RE: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops

[Ling, WeiX]

> -----Original Message-----
> From: Junfeng Guo <junfeng.guo@intel.com>
> Sent: Wednesday, June 28, 2023 5:12 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang
> <xingguang.he@intel.com>; Laatz, Kevin <kevin.laatz@intel.com>; Guo,
> Junfeng <junfeng.guo@intel.com>
> Subject: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close
> ops
> 
> During EAL cleanup stage, all bus devices are cleaned up properly.
> In the meantime, the ntb example app will also do the device cleanup
> process, which may call the dev ops '*dev->dev_ops->dev_close' twice.
> 
> If this dev ops for ntb was called twice, the interrupt handle for EAL will be
> disabled twice and will lead to error for the seconde time. Like this: "EAL:
> Error disabling MSI-X interrupts for fd xx"
> 
> Thus, this patch added the check process for disabling interrupt in dev_close
> ops, to ensure that interrupt only be disabled once.
> 
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> ---
>  drivers/raw/ntb/ntb.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Tested-by: Wei Ling <weix.ling@intel.com>

RE: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops

[Wu, Jingjing]

> -----Original Message-----
> From: Guo, Junfeng <junfeng.guo@intel.com>
> Sent: Wednesday, June 28, 2023 5:12 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang <xingguang.he@intel.com>; Laatz, Kevin
> <kevin.laatz@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
> Subject: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops
> 
> During EAL cleanup stage, all bus devices are cleaned up properly.
> In the meantime, the ntb example app will also do the device cleanup
> process, which may call the dev ops '*dev->dev_ops->dev_close' twice.
> 
> If this dev ops for ntb was called twice, the interrupt handle for
> EAL will be disabled twice and will lead to error for the seconde
> time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> 
> Thus, this patch added the check process for disabling interrupt in
> dev_close ops, to ensure that interrupt only be disabled once.
> 
> Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Re: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops

[Thomas Monjalon]

03/07/2023 08:53, Wu, Jingjing:
> 
> > -----Original Message-----
> > From: Guo, Junfeng <junfeng.guo@intel.com>
> > Sent: Wednesday, June 28, 2023 5:12 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org; He, Xingguang <xingguang.he@intel.com>; Laatz, Kevin
> > <kevin.laatz@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
> > Subject: [PATCH v2] raw/ntb: add check for disabling interrupt in dev close ops
> > 
> > During EAL cleanup stage, all bus devices are cleaned up properly.
> > In the meantime, the ntb example app will also do the device cleanup
> > process, which may call the dev ops '*dev->dev_ops->dev_close' twice.
> > 
> > If this dev ops for ntb was called twice, the interrupt handle for
> > EAL will be disabled twice and will lead to error for the seconde
> > time. Like this: "EAL: Error disabling MSI-X interrupts for fd xx"
> > 
> > Thus, this patch added the check process for disabling interrupt in
> > dev_close ops, to ensure that interrupt only be disabled once.
> > 
> > Fixes: 1cab1a40ea9b ("bus: cleanup devices on shutdown")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> 
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Applied, thanks.