Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
From: Julia Lawall Date: 2020-06-20 17:37:19 To: Markus Elfring Cc: Bernard Zhao ,opensource.ker...@vivo.com,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,kernel-janit...@vger.kernel.org,linux-ker...@vger.kernel.org,Alex Deucher ,"Christian König" ,"Felix Kühling" ,Daniel Vetter ,David Airlie 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. 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 >> ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
> 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 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
>> 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-bern...@vivo.com/ https://lore.kernel.org/patchwork/patch/1260303/ Regards, Markus ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
On Sat, 20 Jun 2020, Bernard wrote: > > > From: Julia Lawall > Date: 2020-06-20 17:37:19 > To: Markus Elfring > Cc: Bernard Zhao > ,opensource.ker...@vivo.com,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,kernel-janit...@vger.kernel.org,linux-ker...@vger.kernel.org,Alex > Deucher ,"Christian König" > ,"Felix Kühling" ,Daniel > Vetter ,David Airlie > 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 > >> > > >___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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? "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 >___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
