[Accel-config] Re: [PATCH] accel-config/test: load/unload uacce module at cleanup

On 4/5/2021 11:11 PM, Thomas, Ramesh wrote:
> On Mon, Apr 05, 2021 at 09:11:54PM -0700, Dave Jiang wrote:
>> On 4/5/2021 6:10 PM, Thomas, Ramesh wrote:
>>> On Mon, Apr 05, 2021 at 05:53:21PM -0700, Dave Jiang wrote:
>>>> On 4/5/2021 5:45 PM, ramesh.thomas(a)intel.com wrote:
>>>>> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>>>>>
>>>>> Cleanup unloads and reloads all idxd kernel modules. This is broken due
>>>>> to idxd_module depending on idxd module. The order of idxd_mdev and
>>>>> idxd_uacce modules are not fixed requiring additional checks during the
>>>>> process.
>>>>>
>>>>> Signed-off-by: Tony Zhu <tony.zhu(a)intel.com>
>>>>> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
>>>>> ---
>>>>>     test/common | 9 +++++++++
>>>>>     1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/test/common b/test/common
>>>>> index 4bd4ed3..e6e84dd 100644
>>>>> --- a/test/common
>>>>> +++ b/test/common
>>>>> @@ -99,6 +99,14 @@ check_prereq()
>>>>>     #
>>>>>     _cleanup()
>>>>>     {
>>>>> +lsmod | grep -q "idxd_mdev" && {
>>>>> +modprobe -r idxd_mdev 2>/dev/null || :
>>>>> +sleep 1
>>>>> +}
>>>>> +lsmod | grep -q "idxd_uacce" && {
>>>>> +modprobe -r idxd_uacce
>>>>> +sleep 1
>>>>> +}
>>>>>     lsmod | grep -q "idxd_mdev" && {
>>>>>     modprobe -r idxd_mdev
>>>>>     sleep 1
>>>> What is the reason having to remove idxd_mdev twice? idxd_uacce
>>>> shouldn't have any dependency on blocking idxd_mdev from loading....
>>> The dependncy order of idxd_uacce and idxd_mdev is not fixed. The one
>>> that has the other as a dependent would fail if unloaded first. This
>>> workaround takes care of both cases, though it is not clean and would be
>>> better if driver ensures a fixed dependency order.
>> Huh I'm surprised that there is dependency between the two at all. They
>> each really should be dependent on idxd.
> It complains about idxd being busy so it is possible the dependency is
> inside idxd. However, if the unload order is switched between uacce and
> mdev, then it would unload ok. The failing order is random.

That is definitely strange. I wonder if it has to do with 
idxd/sysfs/udev. Guess not a big deal if we need to wait for things to 
settle.

>
> Should we remove the code doing the unload and reload of modules in
> dsa_test? I don't think we should need to unload and reload drivers
> after every test which will only hide driver instability issues.

Well for dsa_test, the idea was that we are running on a fresh slate and 
not inherit something from a previous test. That way any issue we found 
is no dependency on previous tests. Also after the test is run, we 
probably want to unload for another test or even just operation for 
something else. It also excercises the load/unload and see if we hit any 
issues from that.


>
>>
>>>>> @@ -108,6 +116,7 @@ _cleanup()
>>>>>     sleep 1
>>>>>     }
>>>>>     modprobe idxd
>>>>> +sleep 1
>>>>>     }
>>>>>
>>>>>     # json2var