Re: [PATCH 0/3] Use implicit kref infra
>>
>>> in this case, we can do something below in our release()
>>> //some cleanup work of B
>>> drm_dev_fini(dev);//destroy A
>>> kfree(adev)
>>>
>>>> 2. Using the kref infrastructure, when the ref goes to 0,
>>>> drm_dev_release is called. And here's the KEY:
>>>> Because WE allocated the container, we should free it--after the release
>>>> method is called, DRM cannot assume anything about the drm
>>>> device or the container. The "release" method is final.
>>>>
>>>> We allocate, we free. And we free only when the ref goes to 0.
>>>>
>>>> DRM can, in due time, "free" itself of the DRM device and stop
>>>> having knowledge of it--that's fine, but as long as the ref
>>>> is not 0, the amdgpu device and thus the contained DRM device,
>>>> cannot be freed.
>>>>
>>>>>
>>>>> You have to make another change something like
>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>> index 13068fdf4331..2aabd2b4c63b 100644
>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>> @@ -815,7 +815,8 @@ static void drm_dev_release(struct kref *ref)
>>>>>
>>>>> drm_managed_release(dev);
>>>>>
>>>>> - kfree(dev->managed.final_kfree);
>>>>> + if (dev->driver->final_release)
>>>>> + dev->driver->final_release(dev);
>>>>> }
>>>>
>>>> No. What's this?
>>>> There is no such thing as "final" release, nor is there a "partial"
>>>> release.
>>>> When the kref goes to 0, the device disappears. Simple.
>>>> If someone is using it, they should kref-get it, and when they're
>>>> done with it, they should kref-put it.
>>>
>>> I just take an example here. add another release in the end. then no one
>>> could touch us. IOW, final_release.
>>
>> No, that's horrible.
>>
>>>
>>>
>>> A destroy B by a callback, then A destroy itself. It assumes B just free
>>> its own resource.
>>> but that makes trouble if some resource of A is allocated by B.
>>> Because B must take care of these common resource shared between A and B.
>>
>> No, that's horrible.
>>
>>>
>>> yes, that logical is more complex. So I think we can revert drm_dev_release
>>> to its previous version.
>>
>> drm_dev_release() in its original form, was pure:
>>
>> static void drm_dev_release(struct kref *ref)
>> {
>> if (dev->driver->release)
>> dev->driver->release(dev);
>> else
>> drm_dev_fini(dev);
>> }
>>
>>>
>>>>
>>>> The whole point is that this is done implicitly, via the kref
>>>> infrastructure.
>>>> drm_dev_init() which we call in our PCI probe function, sets the kref to
>>>> 1--all
>>>> as per the documentation I pointed you to above.
>>>
>>> I am not taking about kref. what we are discussing is all about the release
>>> sequence.
>>
>> You need to understand how the kref infrastructure works in the kernel. I've
>> said
>> it a million times: it's implicit. The "release sequence" as you like to
>> call it,
>> is implicit in the kref infrastructure.
>>
>>>
>>>
>>>>
>>>> Another point is that we can do some other stuff in the release
>>>> function, notify someone, write some registers, free memory we use
>>>> for that PCI device, etc.
>>>>
>>>> If the "managed resources" infrastructure wants to stay, it should hook
>>>> itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register().
>>>> It shouldn't have to be so out-of-place like in patch 2/3 of this series,
>>>> where the drmm_add_final_kfree() is smack-dab in the middle of our PCI
>>>> discovery function, surrounded on top and bottom by drm_dev_init()
>>>> and drm_dev_register(). The "managed resources" infra should be
>>>> non-invasive
>>>> and drivers shouldn't have to change to use it--it should be invisible to
>>>> them.
>>>> Then our kref would just work.
>>>>
>>> yep, that make sense. But you need more changes to fix this issue. this
>>> patchset is insufficient.
>>
>> Or LESS. Less changes. Less is better. Basically revert and redo all this
>> "managed resources".
>>
>> Regards,
>> Luben
>>
>>>
>>>
>>>>>
>>>>> And in the final_release callback we free the dev. But that is a little
>>>>> complex now. so I prefer still using final_kfree.
>>>>> Of course we can do some cleanup work in the driver's release callback.
>>>>> BUT no kfree.
>>>>
>>>> No! No final_kfree. It's a hack.
>>>>
>>>> Read the documentation in drm_drv.c I noted above--it lays out how this
>>>> happens. Reading is required.
>>>>
>>>> Regards,
>>>> Luben
>>>>
>>>>
>>>>>
>>>>> -原始邮件-
>>>>> 发件人: "Tuikov, Luben"
>>>>> 日期: 2020年9月2日 星期三 09:07
>>>>> 收件人: "amd-...@lists.freedesktop.org" ,
>>>>> "dri-devel@lists.freedesktop.org"
>>>>> 抄送: "Deucher, Alexander" , Daniel Vetter
>>>>> , "Pan, Xinhui" , "Tuikov, Luben"
>>>>>
>>>>> 主题: [PATCH 0/3] Use implicit kref infra
>>>>>
>>>>> Use the implicit kref infrastructure to free the container
>>>>> struct amdgpu_device, container of struct drm_device.
>>>>>
>>>>> First, in drm_dev_register(), do not indiscriminately warn
>>>>> when a DRM driver hasn't opted for managed.final_kfree,
>>>>> but instead check if the driver has provided its own
>>>>> "release" function callback in the DRM driver structure.
>>>>> If that is the case, no warning.
>>>>>
>>>>> Remove drmm_add_final_kfree(). We take care of that, in the
>>>>> kref "release" callback when all refs are down to 0, via
>>>>> drm_dev_put(), i.e. the free is implicit.
>>>>>
>>>>> Remove superfluous NULL check, since the DRM device to be
>>>>> suspended always exists, so long as the underlying PCI and
>>>>> DRM devices exist.
>>>>>
>>>>> Luben Tuikov (3):
>>>>> drm: No warn for drivers who provide release
>>>>> drm/amdgpu: Remove drmm final free
>>>>> drm/amdgpu: Remove superfluous NULL check
>>>>>
>>>>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>>>>>drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 --
>>>>>drivers/gpu/drm/drm_drv.c | 3 ++-
>>>>>3 files changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> --
>>>>> 2.28.0.394.ge197136389
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
our PCI probe function, sets the kref to >>> 1--all >>> as per the documentation I pointed you to above. >> >> I am not taking about kref. what we are discussing is all about the release >> sequence. > > You need to understand how the kref infrastructure works in the kernel. I've > said > it a million times: it's implicit. The "release sequence" as you like to call > it, > is implicit in the kref infrastructure. > >> >> >>> >>> Another point is that we can do some other stuff in the release >>> function, notify someone, write some registers, free memory we use >>> for that PCI device, etc. >>> >>> If the "managed resources" infrastructure wants to stay, it should hook >>> itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register(). >>> It shouldn't have to be so out-of-place like in patch 2/3 of this series, >>> where the drmm_add_final_kfree() is smack-dab in the middle of our PCI >>> discovery function, surrounded on top and bottom by drm_dev_init() >>> and drm_dev_register(). The "managed resources" infra should be non-invasive >>> and drivers shouldn't have to change to use it--it should be invisible to >>> them. >>> Then our kref would just work. >>> >> yep, that make sense. But you need more changes to fix this issue. this >> patchset is insufficient. > > Or LESS. Less changes. Less is better. Basically revert and redo all this > "managed resources". > > Regards, > Luben > >> >> >>>> >>>> And in the final_release callback we free the dev. But that is a little >>>> complex now. so I prefer still using final_kfree. >>>> Of course we can do some cleanup work in the driver's release callback. >>>> BUT no kfree. >>> >>> No! No final_kfree. It's a hack. >>> >>> Read the documentation in drm_drv.c I noted above--it lays out how this >>> happens. Reading is required. >>> >>> Regards, >>> Luben >>> >>> >>>> >>>> -原始邮件- >>>> 发件人: "Tuikov, Luben" >>>> 日期: 2020年9月2日 星期三 09:07 >>>> 收件人: "amd-...@lists.freedesktop.org" , >>>> "dri-devel@lists.freedesktop.org" >>>> 抄送: "Deucher, Alexander" , Daniel Vetter >>>> , "Pan, Xinhui" , "Tuikov, Luben" >>>> >>>> 主题: [PATCH 0/3] Use implicit kref infra >>>> >>>> Use the implicit kref infrastructure to free the container >>>> struct amdgpu_device, container of struct drm_device. >>>> >>>> First, in drm_dev_register(), do not indiscriminately warn >>>> when a DRM driver hasn't opted for managed.final_kfree, >>>> but instead check if the driver has provided its own >>>> "release" function callback in the DRM driver structure. >>>> If that is the case, no warning. >>>> >>>> Remove drmm_add_final_kfree(). We take care of that, in the >>>> kref "release" callback when all refs are down to 0, via >>>> drm_dev_put(), i.e. the free is implicit. >>>> >>>> Remove superfluous NULL check, since the DRM device to be >>>> suspended always exists, so long as the underlying PCI and >>>> DRM devices exist. >>>> >>>> Luben Tuikov (3): >>>> drm: No warn for drivers who provide release >>>> drm/amdgpu: Remove drmm final free >>>> drm/amdgpu: Remove superfluous NULL check >>>> >>>>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- >>>>drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- >>>>drivers/gpu/drm/drm_drv.c | 3 ++- >>>>3 files changed, 2 insertions(+), 6 deletions(-) >>>> >>>> -- >>>> 2.28.0.394.ge197136389 >>>> >>>> >>>> >>> >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
On Wed, Sep 2, 2020 at 9:55 PM Daniel Vetter wrote:
>
> On Wed, Sep 2, 2020 at 9:17 PM Alex Deucher wrote:
> >
> > On Wed, Sep 2, 2020 at 3:04 PM Luben Tuikov wrote:
> > >
> > > On 2020-09-02 11:51 a.m., Daniel Stone wrote:
> > > > Hi Luben,
> > > >
> > > > On Wed, 2 Sep 2020 at 16:16, Luben Tuikov wrote:
> > > >> Not sure how I can do this when someone doesn't want to read up on
> > > >> the kref infrastructure. Can you help?
> > > >>
> > > >> When someone starts off with "My understanding of ..." (as in the OP)
> > > >> you know you're
> > > >> in trouble and in for a rough times.
> > > >>
> > > >> Such is the nature of world-wide open-to-everyone mailing lists where
> > > >> anyone can put forth an argument, regardless of their level of
> > > >> understanding.
> > > >> The more obfuscated an argument, the more uncertainty.
> > > >>
> > > >> If one knows the kref infrastructure, it just clicks, no explanation
> > > >> necessary.
> > > >
> > > > Evidently there are more points of view than yours. Evidently your
> > > > method of persuasion is also not working, because this thread is now
> > > > getting quite long and not converging on your point of view (which you
> > > > are holding to be absolutely objectively correct).
> > > >
> > > > I think you need to re-evaluate the way in which you speak to people,
> > > > considering that it costs nothing to be polite and considerate, and
> > > > also takes effort to be rude and dismissive.
> > >
> > > Not sure how to help this:
> > >
> > > > My understanding of the drm core code is like something below.
> > > > struct B {
> > > > strcut A
> > > > }
> > > > we initialize A firstly and initialize B in the end. But destroy B
> > > > firstly and destory A in the end.
> > >
> >
> > Luben, please tone it down a bit. You are coming across very harshly.
> > You do make a good point though. What is the point of having the drm
> > release callback if it's ostensibly useless? We should either use it
> > as intended to release the structures allocated by the driver or the
> > drm core should handle it all. With the managed resources there is an
> > incongruity between allocation and freeing which leads to confusion.
> > Even with the proposed updated documentation, it's not clear to me who
> > should use the managed resources or not. My understanding was that it
> > was optional for drivers that wanted it.
>
> In an ideal world this would all be perfectly clean. In reality we
> have huge existing drivers which, if at all feasible, can only be
> converted over step by step.
>
> So with that there's a few ways we can make this happen:
> - drmm resources are cleaned up before ->release is called. This means
> doing auto-cleanup of the final steps like cleanup up drm_device
> resources is gated on all drivers first being converted completely
> over to drmm, which is never going to happen. And it's holding up
> removing all the fairly simple cleanup code from small driver, which
> is where managed resources (whether drmm or devm) have the most
> benefit, often they completely eliminate the need for any explicit
> teardown code.
> - start in the middle. That just oopses because the unwind order isn't
> the inverse of the setup order anymore, and generally that's required.
> - start at the end. Unfortunately this means that the drm_device
> cannot be freed in the driver's ->release callback, therefore for
> transition purposes I had to sprinkle drmm_add_final_kfree all over
> the place. But if you use devm_drm_dev_alloc (like the updated docs
> recommend) that's not needed anymore, so really not an eyesore for
> driver developers.
>
> Yes there's mildly tricky code in the core as a result, but given that
> you guys wont volunteer to fix up the entire subsystem either we just
> have to live with that I think. Also, the commit adding the
> drm_managed stuff does explain these implementation details and the
> reasons why.
Also note that tons of stuff in drm doesn't yet provide drmm_
versions, teardown-less drivers really only works for really simple
ones. So completely getting rid of the ->release callback will also
need lots of core work, like the currently in-flight series to add
more drmm_ helpers for kms objects:
https://lore.kernel.org/dri-devel/20200827160545.1146-1-p.za...@pengutronix.de/
Help obviously very much welcome.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
On Wed, Sep 2, 2020 at 9:17 PM Alex Deucher wrote:
>
> On Wed, Sep 2, 2020 at 3:04 PM Luben Tuikov wrote:
> >
> > On 2020-09-02 11:51 a.m., Daniel Stone wrote:
> > > Hi Luben,
> > >
> > > On Wed, 2 Sep 2020 at 16:16, Luben Tuikov wrote:
> > >> Not sure how I can do this when someone doesn't want to read up on
> > >> the kref infrastructure. Can you help?
> > >>
> > >> When someone starts off with "My understanding of ..." (as in the OP)
> > >> you know you're
> > >> in trouble and in for a rough times.
> > >>
> > >> Such is the nature of world-wide open-to-everyone mailing lists where
> > >> anyone can put forth an argument, regardless of their level of
> > >> understanding.
> > >> The more obfuscated an argument, the more uncertainty.
> > >>
> > >> If one knows the kref infrastructure, it just clicks, no explanation
> > >> necessary.
> > >
> > > Evidently there are more points of view than yours. Evidently your
> > > method of persuasion is also not working, because this thread is now
> > > getting quite long and not converging on your point of view (which you
> > > are holding to be absolutely objectively correct).
> > >
> > > I think you need to re-evaluate the way in which you speak to people,
> > > considering that it costs nothing to be polite and considerate, and
> > > also takes effort to be rude and dismissive.
> >
> > Not sure how to help this:
> >
> > > My understanding of the drm core code is like something below.
> > > struct B {
> > > strcut A
> > > }
> > > we initialize A firstly and initialize B in the end. But destroy B
> > > firstly and destory A in the end.
> >
>
> Luben, please tone it down a bit. You are coming across very harshly.
> You do make a good point though. What is the point of having the drm
> release callback if it's ostensibly useless? We should either use it
> as intended to release the structures allocated by the driver or the
> drm core should handle it all. With the managed resources there is an
> incongruity between allocation and freeing which leads to confusion.
> Even with the proposed updated documentation, it's not clear to me who
> should use the managed resources or not. My understanding was that it
> was optional for drivers that wanted it.
In an ideal world this would all be perfectly clean. In reality we
have huge existing drivers which, if at all feasible, can only be
converted over step by step.
So with that there's a few ways we can make this happen:
- drmm resources are cleaned up before ->release is called. This means
doing auto-cleanup of the final steps like cleanup up drm_device
resources is gated on all drivers first being converted completely
over to drmm, which is never going to happen. And it's holding up
removing all the fairly simple cleanup code from small driver, which
is where managed resources (whether drmm or devm) have the most
benefit, often they completely eliminate the need for any explicit
teardown code.
- start in the middle. That just oopses because the unwind order isn't
the inverse of the setup order anymore, and generally that's required.
- start at the end. Unfortunately this means that the drm_device
cannot be freed in the driver's ->release callback, therefore for
transition purposes I had to sprinkle drmm_add_final_kfree all over
the place. But if you use devm_drm_dev_alloc (like the updated docs
recommend) that's not needed anymore, so really not an eyesore for
driver developers.
Yes there's mildly tricky code in the core as a result, but given that
you guys wont volunteer to fix up the entire subsystem either we just
have to live with that I think. Also, the commit adding the
drm_managed stuff does explain these implementation details and the
reasons why.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
On Wed, Sep 2, 2020 at 3:04 PM Luben Tuikov wrote:
>
> On 2020-09-02 11:51 a.m., Daniel Stone wrote:
> > Hi Luben,
> >
> > On Wed, 2 Sep 2020 at 16:16, Luben Tuikov wrote:
> >> Not sure how I can do this when someone doesn't want to read up on
> >> the kref infrastructure. Can you help?
> >>
> >> When someone starts off with "My understanding of ..." (as in the OP) you
> >> know you're
> >> in trouble and in for a rough times.
> >>
> >> Such is the nature of world-wide open-to-everyone mailing lists where
> >> anyone can put forth an argument, regardless of their level of
> >> understanding.
> >> The more obfuscated an argument, the more uncertainty.
> >>
> >> If one knows the kref infrastructure, it just clicks, no explanation
> >> necessary.
> >
> > Evidently there are more points of view than yours. Evidently your
> > method of persuasion is also not working, because this thread is now
> > getting quite long and not converging on your point of view (which you
> > are holding to be absolutely objectively correct).
> >
> > I think you need to re-evaluate the way in which you speak to people,
> > considering that it costs nothing to be polite and considerate, and
> > also takes effort to be rude and dismissive.
>
> Not sure how to help this:
>
> > My understanding of the drm core code is like something below.
> > struct B {
> > strcut A
> > }
> > we initialize A firstly and initialize B in the end. But destroy B firstly
> > and destory A in the end.
>
Luben, please tone it down a bit. You are coming across very harshly.
You do make a good point though. What is the point of having the drm
release callback if it's ostensibly useless? We should either use it
as intended to release the structures allocated by the driver or the
drm core should handle it all. With the managed resources there is an
incongruity between allocation and freeing which leads to confusion.
Even with the proposed updated documentation, it's not clear to me who
should use the managed resources or not. My understanding was that it
was optional for drivers that wanted it.
Alex
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
On 2020-09-02 11:51 a.m., Daniel Stone wrote:
> Hi Luben,
>
> On Wed, 2 Sep 2020 at 16:16, Luben Tuikov wrote:
>> Not sure how I can do this when someone doesn't want to read up on
>> the kref infrastructure. Can you help?
>>
>> When someone starts off with "My understanding of ..." (as in the OP) you
>> know you're
>> in trouble and in for a rough times.
>>
>> Such is the nature of world-wide open-to-everyone mailing lists where
>> anyone can put forth an argument, regardless of their level of understanding.
>> The more obfuscated an argument, the more uncertainty.
>>
>> If one knows the kref infrastructure, it just clicks, no explanation
>> necessary.
>
> Evidently there are more points of view than yours. Evidently your
> method of persuasion is also not working, because this thread is now
> getting quite long and not converging on your point of view (which you
> are holding to be absolutely objectively correct).
>
> I think you need to re-evaluate the way in which you speak to people,
> considering that it costs nothing to be polite and considerate, and
> also takes effort to be rude and dismissive.
Not sure how to help this:
> My understanding of the drm core code is like something below.
> struct B {
> strcut A
> }
> we initialize A firstly and initialize B in the end. But destroy B firstly
> and destory A in the end.
Regards,
Luben
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
Hi Luben, On Wed, 2 Sep 2020 at 16:16, Luben Tuikov wrote: > Not sure how I can do this when someone doesn't want to read up on > the kref infrastructure. Can you help? > > When someone starts off with "My understanding of ..." (as in the OP) you > know you're > in trouble and in for a rough times. > > Such is the nature of world-wide open-to-everyone mailing lists where > anyone can put forth an argument, regardless of their level of understanding. > The more obfuscated an argument, the more uncertainty. > > If one knows the kref infrastructure, it just clicks, no explanation > necessary. Evidently there are more points of view than yours. Evidently your method of persuasion is also not working, because this thread is now getting quite long and not converging on your point of view (which you are holding to be absolutely objectively correct). I think you need to re-evaluate the way in which you speak to people, considering that it costs nothing to be polite and considerate, and also takes effort to be rude and dismissive. Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
On 2020-09-02 11:00, Daniel Stone wrote: > Hi Luben, > > On Wed, 2 Sep 2020 at 15:51, Luben Tuikov wrote: >> Of course it's true--good morning! >> >> Let me stop you right there--just read the documentation I pointed >> to you at. >> >> No! >> >> I'm sorry, that doesn't make sense. >> >> No, that's horrible. >> >> No, that's horrible. >> >> You need to understand how the kref infrastructure works in the kernel. I've >> said >> it a million times: it's implicit. >> >> Or LESS. Less changes. Less is better. Basically revert and redo all this >> "managed resources". > > There are many better ways to make your point. At the moment it's just > getting lost in shouting. Hi Daniel, Not sure how I can do this when someone doesn't want to read up on the kref infrastructure. Can you help? When someone starts off with "My understanding of ..." (as in the OP) you know you're in trouble and in for a rough times. Such is the nature of world-wide open-to-everyone mailing lists where anyone can put forth an argument, regardless of their level of understanding. The more obfuscated an argument, the more uncertainty. If one knows the kref infrastructure, it just clicks, no explanation necessary. Regards, Luben > > Cheers, > Daniel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
kref goes to 0, the device disappears. Simple. >> If someone is using it, they should kref-get it, and when they're >> done with it, they should kref-put it. >> >> The whole point is that this is done implicitly, via the kref infrastructure. >> drm_dev_init() which we call in our PCI probe function, sets the kref to >> 1--all >> as per the documentation I pointed you to above. >> >> Another point is that we can do some other stuff in the release >> function, notify someone, write some registers, free memory we use >> for that PCI device, etc. >> >> If the "managed resources" infrastructure wants to stay, it should hook >> itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register(). >> It shouldn't have to be so out-of-place like in patch 2/3 of this series, >> where the drmm_add_final_kfree() is smack-dab in the middle of our PCI >> discovery function, surrounded on top and bottom by drm_dev_init() >> and drm_dev_register(). The "managed resources" infra should be non-invasive >> and drivers shouldn't have to change to use it--it should be invisible to >> them. >> Then our kref would just work. > > Unfortunately some part of that drm managed series is stuck still waiting > for review (I guess I need to resubmit), but with that the docs would tell > you to use devm_drm_dev_alloc instead of hand-rolling all this. That's very very misleading--the documentation DOES NOT tell you this. It tells you exactly how to do it using the "release" method folded into the kref infrastructure. It is your changes to the documentation which tell you to use this new alloc. > > Definitely not any of the ideas proposed in this discussion or patches :-) These are nothing new--it's just standard DRM stuff described in the documentation in drm_drv.c. There are NO NEW IDEAS or anything "proposed" in this discussion or patches--it's just standard stuff from DRM, which had been the case for the last 10 years, until your "managed resources" came along. It's the managed resources which are new. Regards, Luben > > I'll cc you on that series when I send it out again. > > Cheers, Daniel >> >>> >>> And in the final_release callback we free the dev. But that is a little >>> complex now. so I prefer still using final_kfree. >>> Of course we can do some cleanup work in the driver's release callback. BUT >>> no kfree. >> >> No! No final_kfree. It's a hack. >> >> Read the documentation in drm_drv.c I noted above--it lays out how this >> happens. Reading is required. >> >> Regards, >> Luben >> >> >>> >>> -原始邮件- >>> 发件人: "Tuikov, Luben" >>> 日期: 2020年9月2日 星期三 09:07 >>> 收件人: "amd-...@lists.freedesktop.org" , >>> "dri-devel@lists.freedesktop.org" >>> 抄送: "Deucher, Alexander" , Daniel Vetter >>> , "Pan, Xinhui" , "Tuikov, Luben" >>> >>> 主题: [PATCH 0/3] Use implicit kref infra >>> >>> Use the implicit kref infrastructure to free the container >>> struct amdgpu_device, container of struct drm_device. >>> >>> First, in drm_dev_register(), do not indiscriminately warn >>> when a DRM driver hasn't opted for managed.final_kfree, >>> but instead check if the driver has provided its own >>> "release" function callback in the DRM driver structure. >>> If that is the case, no warning. >>> >>> Remove drmm_add_final_kfree(). We take care of that, in the >>> kref "release" callback when all refs are down to 0, via >>> drm_dev_put(), i.e. the free is implicit. >>> >>> Remove superfluous NULL check, since the DRM device to be >>> suspended always exists, so long as the underlying PCI and >>> DRM devices exist. >>> >>> Luben Tuikov (3): >>> drm: No warn for drivers who provide release >>> drm/amdgpu: Remove drmm final free >>> drm/amdgpu: Remove superfluous NULL check >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- >>> drivers/gpu/drm/drm_drv.c | 3 ++- >>> 3 files changed, 2 insertions(+), 6 deletions(-) >>> >>> -- >>> 2.28.0.394.ge197136389 >>> >>> >>> >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
Hi Luben, On Wed, 2 Sep 2020 at 15:51, Luben Tuikov wrote: > Of course it's true--good morning! > > Let me stop you right there--just read the documentation I pointed > to you at. > > No! > > I'm sorry, that doesn't make sense. > > No, that's horrible. > > No, that's horrible. > > You need to understand how the kref infrastructure works in the kernel. I've > said > it a million times: it's implicit. > > Or LESS. Less changes. Less is better. Basically revert and redo all this > "managed resources". There are many better ways to make your point. At the moment it's just getting lost in shouting. Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
ref goes to 0,
>> drm_dev_release is called. And here's the KEY:
>> Because WE allocated the container, we should free it--after the release
>> method is called, DRM cannot assume anything about the drm
>> device or the container. The "release" method is final.
>>
>> We allocate, we free. And we free only when the ref goes to 0.
>>
>> DRM can, in due time, "free" itself of the DRM device and stop
>> having knowledge of it--that's fine, but as long as the ref
>> is not 0, the amdgpu device and thus the contained DRM device,
>> cannot be freed.
>>
>>>
>>> You have to make another change something like
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 13068fdf4331..2aabd2b4c63b 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -815,7 +815,8 @@ static void drm_dev_release(struct kref *ref)
>>>
>>>drm_managed_release(dev);
>>>
>>> - kfree(dev->managed.final_kfree);
>>> + if (dev->driver->final_release)
>>> + dev->driver->final_release(dev);
>>> }
>>
>> No. What's this?
>> There is no such thing as "final" release, nor is there a "partial" release.
>> When the kref goes to 0, the device disappears. Simple.
>> If someone is using it, they should kref-get it, and when they're
>> done with it, they should kref-put it.
>
> I just take an example here. add another release in the end. then no one
> could touch us. IOW, final_release.
No, that's horrible.
>
>
> A destroy B by a callback, then A destroy itself. It assumes B just free its
> own resource.
> but that makes trouble if some resource of A is allocated by B.
> Because B must take care of these common resource shared between A and B.
No, that's horrible.
>
> yes, that logical is more complex. So I think we can revert drm_dev_release
> to its previous version.
drm_dev_release() in its original form, was pure:
static void drm_dev_release(struct kref *ref)
{
if (dev->driver->release)
dev->driver->release(dev);
else
drm_dev_fini(dev);
}
>
>>
>> The whole point is that this is done implicitly, via the kref infrastructure.
>> drm_dev_init() which we call in our PCI probe function, sets the kref to
>> 1--all
>> as per the documentation I pointed you to above.
>
> I am not taking about kref. what we are discussing is all about the release
> sequence.
You need to understand how the kref infrastructure works in the kernel. I've
said
it a million times: it's implicit. The "release sequence" as you like to call
it,
is implicit in the kref infrastructure.
>
>
>>
>> Another point is that we can do some other stuff in the release
>> function, notify someone, write some registers, free memory we use
>> for that PCI device, etc.
>>
>> If the "managed resources" infrastructure wants to stay, it should hook
>> itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register().
>> It shouldn't have to be so out-of-place like in patch 2/3 of this series,
>> where the drmm_add_final_kfree() is smack-dab in the middle of our PCI
>> discovery function, surrounded on top and bottom by drm_dev_init()
>> and drm_dev_register(). The "managed resources" infra should be non-invasive
>> and drivers shouldn't have to change to use it--it should be invisible to
>> them.
>> Then our kref would just work.
>>
> yep, that make sense. But you need more changes to fix this issue. this
> patchset is insufficient.
Or LESS. Less changes. Less is better. Basically revert and redo all this
"managed resources".
Regards,
Luben
>
>
>>>
>>> And in the final_release callback we free the dev. But that is a little
>>> complex now. so I prefer still using final_kfree.
>>> Of course we can do some cleanup work in the driver's release callback. BUT
>>> no kfree.
>>
>> No! No final_kfree. It's a hack.
>>
>> Read the documentation in drm_drv.c I noted above--it lays out how this
>> happens. Reading is required.
>>
>> Regards,
>> Luben
>>
>>
>>>
>>> -原始邮件-
>>> 发件人: "Tuikov, Luben"
>>> 日期: 2020年9月2日 星期三 09:07
>>> 收件人: "amd-...@lists.freedesktop.org" ,
>>> "dri-devel@lists.freedesktop.org"
>>> 抄送: "Deucher, Alexander" , Daniel Vetter
>>> , "Pan, Xinhui" , "Tuikov, Luben"
>>>
>>> 主题: [PATCH 0/3] Use implicit kref infra
>>>
>>>Use the implicit kref infrastructure to free the container
>>>struct amdgpu_device, container of struct drm_device.
>>>
>>>First, in drm_dev_register(), do not indiscriminately warn
>>>when a DRM driver hasn't opted for managed.final_kfree,
>>>but instead check if the driver has provided its own
>>>"release" function callback in the DRM driver structure.
>>>If that is the case, no warning.
>>>
>>>Remove drmm_add_final_kfree(). We take care of that, in the
>>>kref "release" callback when all refs are down to 0, via
>>>drm_dev_put(), i.e. the free is implicit.
>>>
>>>Remove superfluous NULL check, since the DRM device to be
>>>suspended always exists, so long as the underlying PCI and
>>>DRM devices exist.
>>>
>>>Luben Tuikov (3):
>>> drm: No warn for drivers who provide release
>>> drm/amdgpu: Remove drmm final free
>>> drm/amdgpu: Remove superfluous NULL check
>>>
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 --
>>> drivers/gpu/drm/drm_drv.c | 3 ++-
>>> 3 files changed, 2 insertions(+), 6 deletions(-)
>>>
>>>--
>>>2.28.0.394.ge197136389
>>>
>>>
>>>
>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
stuff in the release > function, notify someone, write some registers, free memory we use > for that PCI device, etc. > > If the "managed resources" infrastructure wants to stay, it should hook > itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register(). > It shouldn't have to be so out-of-place like in patch 2/3 of this series, > where the drmm_add_final_kfree() is smack-dab in the middle of our PCI > discovery function, surrounded on top and bottom by drm_dev_init() > and drm_dev_register(). The "managed resources" infra should be non-invasive > and drivers shouldn't have to change to use it--it should be invisible to > them. > Then our kref would just work. Unfortunately some part of that drm managed series is stuck still waiting for review (I guess I need to resubmit), but with that the docs would tell you to use devm_drm_dev_alloc instead of hand-rolling all this. Definitely not any of the ideas proposed in this discussion or patches :-) I'll cc you on that series when I send it out again. Cheers, Daniel > > > > > And in the final_release callback we free the dev. But that is a little > > complex now. so I prefer still using final_kfree. > > Of course we can do some cleanup work in the driver's release callback. BUT > > no kfree. > > No! No final_kfree. It's a hack. > > Read the documentation in drm_drv.c I noted above--it lays out how this > happens. Reading is required. > > Regards, > Luben > > > > > > -原始邮件- > > 发件人: "Tuikov, Luben" > > 日期: 2020年9月2日 星期三 09:07 > > 收件人: "amd-...@lists.freedesktop.org" , > > "dri-devel@lists.freedesktop.org" > > 抄送: "Deucher, Alexander" , Daniel Vetter > > , "Pan, Xinhui" , "Tuikov, Luben" > > > > 主题: [PATCH 0/3] Use implicit kref infra > > > > Use the implicit kref infrastructure to free the container > > struct amdgpu_device, container of struct drm_device. > > > > First, in drm_dev_register(), do not indiscriminately warn > > when a DRM driver hasn't opted for managed.final_kfree, > > but instead check if the driver has provided its own > > "release" function callback in the DRM driver structure. > > If that is the case, no warning. > > > > Remove drmm_add_final_kfree(). We take care of that, in the > > kref "release" callback when all refs are down to 0, via > > drm_dev_put(), i.e. the free is implicit. > > > > Remove superfluous NULL check, since the DRM device to be > > suspended always exists, so long as the underlying PCI and > > DRM devices exist. > > > > Luben Tuikov (3): > > drm: No warn for drivers who provide release > > drm/amdgpu: Remove drmm final free > > drm/amdgpu: Remove superfluous NULL check > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- > > drivers/gpu/drm/drm_drv.c | 3 ++- > > 3 files changed, 2 insertions(+), 6 deletions(-) > > > > -- > > 2.28.0.394.ge197136389 > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
; If someone is using it, they should kref-get it, and when they're > done with it, they should kref-put it. I just take an example here. add another release in the end. then no one could touch us. IOW, final_release. A destroy B by a callback, then A destroy itself. It assumes B just free its own resource. but that makes trouble if some resource of A is allocated by B. Because B must take care of these common resource shared between A and B. yes, that logical is more complex. So I think we can revert drm_dev_release to its previous version. > > The whole point is that this is done implicitly, via the kref infrastructure. > drm_dev_init() which we call in our PCI probe function, sets the kref to > 1--all > as per the documentation I pointed you to above. I am not taking about kref. what we are discussing is all about the release sequence. > > Another point is that we can do some other stuff in the release > function, notify someone, write some registers, free memory we use > for that PCI device, etc. > > If the "managed resources" infrastructure wants to stay, it should hook > itself into drm_dev_fini() and into drm_dev_init() or drm_dev_register(). > It shouldn't have to be so out-of-place like in patch 2/3 of this series, > where the drmm_add_final_kfree() is smack-dab in the middle of our PCI > discovery function, surrounded on top and bottom by drm_dev_init() > and drm_dev_register(). The "managed resources" infra should be non-invasive > and drivers shouldn't have to change to use it--it should be invisible to > them. > Then our kref would just work. > yep, that make sense. But you need more changes to fix this issue. this patchset is insufficient. >> >> And in the final_release callback we free the dev. But that is a little >> complex now. so I prefer still using final_kfree. >> Of course we can do some cleanup work in the driver's release callback. BUT >> no kfree. > > No! No final_kfree. It's a hack. > > Read the documentation in drm_drv.c I noted above--it lays out how this > happens. Reading is required. > > Regards, > Luben > > >> >> -原始邮件- >> 发件人: "Tuikov, Luben" >> 日期: 2020年9月2日 星期三 09:07 >> 收件人: "amd-...@lists.freedesktop.org" , >> "dri-devel@lists.freedesktop.org" >> 抄送: "Deucher, Alexander" , Daniel Vetter >> , "Pan, Xinhui" , "Tuikov, Luben" >> >> 主题: [PATCH 0/3] Use implicit kref infra >> >>Use the implicit kref infrastructure to free the container >>struct amdgpu_device, container of struct drm_device. >> >>First, in drm_dev_register(), do not indiscriminately warn >>when a DRM driver hasn't opted for managed.final_kfree, >>but instead check if the driver has provided its own >>"release" function callback in the DRM driver structure. >>If that is the case, no warning. >> >>Remove drmm_add_final_kfree(). We take care of that, in the >>kref "release" callback when all refs are down to 0, via >>drm_dev_put(), i.e. the free is implicit. >> >>Remove superfluous NULL check, since the DRM device to be >>suspended always exists, so long as the underlying PCI and >>DRM devices exist. >> >>Luben Tuikov (3): >> drm: No warn for drivers who provide release >> drm/amdgpu: Remove drmm final free >> drm/amdgpu: Remove superfluous NULL check >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- >> drivers/gpu/drm/drm_drv.c | 3 ++- >> 3 files changed, 2 insertions(+), 6 deletions(-) >> >>-- >>2.28.0.394.ge197136389 >> >> >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
r kref would just work. > > And in the final_release callback we free the dev. But that is a little > complex now. so I prefer still using final_kfree. > Of course we can do some cleanup work in the driver's release callback. BUT > no kfree. No! No final_kfree. It's a hack. Read the documentation in drm_drv.c I noted above--it lays out how this happens. Reading is required. Regards, Luben > > -原始邮件- > 发件人: "Tuikov, Luben" > 日期: 2020年9月2日 星期三 09:07 > 收件人: "amd-...@lists.freedesktop.org" , > "dri-devel@lists.freedesktop.org" > 抄送: "Deucher, Alexander" , Daniel Vetter > , "Pan, Xinhui" , "Tuikov, Luben" > > 主题: [PATCH 0/3] Use implicit kref infra > > Use the implicit kref infrastructure to free the container > struct amdgpu_device, container of struct drm_device. > > First, in drm_dev_register(), do not indiscriminately warn > when a DRM driver hasn't opted for managed.final_kfree, > but instead check if the driver has provided its own > "release" function callback in the DRM driver structure. > If that is the case, no warning. > > Remove drmm_add_final_kfree(). We take care of that, in the > kref "release" callback when all refs are down to 0, via > drm_dev_put(), i.e. the free is implicit. > > Remove superfluous NULL check, since the DRM device to be > suspended always exists, so long as the underlying PCI and > DRM devices exist. > > Luben Tuikov (3): > drm: No warn for drivers who provide release > drm/amdgpu: Remove drmm final free > drm/amdgpu: Remove superfluous NULL check > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- > drivers/gpu/drm/drm_drv.c | 3 ++- > 3 files changed, 2 insertions(+), 6 deletions(-) > > -- > 2.28.0.394.ge197136389 > > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/3] Use implicit kref infra
If you take a look at the below function, you should not use driver's release
to free adev. As dev is embedded in adev.
809 static void drm_dev_release(struct kref *ref)
810 {
811 struct drm_device *dev = container_of(ref, struct drm_device, ref);
812
813 if (dev->driver->release)
814 dev->driver->release(dev);
815
816 drm_managed_release(dev);
817
818 kfree(dev->managed.final_kfree);
819 }
You have to make another change something like
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 13068fdf4331..2aabd2b4c63b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -815,7 +815,8 @@ static void drm_dev_release(struct kref *ref)
drm_managed_release(dev);
- kfree(dev->managed.final_kfree);
+ if (dev->driver->final_release)
+ dev->driver->final_release(dev);
}
And in the final_release callback we free the dev. But that is a little complex
now. so I prefer still using final_kfree.
Of course we can do some cleanup work in the driver's release callback. BUT no
kfree.
-原始邮件-
发件人: "Tuikov, Luben"
日期: 2020年9月2日 星期三 09:07
收件人: "amd-...@lists.freedesktop.org" ,
"dri-devel@lists.freedesktop.org"
抄送: "Deucher, Alexander" , Daniel Vetter
, "Pan, Xinhui" , "Tuikov, Luben"
主题: [PATCH 0/3] Use implicit kref infra
Use the implicit kref infrastructure to free the container
struct amdgpu_device, container of struct drm_device.
First, in drm_dev_register(), do not indiscriminately warn
when a DRM driver hasn't opted for managed.final_kfree,
but instead check if the driver has provided its own
"release" function callback in the DRM driver structure.
If that is the case, no warning.
Remove drmm_add_final_kfree(). We take care of that, in the
kref "release" callback when all refs are down to 0, via
drm_dev_put(), i.e. the free is implicit.
Remove superfluous NULL check, since the DRM device to be
suspended always exists, so long as the underlying PCI and
DRM devices exist.
Luben Tuikov (3):
drm: No warn for drivers who provide release
drm/amdgpu: Remove drmm final free
drm/amdgpu: Remove superfluous NULL check
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 --
drivers/gpu/drm/drm_drv.c | 3 ++-
3 files changed, 2 insertions(+), 6 deletions(-)
--
2.28.0.394.ge197136389
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] Use implicit kref infra
Use the implicit kref infrastructure to free the container struct amdgpu_device, container of struct drm_device. First, in drm_dev_register(), do not indiscriminately warn when a DRM driver hasn't opted for managed.final_kfree, but instead check if the driver has provided its own "release" function callback in the DRM driver structure. If that is the case, no warning. Remove drmm_add_final_kfree(). We take care of that, in the kref "release" callback when all refs are down to 0, via drm_dev_put(), i.e. the free is implicit. Remove superfluous NULL check, since the DRM device to be suspended always exists, so long as the underlying PCI and DRM devices exist. Luben Tuikov (3): drm: No warn for drivers who provide release drm/amdgpu: Remove drmm final free drm/amdgpu: Remove superfluous NULL check drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 2 -- drivers/gpu/drm/drm_drv.c | 3 ++- 3 files changed, 2 insertions(+), 6 deletions(-) -- 2.28.0.394.ge197136389 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
