Date:	Sun, 21 Dec 2014 17:10:05 +0100
From:	Christian König <deathsimple@...afone.de>
To:	Oded Gabbay <oded.gabbay@....com>, dri-devel@...ts.freedesktop.org
CC:	Alexander.Deucher@....com, linux-kernel@...r.kernel.org,
	Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init

Am 21.12.2014 um 17:03 schrieb Oded Gabbay:
>
>
> On 12/21/2014 05:57 PM, Christian König wrote:
>>> There should be, but when the modules are compiled in, they are 
>>> loaded based on
>>> link order only, if they are in the same group, and the groups are 
>>> loaded by a
>>> pre-defined order.
>> Is that really still up to date? I've seen effort to change that 
>> something like
>> 10+ years ago when Rusty reworked the module system. And it is 
>> comming up on the
>> lists again from time to time.
> From what I can see in the Makefile rules, code and google, yes, 
> that's still the situation. If someone will prove me wrong I will be 
> more than happy to correct my code.
>>
>>> I don't want to move iommu before gpu, so I don't have a solution 
>>> for the
>>> order between amdkfd and amd_iommu_v2.
>> Why not? That's still better than creating a kernel workqueue, 
>> scheduling one
>> work item on it, rescheduling the task until everything is completed 
>> and you can
>> continue.
> Because I don't know the consequences of moving an entire subsystem in 
> front of another one. In addition, even if everyone agrees, I'm pretty 
> sure that Linus won't be happy to do that in -rc stages. So maybe this 
> is something to consider for 3.20 merge window, but I would still like 
> to provide a solution for 3.19.

Yeah, true indeed. How about depending on everything being compiled as 
module for 3.19 then? Still better than having such a hack in the driver 
for as a temporary workaround for one release.

Christian.

