On Sat, 20 Jun 2020, Bernard wrote: > > > From: Julia Lawall <julia.lawall@xxxxxxxx> > Date: 2020-06-20 17:37:19 > To: Markus Elfring <Markus.Elfring@xxxxxx> > Cc: Bernard Zhao <bernard@xxxxxxxx>,opensource.kernel@xxxxxxxx,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,kernel-janitors@xxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,Alex Deucher <alexander.deucher@xxxxxxx>,"Christian König" <christian.koenig@xxxxxxx>,"Felix Kühling" <Felix.Kuehling@xxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,David Airlie <airlied@xxxxxxxx> > 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 > >> > > >
