| Message ID | 20200806191518.593880-7-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blktests: Add support to run nvme tests with tcp/rdma transports | expand |
On 8/6/20 12:15, Sagi Grimberg wrote: > There is no need to retry module unload for rdma_rxe > and siw. This also creates a dependency on > tests/nvmeof/rc which prevents it from using in > other test subsystems. > > Signed-off-by: Sagi Grimberg<sagi@grimberg.me> You might want to CC Bart for this patch.
On 2020-08-06 12:15, Sagi Grimberg wrote: > There is no need to retry module unload for rdma_rxe > and siw. This also creates a dependency on > tests/nvmeof/rc which prevents it from using in > other test subsystems. If it wouldn't be necessary to retry module unload I wouldn't have introduced a loop. How about moving the unload_module() function definitions from tests/srp/rc and tests/nvmeof-mp/rc into common/rc instead? Thanks, Bart.
>> There is no need to retry module unload for rdma_rxe >> and siw. This also creates a dependency on >> tests/nvmeof/rc which prevents it from using in >> other test subsystems. > > If it wouldn't be necessary to retry module unload I wouldn't have > introduced a loop. I thought it was to work-around a driver issue as these drivers traditionally had plenty of stability issues. To be honest this retry loop to me indicated that either the driver has a bug or the test. But maybe there is a need I am not seeing. > How about moving the unload_module() function definitions from tests/srp/rc > and tests/nvmeof-mp/rc into common/rc instead? Don't have an issue with that.
On 2020-08-07 10:23, Sagi Grimberg wrote: > >>> There is no need to retry module unload for rdma_rxe >>> and siw. This also creates a dependency on >>> tests/nvmeof/rc which prevents it from using in >>> other test subsystems. >> >> If it wouldn't be necessary to retry module unload I wouldn't have >> introduced a loop. > > I thought it was to work-around a driver issue as these drivers > traditionally had plenty of stability issues. > > To be honest this retry loop to me indicated that either the > driver has a bug or the test. But maybe there is a need I > am not seeing. That loop was introduced a long time ago. I haven't tried to root-cause it but my guess is that the loop is necessary because the module refcount only drops to zero a short time after the first attempt to unload these kernel modules. Bart.
>>>> There is no need to retry module unload for rdma_rxe >>>> and siw. This also creates a dependency on >>>> tests/nvmeof/rc which prevents it from using in >>>> other test subsystems. >>> >>> If it wouldn't be necessary to retry module unload I wouldn't have >>> introduced a loop. >> >> I thought it was to work-around a driver issue as these drivers >> traditionally had plenty of stability issues. >> >> To be honest this retry loop to me indicated that either the >> driver has a bug or the test. But maybe there is a need I >> am not seeing. > > That loop was introduced a long time ago. I haven't tried to root-cause > it but my guess is that the loop is necessary because the module refcount > only drops to zero a short time after the first attempt to unload these > kernel modules. Didn't see any of that in the nvme testing...
diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma index 676d2837fb06..e54b2a96153c 100644 --- a/common/multipath-over-rdma +++ b/common/multipath-over-rdma @@ -457,7 +457,7 @@ stop_soft_rdma() { fi done ) - if ! unload_module rdma_rxe 10; then + if ! modprobe -r rdma_rxe; then echo "Unloading rdma_rxe failed" return 1 fi @@ -469,7 +469,7 @@ stop_soft_rdma() { echo "Failed to unbind the siw driver from ${i%_siw}" done ) - if ! unload_module siw 10; then + if ! modprobe -r siw; then echo "Unloading siw failed" return 1 fi
There is no need to retry module unload for rdma_rxe and siw. This also creates a dependency on tests/nvmeof/rc which prevents it from using in other test subsystems. Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- common/multipath-over-rdma | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)