>
>     Oded
>>
>> Christian.
>>
>> Am 21.12.2014 um 14:24 schrieb Oded Gabbay:
>>>
>>>
>>> On 12/21/2014 03:06 PM, Oded Gabbay wrote:
>>>>
>>>>
>>>> On 12/21/2014 02:19 PM, Christian König wrote:
>>>>> Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
>>>>>>
>>>>>>
>>>>>> On 12/21/2014 01:27 PM, Christian König wrote:
>>>>>>> Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
>>>>>>>> When amdkfd and radeon are compiled inside the kernel image 
>>>>>>>> (not as
>>>>>>>> modules),
>>>>>>>> radeon will load before amdkfd and will set *kfd2kgd to its 
>>>>>>>> interface
>>>>>>>> structure. Therefore, we must not set *kfd2kgd to NULL when 
>>>>>>>> amdkfd is loaded
>>>>>>>> because it will override radeon's initialization and cause 
>>>>>>>> kernel BUG.
>>>>>>>>
>>>>>>>> Signed-off-by: Oded Gabbay <oded.gabbay@....com>
>>>>>>>
>>>>>>> You should probably rather fix the dependency between the two 
>>>>>>> modules to
>>>>>>> get an
>>>>>>> determined load order instead of doing such nasty workarounds.
>>>>>>>
>>>>>>> Christian.
>>>>>>
>>>>>> The problem is that when modules are compiled inside the kernel, 
>>>>>> there is NO
>>>>>> determined load order and there is no mechanism to enforce that. 
>>>>>> If there
>>>>>> is/was such a mechanism, I would of course prefer to use it.
>>>>>
>>>>> There should be an determined order based on the symbol use, 
>>>>> otherwise
>>>>> initializing most of the kernel modules wouldn't work as expected. 
>>>>> For example
>>>>> radeon depends on the drm module must be loaded before radeon is 
>>>>> loaded.
>>>> There should be, but when the modules are compiled in, they are 
>>>> loaded based on
>>>> link order only, if they are in the same group, and the groups are 
>>>> loaded by a
>>>> pre-defined order.
>>>> The groups are: pure, core, postcore, arch, subsys, fs, device 
>>>> (which represents
>>>> all the modules) and late. See init.h
>>>>
>>>> So radeon, amdkfd and amd_iommu_v2 are all in device group, and in 
>>>> the group
>>>> they are ordered by their link order.
>>>>
>>>> Yes, radeon loads after drm, because drm*.o are before radeon*.o in 
>>>> the
>>>> Makefile. See
>>>> http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loading 
>>>>
>>>>
>>>>
>>>
>>> So I tried moving amdkfd inside the Makefile before radeon, and that 
>>> made
>>> amdkfd load before radeon did. This solves my first problem - order 
>>> between
>>> amdkfd and radeon. However, amd_iommu_v2 doesn't belong to the drm 
>>> Makefile,
>>> and I don't want to move iommu before gpu, so I don't have a 
>>> solution for the
>>> order between amdkfd and amd_iommu_v2.
>>>
>>>     Oded
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Actually, I don't understand why the kernel doesn't enforce the 
>>>>>> order
>>>>>> according to the use of exported symbols, like it does with modules.
>>>>>
>>>>> Yeah, that's indeed rather strange. There must be something in the 
>>>>> amdkfd code
>>>>> which broke that somehow.
>>>> IMO, that's a far-fetched guess. Could you point to something more 
>>>> specific ?
>>>>
>>>>>
>>>>> As far as I understand you the desired init order is radeon and 
>>>>> amd_iommu_v2
>>>>> first and then amdkfd, right?
>>>> Actually no. The preferred order is amd_iommu_v2, amdkfd and radeon 
>>>> last. This
>>>> is the order that happens when all three are built as modules. More 
>>>> accurately,
>>>> radeon inits, but its init triggers amdkfd init, which triggers 
>>>> amd_iommu_v2
>>>> init. So before radeon reaches its probe stage, all the modules were
>>>> initialized.
>>>>
>>>> So what happens when you boot with radeon,
>>>>> amd_iommu_v2 and amdkfd blacklisted for automatically load and 
>>>>> only load amdkfd
>>>>> manually?
>>>> As said above, that's ok.
>>>>>
>>>>>> There will always be dependencies between kgd (radeon) and amdkfd 
>>>>>> and between
>>>>>> amdkfd and amd_iommu_v2. I don't think I can eliminate those 
>>>>>> dependencies, not
>>>>>> without a very complex solution. And the fact that this complex 
>>>>>> solution
>>>>>> occurs only in a very specific use case (all modules compiled 
>>>>>> in), makes me
>>>>>> less inclined to do that.
>>>>>>
>>>>>> So I don't see it as a "nasty workaround". I would call it just a 
>>>>>> "workaround"
>>>>>> for a specific use case, which should be solved by a generic 
>>>>>> solution to the
>>>>>> kernel enforcing load orders.
>>>>>
>>>>> The normal kernel module handling already should provide the 
>>>>> correct init
>>>>> order,
>>>>> so I would still call this a rather nasty workaround because we 
>>>>> couldn't find
>>>>> the underlying problem.
>>>> Well, the normal kernel module handling doesn't work when all 
>>>> modules are
>>>> compiled in. I'm not a huge expert on this issue so I had Joerg 
>>>> Roedel help me
>>>> analyze this (thanks Joerg) and he agreed that there is no 
>>>> enforcement of order
>>>> in this case.
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>>     Oded
>>>>>>>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++---
>>>>>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>>>>>>>> index 95d5af1..236562f 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
>>>>>>>> @@ -34,7 +34,7 @@
>>>>>>>>   #define KFD_DRIVER_MINOR    7
>>>>>>>>   #define KFD_DRIVER_PATCHLEVEL    0
>>>>>>>> -const struct kfd2kgd_calls *kfd2kgd;
>>>>>>>> +const struct kfd2kgd_calls *kfd2kgd = NULL;
>>>>>>>>   static const struct kgd2kfd_calls kgd2kfd = {
>>>>>>>>       .exit        = kgd2kfd_exit,
>>>>>>>>       .probe        = kgd2kfd_probe,
>>>>>>>> @@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);
>>>>>>>>   void kgd2kfd_exit(void)
>>>>>>>>   {
>>>>>>>> +    kfd2kgd = NULL;
>>>>>>>>   }
>>>>>>>>   static int __init kfd_module_init(void)
>>>>>>>>   {
>>>>>>>>       int err;
>>>>>>>> -    kfd2kgd = NULL;
>>>>>>>> -
>>>>>>>>       /* Verify module parameters */
>>>>>>>>       if ((sched_policy < KFD_SCHED_POLICY_HWS) ||
>>>>>>>>           (sched_policy > KFD_SCHED_POLICY_NO_HWS)) {
>>>>>>>
>>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@...ts.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-kernel" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists