* Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
@ 2020-06-20 9:20 Markus Elfring
2020-06-20 9:37 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Markus Elfring @ 2020-06-20 9:20 UTC (permalink / raw)
To: Bernard Zhao, opensource.kernel, amd-gfx, dri-devel
Cc: David Airlie, Felix Kühling, kernel-janitors, linux-kernel,
Daniel Vetter, Alex Deucher, Christian König
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.
I suggest to improve this change description.
* Can an other wording variant be nicer?
* Will the tag “Fixes” become helpful for the commit message?
Regards,
Markus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches 2020-06-20 9:20 [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Markus Elfring @ 2020-06-20 9:37 ` Julia Lawall 2020-06-20 11:16 ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Julia Lawall @ 2020-06-20 9:37 UTC (permalink / raw) To: Markus Elfring Cc: opensource.kernel, David Airlie, Bernard Zhao, Felix Kühling, kernel-janitors, linux-kernel, dri-devel, amd-gfx, Daniel Vetter, Alex Deucher, Christian König [-- Attachment #1: Type: text/plain, Size: 2250 bytes --] On Sat, 20 Jun 2020, Markus Elfring wrote: > > The function kobject_init_and_add alloc memory like: > > kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs > > ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller > > ->kmalloc_slab, in err branch this memory not free. If use > > kmemleak, this path maybe catched. > > These changes are to add kobject_put in kobject_init_and_add > > failed branch, fix potential memleak. > > I suggest to improve this change description. > > * Can an other wording variant be nicer? Markus's suggestion is as usual extremely imprecise. However, I also find the message quite unclear. It would be good to always use English words. alloc and err are not English words. Perhaps most people will figure out what they are abbreviations for, but it would be better to use a few more letters to make it so that no one has to guess. Then there are a bunch of things that are connected by arrows with no spaces between them. The most obvious meaning of an arrow with no space around it is a variable dereference. After spending some mental effort, one can realize that that is not what you mean here. A layout like: first_function -> second_function -> third_function would be much more readable. I don't know what "this patch maybe catched" means. Is "catched" supposed to be "caught" or "cached"? Overall, the sentence could be "Kmemleak could possibly detect this issue", or something like that. But I don't know what this means. Did you detect the problem with kmemleak? if you did not detect the problem with kmemleak, and overall you don't know whether kmemleak would detect the bug or not, is this information useful at all for the patch? "These changes are to" makes a lot of words with no information. While it is perhaps not necessary to slavishly follow the rule about using the imperative, if it is convenient to use the imperative, doing so eliminates such meaningless phrases. memleak is also not an English word. Memory leak is only a few more characters, and doesn't require the reader to make the small extra effort to figure out what you mean. julia > > * Will the tag “Fixes” become helpful for the commit message? > > Regards, > Markus > ^ permalink raw reply [flat|nested] 6+ messages in thread
* =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn 2020-06-20 9:37 ` Julia Lawall @ 2020-06-20 11:16 ` Bernard 2020-06-20 11:26 ` Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Julia Lawall 2020-06-20 12:56 ` Markus Elfring 2020-06-20 16:14 ` [v2] " Markus Elfring 2 siblings, 1 reply; 6+ messages in thread From: Bernard @ 2020-06-20 11:16 UTC (permalink / raw) To: Julia Lawall Cc: opensource.kernel, David Airlie, Felix Kühling, kernel-janitors, linux-kernel, dri-devel, Markus Elfring, amd-gfx, Daniel Vetter, Alex Deucher, Christian König CgpGcm9tOiBKdWxpYSBMYXdhbGwgPGp1bGlhLmxhd2FsbEBpbnJpYS5mcj4KRGF0ZTogMjAyMC0w Ni0yMCAxNzozNzoxOQpUbzogIE1hcmt1cyBFbGZyaW5nIDxNYXJrdXMuRWxmcmluZ0B3ZWIuZGU+ CkNjOiAgQmVybmFyZCBaaGFvIDxiZXJuYXJkQHZpdm8uY29tPixvcGVuc291cmNlLmtlcm5lbEB2 aXZvLmNvbSxhbWQtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZyxkcmktZGV2ZWxAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnLGtlcm5lbC1qYW5pdG9yc0B2Z2VyLmtlcm5lbC5vcmcsbGludXgta2VybmVs QHZnZXIua2VybmVsLm9yZyxBbGV4IERldWNoZXIgPGFsZXhhbmRlci5kZXVjaGVyQGFtZC5jb20+ LCJDaHJpc3RpYW4gS8O2bmlnIiA8Y2hyaXN0aWFuLmtvZW5pZ0BhbWQuY29tPiwiRmVsaXggS8O8 aGxpbmciIDxGZWxpeC5LdWVobGluZ0BhbWQuY29tPixEYW5pZWwgVmV0dGVyIDxkYW5pZWxAZmZ3 bGwuY2g+LERhdmlkIEFpcmxpZSA8YWlybGllZEBsaW51eC5pZT4KU3ViamVjdDogUmU6IFtQQVRD SCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYnJh bmNoZXM+Cj4KPk9uIFNhdCwgMjAgSnVuIDIwMjAsIE1hcmt1cyBFbGZyaW5nIHdyb3RlOgo+Cj4+ ID4gVGhlIGZ1bmN0aW9uIGtvYmplY3RfaW5pdF9hbmRfYWRkIGFsbG9jIG1lbW9yeSBsaWtlOgo+ PiA+IGtvYmplY3RfaW5pdF9hbmRfYWRkLT5rb2JqZWN0X2FkZF92YXJnLT5rb2JqZWN0X3NldF9u YW1lX3ZhcmdzCj4+ID4gLT5rdmFzcHJpbnRmX2NvbnN0LT5rc3RyZHVwX2NvbnN0LT5rc3RyZHVw LT5rbWFsbG9jX3RyYWNrX2NhbGxlcgo+PiA+IC0+a21hbGxvY19zbGFiLCBpbiBlcnIgYnJhbmNo IHRoaXMgbWVtb3J5IG5vdCBmcmVlLiBJZiB1c2UKPj4gPiBrbWVtbGVhaywgdGhpcyBwYXRoIG1h eWJlIGNhdGNoZWQuCj4+ID4gVGhlc2UgY2hhbmdlcyBhcmUgdG8gYWRkIGtvYmplY3RfcHV0IGlu IGtvYmplY3RfaW5pdF9hbmRfYWRkCj4+ID4gZmFpbGVkIGJyYW5jaCwgZml4IHBvdGVudGlhbCBt ZW1sZWFrLgo+Pgo+PiBJIHN1Z2dlc3QgdG8gaW1wcm92ZSB0aGlzIGNoYW5nZSBkZXNjcmlwdGlv bi4KPj4KPj4gKiBDYW4gYW4gb3RoZXIgd29yZGluZyB2YXJpYW50IGJlIG5pY2VyPwo+Cj5NYXJr dXMncyBzdWdnZXN0aW9uIGlzIGFzIHVzdWFsIGV4dHJlbWVseSBpbXByZWNpc2UuICBIb3dldmVy LCBJIGFsc28gZmluZAo+dGhlIG1lc3NhZ2UgcXVpdGUgdW5jbGVhci4KPgo+SXQgd291bGQgYmUg Z29vZCB0byBhbHdheXMgdXNlIEVuZ2xpc2ggd29yZHMuICBhbGxvYyBhbmQgZXJyIGFyZSBub3QK PkVuZ2xpc2ggd29yZHMuICBQZXJoYXBzIG1vc3QgcGVvcGxlIHdpbGwgZmlndXJlIG91dCB3aGF0 IHRoZXkgYXJlCj5hYmJyZXZpYXRpb25zIGZvciwgYnV0IGl0IHdvdWxkIGJlIGJldHRlciB0byB1 c2UgYSBmZXcgbW9yZSBsZXR0ZXJzIHRvCj5tYWtlIGl0IHNvIHRoYXQgbm8gb25lIGhhcyB0byBn dWVzcy4KPgo+VGhlbiB0aGVyZSBhcmUgYSBidW5jaCBvZiB0aGluZ3MgdGhhdCBhcmUgY29ubmVj dGVkIGJ5IGFycm93cyB3aXRoIG5vCj5zcGFjZXMgYmV0d2VlbiB0aGVtLiAgVGhlIG1vc3Qgb2J2 aW91cyBtZWFuaW5nIG9mIGFuIGFycm93IHdpdGggbm8gc3BhY2UKPmFyb3VuZCBpdCBpcyBhIHZh cmlhYmxlIGRlcmVmZXJlbmNlLiAgQWZ0ZXIgc3BlbmRpbmcgc29tZSBtZW50YWwgZWZmb3J0LAo+ b25lIGNhbiByZWFsaXplIHRoYXQgdGhhdCBpcyBub3Qgd2hhdCB5b3UgbWVhbiBoZXJlLiAgQSBs YXlvdXQgbGlrZToKPgo+ICAgZmlyc3RfZnVuY3Rpb24gLT4KPiAgICAgc2Vjb25kX2Z1bmN0aW9u IC0+Cj4gICAgICAgdGhpcmRfZnVuY3Rpb24KPgo+d291bGQgYmUgbXVjaCBtb3JlIHJlYWRhYmxl Lgo+Cj5JIGRvbid0IGtub3cgd2hhdCAidGhpcyBwYXRjaCBtYXliZSBjYXRjaGVkIiBtZWFucy4g IElzICJjYXRjaGVkIiBzdXBwb3NlZAo+dG8gYmUgImNhdWdodCIgb3IgImNhY2hlZCI/ICBPdmVy YWxsLCB0aGUgc2VudGVuY2UgY291bGQgYmUgIkttZW1sZWFrCj5jb3VsZCBwb3NzaWJseSBkZXRl Y3QgdGhpcyBpc3N1ZSIsIG9yIHNvbWV0aGluZyBsaWtlIHRoYXQuICBCdXQgSSBkb24ndAo+a25v dyB3aGF0IHRoaXMgbWVhbnMuICBEaWQgeW91IGRldGVjdCB0aGUgcHJvYmxlbSB3aXRoIGttZW1s ZWFrPyAgaWYgeW91Cj5kaWQgbm90IGRldGVjdCB0aGUgcHJvYmxlbSB3aXRoIGttZW1sZWFrLCBh bmQgb3ZlcmFsbCB5b3UgZG9uJ3Qga25vdwo+d2hldGhlciBrbWVtbGVhayB3b3VsZCBkZXRlY3Qg dGhlIGJ1ZyBvciBub3QsIGlzIHRoaXMgaW5mb3JtYXRpb24gdXNlZnVsCj5hdCBhbGwgZm9yIHRo ZSBwYXRjaD8KCkhpOgoKS21lbWxlYWsgZGV0ZWN0ZWQgYSBtZW1vcnkgbGVhayBhcyBiZWxvdzoK a29iamVjdF9pbml0X2FuZF9hZGQtPgoJa29iamVjdF9hZGRfdmFyZy0+CgkJa29iamVjdF9zZXRf bmFtZV92YXJncy0+CgkJCWt2YXNwcmludGZfY29uc3QtPgoJCQkJa3N0cmR1cF9jb25zdC0+CgkJ CQkJa3N0cmR1cC0+CgkJCQkJCWttYWxsb2NfdHJhY2tfY2FsbGVyLT4KCQkJCQkJCWttYWxsb2Nf c2xhYgoJCklmIGtvYmplY3RfaW5pdF9hbmRfYWRkIGlzIGNhbGxlZCwgYnV0IGtvYmplY3RfcHV0 IGlzIG5vdCBjYWxsZWQgaW4gdGhlIGVycm9yIGJyYW5jaC4KVGhpcyB3aWxsIGJlIGRldGVjdGVk IGJ5IGttZW1sZWFrLgoKQlIvL0Jlcm5hcmQKCj4iVGhlc2UgY2hhbmdlcyBhcmUgdG8iIG1ha2Vz IGEgbG90IG9mIHdvcmRzIHdpdGggbm8gaW5mb3JtYXRpb24uICBXaGlsZSBpdAo+aXMgcGVyaGFw cyBub3QgbmVjZXNzYXJ5IHRvIHNsYXZpc2hseSBmb2xsb3cgdGhlIHJ1bGUgYWJvdXQgdXNpbmcg dGhlCj5pbXBlcmF0aXZlLCBpZiBpdCBpcyBjb252ZW5pZW50IHRvIHVzZSB0aGUgaW1wZXJhdGl2 ZSwgZG9pbmcgc28gZWxpbWluYXRlcwo+c3VjaCBtZWFuaW5nbGVzcyBwaHJhc2VzLgo+Cj5tZW1s ZWFrIGlzIGFsc28gbm90IGFuIEVuZ2xpc2ggd29yZC4gIE1lbW9yeSBsZWFrIGlzIG9ubHkgYSBm ZXcgbW9yZQo+Y2hhcmFjdGVycywgYW5kIGRvZXNuJ3QgcmVxdWlyZSB0aGUgcmVhZGVyIHRvIG1h a2UgdGhlIHNtYWxsIGV4dHJhIGVmZm9ydAo+dG8gZmlndXJlIG91dCB3aGF0IHlvdSBtZWFuLgo+ Cj5qdWxpYQo+Cj4+Cj4+ICogV2lsbCB0aGUgdGFnIOKAnEZpeGVz4oCdIGJlY29tZSBoZWxwZnVs IGZvciB0aGUgY29tbWl0IG1lc3NhZ2U/Cj4+Cj4+IFJlZ2FyZHMsCj4+IE1hcmt1cwo+PgoNCg0K ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches 2020-06-20 11:16 ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard @ 2020-06-20 11:26 ` Julia Lawall 0 siblings, 0 replies; 6+ messages in thread From: Julia Lawall @ 2020-06-20 11:26 UTC (permalink / raw) To: Bernard Cc: opensource.kernel, David Airlie, Felix Kühling, kernel-janitors, linux-kernel, dri-devel, Markus Elfring, amd-gfx, Daniel Vetter, Alex Deucher, Christian König [-- Attachment #1: Type: text/plain, Size: 3658 bytes --] On Sat, 20 Jun 2020, Bernard wrote: > > > From: Julia Lawall <julia.lawall@inria.fr> > Date: 2020-06-20 17:37:19 > To: Markus Elfring <Markus.Elfring@web.de> > Cc: Bernard Zhao <bernard@vivo.com>,opensource.kernel@vivo.com,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,kernel-janitors@vger.kernel.org,linux-kernel@vger.kernel.org,Alex Deucher <alexander.deucher@amd.com>,"Christian König" <christian.koenig@amd.com>,"Felix Kühling" <Felix.Kuehling@amd.com>,Daniel Vetter <daniel@ffwll.ch>,David Airlie <airlied@linux.ie> > Subject: Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches> > > > >On Sat, 20 Jun 2020, Markus Elfring wrote: > > > >> > The function kobject_init_and_add alloc memory like: > >> > kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs > >> > ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller > >> > ->kmalloc_slab, in err branch this memory not free. If use > >> > kmemleak, this path maybe catched. > >> > These changes are to add kobject_put in kobject_init_and_add > >> > failed branch, fix potential memleak. > >> > >> I suggest to improve this change description. > >> > >> * Can an other wording variant be nicer? > > > >Markus's suggestion is as usual extremely imprecise. However, I also find > >the message quite unclear. > > > >It would be good to always use English words. alloc and err are not > >English words. Perhaps most people will figure out what they are > >abbreviations for, but it would be better to use a few more letters to > >make it so that no one has to guess. > > > >Then there are a bunch of things that are connected by arrows with no > >spaces between them. The most obvious meaning of an arrow with no space > >around it is a variable dereference. After spending some mental effort, > >one can realize that that is not what you mean here. A layout like: > > > > first_function -> > > second_function -> > > third_function > > > >would be much more readable. > > > >I don't know what "this patch maybe catched" means. Is "catched" supposed > >to be "caught" or "cached"? Overall, the sentence could be "Kmemleak > >could possibly detect this issue", or something like that. But I don't > >know what this means. Did you detect the problem with kmemleak? if you > >did not detect the problem with kmemleak, and overall you don't know > >whether kmemleak would detect the bug or not, is this information useful > >at all for the patch? > > Hi: > > Kmemleak detected a memory leak as below: > kobject_init_and_add-> > kobject_add_varg-> > kobject_set_name_vargs-> > kvasprintf_const-> > kstrdup_const-> > kstrdup-> > kmalloc_track_caller-> > kmalloc_slab > > If kobject_init_and_add is called, but kobject_put is not called in the error branch. > This will be detected by kmemleak. Thanks. This is much more understandable. The last part still seems a bit hypothetical. After the trace, which explain why you made the change, just say what you did in the patch to fix the problem. julia > > BR//Bernard > > >"These changes are to" makes a lot of words with no information. While it > >is perhaps not necessary to slavishly follow the rule about using the > >imperative, if it is convenient to use the imperative, doing so eliminates > >such meaningless phrases. > > > >memleak is also not an English word. Memory leak is only a few more > >characters, and doesn't require the reader to make the small extra effort > >to figure out what you mean. > > > >julia > > > >> > >> * Will the tag “Fixes” become helpful for the commit message? > >> > >> Regards, > >> Markus > >> > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches 2020-06-20 9:37 ` Julia Lawall 2020-06-20 11:16 ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard @ 2020-06-20 12:56 ` Markus Elfring 2020-06-20 16:14 ` [v2] " Markus Elfring 2 siblings, 0 replies; 6+ messages in thread From: Markus Elfring @ 2020-06-20 12:56 UTC (permalink / raw) To: Julia Lawall, Bernard Zhao, opensource.kernel, amd-gfx, dri-devel Cc: David Airlie, Felix Kühling, kernel-janitors, linux-kernel, Daniel Vetter, Alex Deucher, Christian König >> I suggest to improve this change description. >> >> * Can an other wording variant be nicer? > > Markus's suggestion is as usual extremely imprecise. I pointed a general possibility out. I did not propose an exact wording alternative as it happened for other patches. > However, I also find the message quite unclear. I find this response interesting. > It would be good to always use English words. I am curious how this review will evolve further with such information also after the third patch version. https://lore.kernel.org/lkml/20200620091152.11206-1-bernard@vivo.com/ https://lore.kernel.org/patchwork/patch/1260303/ Regards, Markus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2] drm/amdkfd: Fix memory leaks according to error branches 2020-06-20 9:37 ` Julia Lawall 2020-06-20 11:16 ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard 2020-06-20 12:56 ` Markus Elfring @ 2020-06-20 16:14 ` Markus Elfring 2 siblings, 0 replies; 6+ messages in thread From: Markus Elfring @ 2020-06-20 16:14 UTC (permalink / raw) To: Julia Lawall, Bernard Zhao, opensource.kernel, amd-gfx, dri-devel Cc: Rob Clark, David Airlie, Felix Kühling, kernel-janitors, linux-kernel, Daniel Vetter, Alex Deucher, Christian König > memleak is also not an English word. Memory leak is only a few more > characters, and doesn't require the reader to make the small extra effort > to figure out what you mean. Would you like to achieve similar adjustments at any more places? How do you think about effects from a corresponding jargon? https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/msm/msm_submitqueue.c?id\x177d3819633cd520e3f95df541a04644aab4c657 Regards, Markus ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-20 16:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-20 9:20 [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Markus Elfring 2020-06-20 9:37 ` Julia Lawall 2020-06-20 11:16 ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard 2020-06-20 11:26 ` Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Julia Lawall 2020-06-20 12:56 ` Markus Elfring 2020-06-20 16:14 ` [v2] " Markus Elfring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).