examples/ntb: remove redundant logic for dev close
Checks
Commit Message
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(-)
Comments
> -----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>
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.
> -----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
>
> -----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
>
> >
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.
> -----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.
>
>
@@ -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);
}