[SNIP]
What we should do instead is to make sure we have only a single lock for the
complete hive instead.
[Dennis Li] If we use a single lock, users will must wait for all devices
resuming successfully, but in fact their tasks are only running in device a. It
is not friendly to users.
Well that is intentional I would say. We can only restart submissions after all
devices are resumed successfully, cause otherwise it can happen that a job on
device A depends on memory on device B which isn't accessible.
[Dennis Li] Yes, you are right. Driver have make sure that the shared
resources(such as the shard memory) are ready before unlock the lock of adev
one by one. But each device still has private hardware resources such as video
and display.
Yeah, but those are negligible and we have a team working on display support
for XGMI. So this will sooner or later become a problem as well.
I still think that a single rwsem for the whole hive is still the best option
here.
[Dennis Li] For your above concern, we can open a new thread to discuss it. If
we make a decision to use a single after discussing, we can create another
patch for it.
That's a really good argument, but I still hesitate to merge this patch.
How severe is the lockdep splat?
At bare minimum we need a "/* TODO: ...." comment why we do this and how to
remove the workaround again.
[Dennis Li] It is not a workaround. According to design of lockdep, each
rw_semaphore should has a separated lock_class_key. I have explained that
init_rwsem has the above limitation, so we must correct it.
Yeah, but this explanation is not really sufficient. Again this is not
an limitation of init_rwsem, but intentional behavior.
In other words in most cases lockdep should splat if you use rw
semaphores like this.
That we avoid this by locking multiple amdgpu device always in the same
order is rather questionable driver design.
Core network driver (net/core/dev.c) has the similar use case with us.
Just took a look at that, but this is completely different use case. The
lockdep classes are grouped by driver type to note the difference in the
network stack.
A quick search showed that no other device driver is doing this in the
kernel, so I'm not sure if such a behavior wouldn't be rejected. Adding
Daniel to comment on this as well.
Felix had some really good arguments to make that an urgent fix, so
either we come up with some real rework of this or at least add a big
"/* TODO....*/".
Regards,
Christian.
Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
